feat(editor): split a query's duration into server, first row and transfer - #2609
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
PluginQueryResult.executionTimeis oneTimeIntervalthat every driver computes asDate().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
PluginQueryTimingcarriestotal, an optional client-measuredfirstRow, and an optional engine-reportedserver, withdatabaseTime = server ?? firstRow ?? totalandtransfer = total - firstRow. It rides onPluginQueryResultthrough 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:
mysql_fetch_row/mysql_stmt_fetchloopPQgetResultin single-row modePluginBoundedStream.collect, which is the path a capped editorSELECTtakeselapsed_nsfromX-ClickHouse-Summaryon the buffered pathendTime - startTimefrom the job statistics, on both the buffered and the capped pathEverything 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
COALESCEreads 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 ANALYZEas the server-time sources. Neither is the right mechanism:SHOW PROFILEShas been deprecated since 5.6.7, is off by default, and costs an extra round trip per query.EXPLAIN ANALYZEre-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, andMainContentCoordinator+ClickHouse.swift. None of them was ever assigned a non-nil value anywhere in the repo (installClickHouseProgressHandlerwas a documented no-op stub), and the deadlastClickHouseProgressbranch sat ahead of the duration branch inExecutionIndicatorView'selse ifchain, so it was directly in the path of the new display.PluginKit ABI 21
scripts/check-pluginkit-abi.shagainst the merge base reports an entirely additive diff with zero removed symbols (the one-line reappears as a+with only@_disfavoredOverloadadded, 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 -uon a rebuilt ClickHouseDriver:None of those exist in a v20 host, and ClickHouse and BigQuery are both registry-published. Left at 20, such a plugin passes
validateBundleVersionsin a shipped app and then failsBundle.loadAndReturnError.minimumCompatiblePluginKitVersionstays at 19.This carries the mandatory
scripts/release-all-plugins.sh 21before 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 mergesmainand keeps one 21, with a doc comment naming both reasons; the release still needs exactly onerelease-all-plugins.sh 21.Verification
verify.sh buildverify.sh buildMySQLDriver, PostgreSQLDriver, ClickHouseDriver, BigQueryDriverPluginverify.sh test(15 suites)verify.sh lint(7 paths)verify.sh docsverify.sh abi <merge-base>Re-run after merging
main(which brought in #2607 and #2608):buildPASS,testPASS 150 of 150 includingSQLExportDDLRewriterTestsandSQLExportForeignKeyOrderTests,docsPASS.verify.sh pluginsfails locally on the knownoracle-nio@TaskLocalmacro 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