Skip to content

feat(editor): split a query's duration into server, first row and transfer - #2609

Merged
datlechin merged 2 commits into
mainfrom
feat/query-timing-split
Sep 2, 2026
Merged

feat(editor): split a query's duration into server, first row and transfer#2609
datlechin merged 2 commits into
mainfrom
feat/query-timing-split

Conversation

@datlechin

@datlechin datlechin commented Sep 2, 2026

Copy link
Copy Markdown
Member

The problem

PluginQueryResult.executionTime is one TimeInterval that every driver computes as Date().timeIntervalSince(startTime) around the whole call: send, execute, transfer, and decode into Swift values. It is the only timing quantity the type system carries, so nothing downstream (the toolbar readout, the history row, the Query Insights ranking) can separate what the server did from what the wire did. On a remote database, or a large result over a tunnel, most of that number is transfer, so it says nothing about whether the query is slow.

A scalar where the phenomenon is multi-phase, so this is a refactor rather than a patch.

The fix

PluginQueryTiming carries total, an optional client-measured firstRow, and an optional engine-reported server, with databaseTime = server ?? firstRow ?? total and transfer = total - firstRow. It rides on PluginQueryResult through a new initializer; both published initializers keep their exact signatures and gain @_disfavoredOverload, because adding a parameter to one of them is what broke every registry plugin in 0.49.0.

Drivers fill in what they can honestly measure:

Driver How
MySQL, MariaDB First row from the mysql_fetch_row / mysql_stmt_fetch loop
PostgreSQL, CockroachDB, Redshift First PQgetResult in single-row mode
Any streaming driver First batch out of PluginBoundedStream.collect, which is the path a capped editor SELECT takes
ClickHouse elapsed_ns from X-ClickHouse-Summary on the buffered path
BigQuery endTime - startTime from the job statistics, on both the buffered and the capped path

Everything else leaves both nil and displays exactly as it did before.

The toolbar keeps the elapsed number as its label and opens a breakdown popover when there is a split to show. The history detail pane lists the parts in place of a single Duration row. Query Insights ranks on COALESCE(server_time, first_row_time, execution_time), so a query that is only slow to transfer stops reading as a slow query; a row written before this release coalesces down to its elapsed time and still ranks.

Storage gains two nullable REAL columns and a v5 migration with no backfill: there is no honest value to invent for an old row, and the COALESCE reads it exactly as its predecessor did.

Two of the issue's suggestions are deliberately not implemented

The issue asks for MySQL profiling and PostgreSQL EXPLAIN ANALYZE as the server-time sources. Neither is the right mechanism:

  • SHOW PROFILES has been deprecated since 5.6.7, is off by default, and costs an extra round trip per query.
  • EXPLAIN ANALYZE re-executes the statement, which is unacceptable for anything with side effects.

The client-side first-row split gives both engines an honest server figure for free. The documented caveat is that it carries one network round trip, which is exactly why the engine's own report wins wherever a protocol already sends one.

Also removed

ClickHouseQueryProgress, ConnectionToolbarState.clickHouseProgress / lastClickHouseProgress, and MainContentCoordinator+ClickHouse.swift. None of them was ever assigned a non-nil value anywhere in the repo (installClickHouseProgressHandler was a documented no-op stub), and the dead lastClickHouseProgress branch sat ahead of the duration branch in ExecutionIndicatorView's else if chain, so it was directly in the path of the new display.

PluginKit ABI 21

scripts/check-pluginkit-abi.sh against the merge base reports an entirely additive diff with zero removed symbols (the one - line reappears as a + with only @_disfavoredOverload added, and attributes do not participate in mangling). Additive is safe in the direction Library Evolution covers, so every already-built plugin keeps loading.

The bump is for the other direction, and it is measured rather than assumed. nm -u on a rebuilt ClickHouseDriver:

_$s17TableProPluginKit0C11QueryTimingV5total8firstRow6serverACSd_SdSgAGtcfC
_$s17TableProPluginKit0C11QueryTimingVMa
_$s17TableProPluginKit23ClickHouseSummaryParserO5parse7headersAC0G0VSgSDyS2SG_tFZ

None of those exist in a v20 host, and ClickHouse and BigQuery are both registry-published. Left at 20, such a plugin passes validateBundleVersions in a shipped app and then fails Bundle.loadAndReturnError. minimumCompatiblePluginKitVersion stays at 19.

This carries the mandatory scripts/release-all-plugins.sh 21 before or with the release. That is a publish action, so it is not run here.

#2607 landed the same bump first, for tableDDLIncludesForeignKeys. This branch merges main and keeps one 21, with a doc comment naming both reasons; the release still needs exactly one release-all-plugins.sh 21.

Verification

Step Result
verify.sh build PASS
verify.sh build MySQLDriver, PostgreSQLDriver, ClickHouseDriver, BigQueryDriverPlugin PASS
verify.sh test (15 suites) PASS, 180 of 180
verify.sh lint (7 paths) PASS, 0 violations
verify.sh docs PASS
verify.sh abi <merge-base> additive, 0 removals

Re-run after merging main (which brought in #2607 and #2608): build PASS, test PASS 150 of 150 including SQLExportDDLRewriterTests and SQLExportForeignKeyOrderTests, docs PASS.

verify.sh plugins fails locally on the known oracle-nio @TaskLocal macro incompatibility (unknown attribute 'usableFromInlinenonisolated'), which predates this branch and stops the whole aggregate. No other error appears in that log, and the four plugins this branch touches were built individually instead. CI runs the aggregate on its own toolchain.

New tests: QueryTimingTests, QueryTimingBreakdownTests, PluginBoundedStreamTimingTests, ClickHouseSummaryParserTests, QueryHistoryTimingTests (round trip, NULL reading back as absent rather than as zero, database-time ranking, and the v4 to v5 migration).

No UI automation: the popover only opens when a driver reports a split, which needs a live MySQL, PostgreSQL, ClickHouse or BigQuery connection, so it does not run deterministically in TableProUITests.

No before/after screenshots: capturing the popover needs a live remote connection for the split to exist at all, and the machine had no such connection available.

Fixes #2503

https://claude.ai/code/session_015EwoyM9XPxsVV75m48Noqa

@mintlify

mintlify Bot commented Sep 2, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Sep 2, 2026, 10:59 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

# Conflicts:
#	CHANGELOG.md
#	TablePro/Core/Plugins/PluginManager.swift
@datlechin
datlechin merged commit de35581 into main Sep 2, 2026
9 checks passed
@datlechin
datlechin deleted the feat/query-timing-split branch September 2, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate query time from network time

1 participant