Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…ion after the run as well as before
…peats, startup commands or imports
…real table's name
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
…e-forgets-table-settings # Conflicts: # CHANGELOG.md # TablePro/Core/Utilities/SQL/SQLTokenCursor.swift
This branch was successfully deployed
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.
Root cause
A table's saved settings (filters, column layout, highlight rules, value display formats, FK label columns), its favorite, its Recent entry and any queued Truncate or Drop are adopted by
CatalogEditAdoption.adoptDroppedTablesandadoptTableRename. Only two events reach them:CatalogEvent.tablesDropped, posted by the sidebar Drop save path, and.tableRenamed, posted by the sidebar Rename. SQL run in a query tab, and SQL an MCP client runs, posts.statementsRan, which only schedules a catalog refresh. SoDROP TABLE people; CREATE TABLE people (...)left the old table's filters, formats and highlight rules on the new table (a saved filter on a column the new table lacks opens the tab on a server error), kept its star and Recent entry, and left a queued sidebar Drop armed against the new table..statementsRancannot carry this. It is posted for failed statements too, on purpose, because a refresh errs toward running. An adoption that forgets settings has to err the other way.Fix
A second event,
CatalogEvent.statementsSucceeded(SucceededStatements), carries only the statements the server accepted, theDatabaseScopethey started in, and what the session said about committing them.CatalogChangeServiceturns it into drops and renames and feeds each one, in order, through the samerecordDroppedTablesandrecordRenamedTablethe sidebar uses, so tabs close or take the new name exactly as they do for a sidebar drop or rename.TableEditStatementParserreadsDROP TABLE|VIEW|MATERIALIZED VIEW|FOREIGN TABLE [IF EXISTS] a, b [CASCADE ...],ALTER TABLE|VIEW ... RENAME TO|AS, MySQL's bareRENAME b,RENAME TABLE a TO b, c TO d [ON CLUSTER]and SQL Server'sEXEC sp_rename 'schema.old', 'new' [, 'OBJECT'], plus transaction control,USE, schema changes, temporary-table creation, and procedure calls and anonymous blocks. It works onSQLTokenCursor, which now exposes the spelling of the last token so an unquoted name keeps its case where the engine preserves it. A statement is read only when it is exactly one of these forms; a column rename, an unknown clause, or a second T-SQL command without a;makes it.other. A conditionalALTER TABLE IF EXISTS ... RENAMEis not read, because PostgreSQL answers it with a notice when the table is missing, and reading it as a rename would move stale settings over the target's own.TableEditDialectplaces a name per engine: fold to lowercase (PostgreSQL, Redshift) or uppercase (Oracle), otherwise keep it; what a two- and three-part name means; whetherUSEselects a database. Covered: the MySQL family, the PostgreSQL family and Redshift, SQLite, SQL Server, Oracle, ClickHouse, DuckDB. CockroachDB, Snowflake and everything else answer nil and are left alone.set_config, a function or an earlier run (the driver re-pins the schema only when its own record differs), or a temporary table made bySELECT ... INTO TEMPor a function, changes where it resolves, so there only a qualified name is placed. SQL Server resolves a bare name against the login's default schema, so the same holds. On MySQL, SQLite, ClickHouse and Oracle a bare name is placed unless the connection'sTableNameHazardssay otherwise. That record only grows: a temporary table of that name (CREATE TEMPORARY TABLE, SQLite'sCREATE TABLE temp.x), a schema change such asALTER SESSION SET CURRENT_SCHEMA, or code the server ran unseen (CALL,EXEC,DO, a PL/SQL block, MariaDB'sBEGIN NOT ATOMIC). It is read from every statement that ran, failed ones included, because a procedure that failed can have created a temporary table first, and also from the connection's Startup Commands, which run on every connect before the session reports anything, and from the statements a SQL file run through Import Data sends, since the import runs on the editor's own session. A MySQL temporary table also hides the real one under its qualified name, so there the record covers qualified names too. Temporary names are compared case-insensitively, as SQLite finds them. Names insidepg_temp,pg_temp_Nor DuckDB'stempcatalog are never placed, an Oracle global temporary table is treated as the permanent table it is, and a drop or rename inside a MySQL/*!NNNNN ... */comment is not read, because an older server skips the body and still answers success.CommittedTableEditsdecides what is committed. MySQL, MariaDB, Oracle and ClickHouse commit DDL as it runs, so success is enough. On a transactional engine a lone statement counts only when the session holds no transaction after it (read inside the same lease throughheldSessionTransactionState(), and only for a statement that edits a table). A run counts only when the session held no transaction both before its first statement and after its last, both read inside the run's lease; the multi-statement run asks the second time only when it drops or renames a table, because on SQL Server asking is a round trip. A run the app wrapped keeps its edits pending until the app'sCOMMIT, since a bareROLLBACKin the script does not stop the app wrapping it and is what ends the app's transaction. The text's ownBEGIN,COMMIT,ROLLBACK,ROLLBACK TO,SAVEPOINTandRELEASEare followed with a nesting depth, soBEGIN; DROP TABLE public.people; ROLLBACKchanges nothing and an edit still open when the run ends is dropped. A run the app wrapped and then rolled back counts for nothing.SET IMPLICIT_TRANSACTIONS,COMMIT AND CHAINandPREPARE TRANSACTIONstop the tracking for the rest of the run. The text alone cannot be trusted even between two idle answers: under SQL Server'sIMPLICIT_TRANSACTIONS, set by an earlier run or by the server'suser options, aDROPopens a transaction no statement shows,BEGIN TRANtakes@@TRANCOUNTto 2 and the script'sCOMMITleaves it at 1. So a run whose text closes a transaction it never opened, or leaves one open that the session says is closed, adopts nothing. On PostgreSQL a strayCOMMITorROLLBACKis only a warning, so there this holds back a drop that did commit.IF,ELSE,WHILE,GOTO,RETURN,BREAK,CONTINUE, a label or aTRY...CATCHcan skip one or swallow its error. A SQL Server run holding any of them adopts nothing. A batch run withGO nis replayed n times, so a rename chain run twice moves nothing.ScriptBatchProgressbox, with what the session held after the failing batch, because SQL Server committed them unless a transaction is still open.CatalogEditAdoptiontakes its stores and its favorites storage by injection, matches queued operations by the object they name (TableScope) instead of byDatabaseTreeTableRefequality, and builds a SQL-named table's reference with the favorite spelling the sidebar saved, since the tree keys a favorite by the driver listing's schema, which Oracle, MySQL and SQLite leave empty.Review
Four Codex review rounds and one independent review. Round one found a no-op
ALTER TABLE IF EXISTSrename moving settings, temporary tables not tracked across runs,sp_renamemissing, and the successful prefix of a failed MCP script going unreported; all four were fixed. Round two found new defects in the round-one temporary-table tracking (unobserved creation forms, session keying, rollback) and the cross-run search-path drift. Rather than patch those, bare names were cut back on PostgreSQL, Redshift and DuckDB, and the per-connection record became one that only grows, so nothing can take a hazard back wrongly. Two round-two findings are under-adoption, which leaves settings where they already are, and are listed below. Round three found three cheap, real defects, fixed without reopening the loop: aROLLBACKinside a run the app wrapped, a temporary table matched case-sensitively on SQLite, and DDL inside a version-gated MySQL comment. Its fourth finding, SSH-backed SQLite answering.unknownfor its transaction state so a lone statement there adopts nothing, is under-adoption and listed below.An independent review then found two more. SQL Server implicit transactions set by an earlier run let an uncommitted
DROPin aGOscript or an MCP script be adopted, because the evidence carried only the session's state before the run: fixed by carrying the state after it too, and by treating a strayCOMMITorROLLBACK, or a transaction the text leaves open, as the text and the session disagreeing. And the CHANGELOG entry claimed every SQL drop, while a SQL file run through Import Data still only refreshes: the entry now names query tabs and MCP clients, and the docs say Import Data and restores keep table settings. A fourth Codex round on that found three defects in the original design: hazards from Startup Commands and SQL import never recorded, T-SQL control flow skipping a statement the batch still listed, andGO nreplayed once. All three are fixed. A fifth Codex round found two more gaps in the hazard record, both fixed: a temporary table renamed onto a real table's name (ALTER TABLE scratch RENAME TO peopleon SQLite) now keeps shadowing under the new name, and SQLite'sCREATE VIRTUAL TABLE temp.people USING fts5(...)is read as a temporary table. It also raised three that are left, listed below with the reason.Tests
Built and tested with
verify.shin the worktree.TableEditStatementParserTests,TableEditDialectTests,CommittedTableEditsTests,SucceededStatementsProbeTests,ScriptBatchProgressTests,BatchStatementOutcomeSucceededCountTests,SQLTableEditAdoptionTests, and one case inSQLTokenCursorTests. WithDatabaseAccessBridgeScriptTests,DatabaseAccessBridgeStatementTests,MCPScriptResultTests,ScriptResultEncoderTests,CatalogChangeServiceTestsandCatalogChangeTestsin the same run: 134 cases, all passed.SQLTableEditAdoptionTestsdrivesCatalogChangeService.record(...)into a realCatalogEditAdoptionwith a recording settings store and an isolated favorites storage: a drop, a rename, a rolled-back drop, an Oracle favorite saved without a schema, a queued sidebar Drop unstaged when SQL drops and recreates the table, a MySQL temporary table across runs, a failed procedure call reported only through.statementsRan, and a no-op conditional rename.BatchStatementRunTests,CatalogChangeClassifierTests,LoadedBrowseCatalogTests,RestoreStagedTableOperationsTests,SavedConnectionDatabaseAdoptionTests,SchemaServiceStaleSchemaTests,SQLSetAssignmentsTests,TableScopedSettingsRegistryTests,TabQueryTaskGuardTests,FavoriteTablesStorageTests,MainContentCoordinatorLazyLoadTests,MainContentCoordinatorSortTests,TabQueryIsolationTests,MySQLKillLatchTests, plusDatabaseAccessBridgeStatementTests: 202 cases, all passed on the commit before the round-three fixes, which touch only the parser, the walker and the evidence type covered by the run above.CommittedTableEditsTests,SucceededStatementsProbeTests,ScriptBatchProgressTests,ExecutedStatementTextsTests,ImportNameHazardTests,BatchStatementOutcomeSucceededCountTests,SQLTableEditAdoptionTests,TableEditStatementParserTests,TableEditDialectTests,DatabaseAccessBridgeScriptTests,DatabaseAccessBridgeStatementTests,MCPScriptResultTests,ScriptResultEncoderTests,ExplainAuthorizationTests,TabQueryIsolationTests,CatalogChangeServiceTests,BatchStatementRunTests,MultiStatementFailureTests,QueryBatchPlannerTests,SQLServerImportBatchTests,ImportDataSinkAdapterMappingTests,SQLTokenCursorTests: 268 cases, all passed. New cases cover a SQL Server run ending inside a transaction, a strayROLLBACKandCOMMIT,BEGIN TRAN ... COMMITunder implicit transactions, a transaction closed unseen, an MCP script whose session ends inside a transaction (throughScriptBatchRunandScriptBatchProgress),GOTO,IFandTRY...CATCHbatches, aGO 2rename swap, a temporary table from Startup Commands, an import's hazard statements, a renamed temporary table and a temporary virtual table.swiftlint lint --stricton every changed Swift file: 0 violations. Docs:check-writing-style.shandcheck-docs-against-source.pypass.Not verified, and what is left
IMPLICIT_TRANSACTIONS(@@TRANCOUNT0 before the first statement,BEGIN TRANcounting 2) is from Microsoft's documentation and the driver's own measured comment, not measured in this branch.COMMITorROLLBACKon PostgreSQL holds a run's drops back; a SQL Server run holding any control flow adopts nothing, including drops in its other batches; an MCP script stopped by a timeout adopts nothing, because the session's end answer never came.GO nbatch whose first repetition committed and whose second failed counts as a failed batch and adopts nothing; carrying the repetition count through the batch outcome is its own change, and the gap only leaves settings in place. Savepoints are counted as nesting, soBEGIN; SAVEPOINT s; DROP ...; COMMIT(and SQL Server'sSAVE TRAN, which does not raise@@TRANCOUNT) ends the walk one level deep and holds the drop back; a savepoint stack per engine would fix it, again under-adoption only. And a procedure that drops and recreatespublic.peoplefollowed byALTER TABLE public.people RENAME TO personsmoves the name's settings topersons: settings are keyed by name, the hidden drop already left them on the recreated table before this change, and refusing the visible rename would only strand them under a name that no longer exists. Transaction state after a statement comes from each driver'ssessionTransactionState(); a driver that answers.unknownon a transactional engine (remote SQLite over SSH, PGlite, libSQL, Turso as far as I can tell) adopts nothing from a single statement.DROP TABLE peoplethere still leaves the settings. Placing it safely needs the server's answer rather than the text's: the session'scurrent_schemas(true)read inside the lease after the statement, or a check that the placed table is gone, per statement. That is its own change.sp_renamewith named arguments; Oracle's keyword-lessRENAME a TO b, which resolves in the login's own schema; a rename that moves a table to another database or schema (ALTER TABLE ... SET SCHEMA, MySQLRENAME TABLE a TO other.a);CREATE OR REPLACE TABLE;DROP DATABASEandDROP SCHEMArun as SQL (the sidebar's container drop has its own adoption); dependents aCASCADEdrop takes with it; SQL import runs; a function called from a query that creates a temporary table on MySQL or SQLite.DROP TABLE IF EXISTS t; CREATE TABLE t ...in a query tab now closes open tabs ontand forgets its saved settings where the name is placed, the same as a sidebar drop would.Found while investigating #3172 (#3183)
CI fixes
The failed Package Tests job was
SyncRecordMapperTests.unknownWireValueFailsClosed: the sync mapper's duplicate decoder turned an unknown Safe Mode value into Off. It now usesSafeModeLevel(wireValue:isReadOnly:), requiring confirmation while retaining legacy read-only restrictions. Added regressions for unknown read-only values and renames that preserve an unknown wire value.Integrated
main, preserving both sets of changelog entries. Fixed the confirmed SurrealDB compile error introduced by the integration:textcalled a length-taking JSON helper that had become a property. Restored that helper, retaining valid display truncation and complete export text.Validation for these fixes:
swift test --package-path Packages/TableProCore: passed (1,197 Swift Testing tests plus XCTest).TableProApp/rust-dameng,Contents/MacOS,Contents/Helpers, and/Applications/Xcode-beta.app.GitHub Actions rerun after pushing these fixes.