fix(sidebar): key per-schema object lists by database - #3095
Merged
Merged
Conversation
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.
Found while investigating #3048 (#3060). The two query storms from the same investigation follow in a PR stacked on this one.
The defect
On an engine grouped by hierarchical schema that changes database on a live connection (Snowflake and Trino), switching database left the previous database's objects under the new one's schema rows:
SALESon Snowflake and expandPUBLIC: it listsORDERS.MARKETING, which also has aPUBLIC.PUBLICunderMARKETINGstill listsORDERSuntil its reload comes round, and keeps listing it for good if that reload fails.Autocomplete and Open Quickly read the same lists, so they offered
SALEStables while browsingMARKETING. Oracle, BigQuery, Spanner, Dameng and Cloudflare R2 SQL are grouped the same way but never change database, so they could not hit it.Root cause
SchemaServicekept its per-schema lists inperSchemaStatesandperSchemaSideObjects, keyed by connection and schema name only (SchemaKey(connectionId, schema)). An in-place switch (DatabaseManager.switchDatabase, thePluginDriverAdapterbranch) never invalidatesSchemaService; only the reconnect-to-switch path does. So once the new database's schema list committed:PUBLICthat exists in both databases answered with the old database's tables untilrefreshLoadedSchemaObjectsreloaded it, one schema after another;loadSchemaObjectsreturned early for any schema still marked loaded, so expandingPUBLICunder the new database never fetched it;Open Quickly already carried a workaround for this: on a hierarchical engine the all-schema listing overrode the schema service once it arrived. The investigation behind #3063 hit the same missing key independently and cut its catalog-based path for it.
The fix
A refactor rather than a patch: the key was missing a dimension, and the workaround downstream was compensating for it.
SchemaKeycarries the database. Every per-schema load names the scope it reads (loadSchemaObjects(schema:in:driver:),reloadSchemaObjects(schema:in:driver:),refreshLoadedSchemaObjects(in:driver:)), and its rows are kept under that scope's database.loadedScopes), so the schema rows and the objects under them always describe one database, including while a switch settles.noteScopeCoveredno longer records a scope in another database as covered by what is loaded.Measured
Mock driver counting
fetchTables(schema:), Snowflake connection, the refresh that follows a switch fromSALES(two schemas expanded) toMARKETING:MARKETINGon behalf ofSALES's loaded schemasPUBLICunderMARKETINGright after the switchORDERS, fromSALESThe before column comes from the code path (
refreshLoadedSchemaObjectsreloaded every loaded or loading key of the connection). My red run of the new suite against unmodified main was lost to a full disk on the build machine (every case reported at 0.000 s with ENOSPC in the log), so the before numbers are not a measurement. The edit that turns the suite red is droppingdatabasefromSchemaKey, which is the old shape.Tests
New
SchemaServiceDatabaseSwitchTests(6 cases): a switch does not carryPUBLIC's tables or routines across; a failed load after a switch reports the failure instead of the old tables; the refresh after a switch reads nothing for the old database; a load for the old database finishing after the switch is discarded; objects loaded for the new database before its schema list arrives are kept without a second read; switching back reads the first database again.Updated in the same commit for the new signatures:
SchemaServiceTests,SchemaServiceRefreshTests,SchemaServiceHierarchicalTests,SchemaServiceSideObjectsTests,QuickSwitcherCrossSchemaTests(the hierarchical exception's two cases replaced by one asserting a loaded hierarchical schema keeps the listing's stale rows out, and one for the stand-in before the listing arrives).Verification
verify.sh testoverSchemaServiceDatabaseSwitchTests SchemaServiceTests SchemaServiceRefreshTests SchemaServiceHierarchicalTests SchemaServiceSideObjectsTests SchemaServiceRoutinesTests SchemaRefreshServiceTests SchemaRefreshAfterWriteTests QuickSwitcherCrossSchemaTests QuickSwitcherViewModelTests CatalogEditAdoptionTests: PASS, 155 executed, 155 passed (the build step is part of the run).verify.sh linton the 9 changed Swift files: 0 violations.Reviewed with a
feature-dev:code-revieweragent (Codex is out of quota until Sep 29). It found nothing at its confidence bar; its one coverage note, the side-object lists across a switch, is now asserted inswitchDoesNotCarryTablesAcross.No UI automation: reproducing it needs a live Snowflake or Trino server, and the switch is covered at the service boundary the sidebar reads.