perf(compare): read a whole schema in a few queries and make a saved comparison set both endpoints - #2597
Merged
Merged
Conversation
…comparison set both endpoints Claude-Session: https://claude.ai/code/session_01HmAbnPkpZeruKdVUG7gzgS
…uginKit to 20 Claude-Session: https://claude.ai/code/session_01HmAbnPkpZeruKdVUG7gzgS
|
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. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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.
A licensed user reported Compare & Sync as "slow and not very easy (needs many steps)". Both halves are real and both have a single cause.
Slow: the read was one statement per table, per kind, per side
CompareMetadataService.tableReadsasked for columns, indexes, foreign keys and table metadata one table at a time. On MySQLfetchColumnsis itself two statements (SHOW FULL COLUMNSplus a generation-expression lookup), so a table cost five round trips. Source was read to completion, then target. A 200-table pair cost2 + 10 × 200 = 2002round trips before one difference appeared.Two things made it worse than it looks.
The existing concurrency was a no-op.
CompareMetadataServicefanned out four tasks against the driverwithMetadataDriveryields, but that is one driver, and a driver dispatches its statements onto its own serial queue (MariaDBPluginConnection.swift:153). The four tasks queued behind each other. Its gate wassupportsConnectionPooling, which answers whether a second connection is safe, not whether one connection can run two statements at once. Removed.PluginKit already had three of the four whole-schema reads and the comparison called none of them.
fetchAllColumns,fetchAllForeignKeysandfetchAllTriggersare single queries on every flagship driver. There was no whole-schema read for indexes or table metadata at all.You cannot just switch to the bulk calls, which is why this is not a one-line change:
fetchAllColumnsagainstfetchColumnsgenerationExpressionandgenerationKindpragma_table_info, which omits generated columns entirely (its ownfetchColumnsusestable_xinfofor exactly that reason)A naive switch would have silently stopped reporting generated-column differences on MySQL and made generated columns vanish from a SQLite comparison. Both are fixed first, and both drivers now share one shaping function with their per-table read so the two cannot drift again.
What the read does now
fetchAllIndexes(schema:)andfetchAllTableMetadata(schema:)added toPluginDatabaseDriverwith N+1 default implementations, plusprovidesBulkColumnFetch/providesBulkIndexFetch/providesBulkTableMetadataFetch/providesBulkTriggerFetchalongside the existingprovidesBulkForeignKeyFetch. Implemented for MySQL, PostgreSQL and SQLite. This bumps PluginKit to 20, see below.CompareMetadataService.readbuilds a read plan: whole-schema where the driver declares it, per-table where it does not, and per-table always for a caller that named specific tables (Copy Objects), which would otherwise read the rest of the database to throw it away.SessionDriverGateis a FIFO queue per connection, not a lock a task deadlocks itself on, and these are concurrent rather than nested, so the worst case (both sides on one connection's shared driver) is that they serialise, which is what they did before.A 200-table structure comparison now costs four statements a side instead of a thousand, and the two sides overlap.
Many steps: the feature that saves the setup could not be reached without redoing the setup
CompareSyncSession.apply(_:)restored the mode, the kinds and the options and never setsourceortarget. MeanwhilesavedProfilesfiltered on the current source, target and mode, so a saved comparison only appeared once its own two endpoints had been picked by hand. The stored scopes were used for nothing but the storage key.Found while working on this: the Apply sheet's hazard allowance was unreachable
applyDisabledReasonreturned a reason while any statement carried an unacknowledged hazard, andpresentApplySheetguarded oncanApply. So the sheet could only open once every hazard was already allowed, and the allowance UI inside it (added specifically because "the only checkbox that could allow one lived in the script pane behind this sheet") never rendered. ItsonAppear, which opens the sheet on the Warnings tab when something is held back, could not fire either.The gate moved to where the control is:
applyDisabledReasonasks only whether there is something to apply, and the newrunRefusalReasonis what the sheet's Apply button andCompareRunner.apply()both answer to, so an unallowed hazard still stops the run. The sheet's own copy said held-back statements "stay out of this run", which contradicted a disabled button; it now says what actually happens.Two smaller ones, both on this path:
fetchIndexesmatched onrelnamewith no namespace predicate, so two schemas holding a table of the same name returned each other's indexes merged into one list. A comparison between those two schemas reported neither side as differing. The page documents two schemas in one database as a valid pair.Fencing the async work
Both new flows publish after an await, and the pickers stay live across it.
CompareSyncSession.setupGenerationis bumped by everyresetComparison(), which is every path that changes an endpoint, the mode or an option; each async publisher captures it and drops its result if it moved. Without it, changing the target whilebuildScriptIfNeeded()was in flight would have opened the Apply sheet labelled with the new target over a script built for the old one, and a data-plan load could publish one pair's tables, columns and snapshots into another pair's session.The same generation is now part of the toolbar's chrome key, so loading a comparison for the pair already on screen still triggers the reload it promises.
Two related gates came with it. Loading a saved comparison is refused while a comparison, build or apply is running, because a run owns the target it captured; and the "this profile names a connection that no longer exists" message moved to a
setupErrorMessagethat survives the reset the load itself causes, since the option changes a load makes fire their ownonChangereset and were wiping it.PluginKit 20, and why the additive rule was not enough
CLAUDE.mdsays an added requirement with a default needs no version bump, and for the direction it describes that holds: an already-built v19 plugin keeps loading in this app. The reverse direction is not covered and does not hold.A
CassandraDriverrebuilt from this branch implements none of the new requirements and still imports all six as undefined symbols:None of those exist in main's PluginKit, and
validateBundleVersionsonly rejectsversion > currentPluginKitVersion. A plugin rebuilt from this branch and stamped 19 would therefore be accepted by the shipped v19 app and then failBundle.loadAndReturnError, taking that driver away.So
currentPluginKitVersiongoes to 20 and every pluginInfo.plistwith it, whileminimumCompatiblePluginKitVersionstays 19. A v19 app now refuses a v20 plugin cleanly and says to update; a v19 plugin still loads here.Important
This carries
CLAUDE.md's mandatory post-bump checklist.scripts/release-all-plugins.sh 20has to run before or with the app release, or users on the new app hitnoCompatibleBinaryuntil the registry catches up.Verified
verify.sh buildverify.sh test(6 compare suites)verify.sh lint TablePro Plugins TableProTests TableProUITestsverify.sh docsverify.sh abi mainscripts/check-sqlite-bulk-metadata.shCassandraDriver,CloudflareD1DriverPlugin,DamengDriver,LibSQLDriverPlugin,MSSQLDriver,TeradataDriverall build against PluginKit 20verify.sh pluginsfails locally onOracleNIO's@TaskLocalmacro (unknown attribute 'usableFromInlinenonisolated'), which is a known toolchain difference in this checkout and not related to this change: the error is inside the vendored package, and the Oracle plugin's own edit here is a singleprovidesBulkTriggerFetchline. CI compiles the aggregate.Two Codex passes ran over this branch. The line-level review found seventeen issues, six P1; the adversarial pass returned "do not ship" with six more. All but one are fixed above.
The adversarial pass is what caught the PluginKit direction, the Oracle and Dameng trigger scope, the case-folded lookup cross-assignment, and the fact that reusing preloaded data plans across an explicit Compare reintroduced a stale-key path that could address the wrong rows. Its remaining recommendations that are already implemented here: one immutable run claim per run, committed once; a script revision distinct from the setup generation; and a metadata refresh on every explicit Compare. The one dismissed is its reading of the
CLAUDE.mdno-comments rule against the///blocks in this diff: every file in this subsystem records the defect behind a rule that way, so the diff follows the surrounding code. One block that narrated mechanism rather than reason was trimmed.New coverage:
CompareMetadataReadPlanTestscounts round trips against a recording driver (a whole-schema driver is asked four times for 200 tables; a driver without the bulk reads still reads every table; a narrowed read stays per-table; a failed whole-schema query falls back per table; one unreadable table does not lose the others).CompareSyncSetupRestoreTestscovers loading, restoring and the rule that a target is never inherited across an unrelated source.UI automation: the Compare & Sync window is license-gated and the UI test sandbox carries no license, so the window itself cannot be driven from XCUITest.
CompareSyncUITestsgains the one assertion that does run without a license, that Save Comparison is in the menu bar; the window's behaviour is covered by the unit suites above, which is whereCompareSyncStartStateTestsalready put it for the same reason.Screenshots:
docs/images/compare-sync-window.pngnow shows a toolbar without the Comparisons button and needs re-capturing. It could not be captured here for the same reason as the UI tests: the window does not open without a license.