fix(editor): decide a batch's transaction from the engine, the script and the session - #2971
Merged
Merged
Conversation
…-and-legacy-mysql-gaps # Conflicts: # Plugins/MySQLDriverPlugin/MySQLPluginDriver+Schema.swift # Plugins/MySQLDriverPlugin/MySQLPluginDriver.swift # TablePro/Core/Coordinators/PaginationCoordinator.swift # TablePro/Core/Coordinators/QueryExecutionCoordinator+Parameters.swift # TablePro/Core/Plugins/PluginManager.swift # TablePro/Views/Main/MainContentCoordinator.swift
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
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.
Nine defects found while investigating #2949, fixed together because six of them meet in one function: the multi-statement runner in the SQL editor.
What was wrong
The batch wrapped everything in a transaction, whatever the script or the connection was doing.
VACUUM,CREATE INDEX CONCURRENTLY,ALTER SYSTEM,DISCARD ALL; SQLiteVACUUM,DETACH,PRAGMA journal_mode; DuckDBCHECKPOINT; MySQLSET sql_log_bin(error 1694, the first line of a GTIDmysqldump).PRAGMA foreign_keyswas worse: accepted, silently ignoredCOMMITcommitted their work, itsROLLBACKdiscarded it. MySQL:START TRANSACTIONcommitted it and releasedLOCK TABLES. SQLite refused the nestedBEGIN; DuckDB and CockroachDB aborted the user's transactionCOMMITMULTI/EXEC: every result readQUEUED,DELreported 0, andEXEC's per-command errors were thrown awayAnd four legacy-server defects, each of which reported success:
SET SESSION max_execution_time(MySQL) ormax_statement_time(MariaDB) whatever the server was. MySQL before 5.7.8 and MariaDB before 10.1.1 answerERROR 1193, so there was no timeout at all; the socket timeout then fired and the driver re-ran the statement while the first copy kept running on the server.CHECKand discard it.ALTER USER … IDENTIFIED BYandWITH MAX_USER_CONNECTIONS, which MySQL before 5.7.6 and MariaDB before 10.2 reject.START TRANSACTION, so a server whose sessions default to read-only refused it with 1792 where the Mac succeeded.What it does now
One decision, made once per run.
BatchTransactionPolicy.plan(for:databaseType:rules:)returns aBatchTransactionPlan:appTransaction: the batch owns a transaction, as before.scriptTransaction: the script opens its own (BEGIN,START TRANSACTION,XA START,SET autocommit), so nothing is wrapped around it.autocommit: the script holds a statement the engine refuses inside a transaction, so every statement commits as it runs.sessionTransaction: the connection already has one open, so the batch joins it and sends noBEGIN,COMMITorROLLBACK.The engine rules are per family (
TransactionEngineFamily, curated byDatabaseType, so DuckDB is not SQLite and SQL Server is not generic) and read with a lazySQLTokenCursorthat never copies the statement: 100,053 statements of a realmysqldumpclassify in 80ms. Redis is a family whose batch is never wrapped.Session state is a driver question, so PluginKit gained
sessionTransactionState()with an.unknowndefault: libpq fromPQtransactionStatus, MySQL from the server'sSERVER_STATUS_IN_TRANSflag, SQLite fromsqlite3_get_autocommit, DuckDB and SQL Server through their own probes. Grid saves, structure changes and a Users & Roles apply take the same answer, and report their statements as pending rather than written when they join a transaction the user must still commit.The commit is a point of no return, and the UI says so. It runs shielded from cancellation, Stop is unavailable for that one round trip, and a lost connection there reports that the statements may or may not be saved rather than claiming a rollback.
Cancellation is per tab.
TabQueryTasksreplaces the window's single query task,cancelRunningQueryreaches only the owner's leases, and a second tab waits for the connection instead of stopping the first.Legacy servers: the timeout enforcement comes from the server's own answer (
ERROR 1193switches to a client-side deadline that kills the statement from a second connection), a statement that outlasts the socket timeout is never replayed and its orphan is killed, check-constraint editing is withdrawn with the reason where the server discards it, account statements fall back toGRANT USAGE … IDENTIFIED BY, and iOS opens its write transactions read-write.PluginKit
Version 33, additive:
sessionTransactionState()andcheckConstraintRefusal, both with defaults, plus one non-frozen enum.scripts/check-pluginkit-abi.shagainstorigin/mainreports those additions and no removals, sominimumCompatiblePluginKitVersionstays 19 and nothing needs re-publishing. DuckDB and SQL Server are registry-only, so they answer.unknownuntil their plugins are next published at kit 33.Verification
check-ios-shared-isolation.pyPASS.QueryRunUITestsandQueryErrorBannerUITests, 22 cases, PASS, including four new end-to-end cases on the SQLite sample (a script with its ownBEGIN, a failed script's transaction rolled back,PRAGMA foreign_keysapplied in the same script,VACUUMin a batch).swiftlint --strictover all 181 changed Swift files: clean of anything this branch introduced.check-writing-style.sh,check-docs-against-source.py,mint validateandmint broken-linksall pass.check-mysql-autocommit-only-variables.sh,check-mysql-query-timeout.sh,check-redis-multi-semantics.sh.Measured against MySQL 5.5.62, 5.6.51, 5.7.44, 8.4.11, 9.7.2, MariaDB 5.5.64, 10.0.38, 10.1.48, 10.6.28, 11.4.13, 12.3.3, Percona 5.5.61, PostgreSQL 17.11, CockroachDB 25.2.23, SQLite 3.54.0, DuckDB 1.5.4 and Redis 8.10.1.
Known gaps
autocommit; the rows are in the database and a refresh shows them.mysqldumprestored through File > Import still hits error 1694. It streams the file, so it needs its own fix.UITestCase's editor lookup is ambiguous and the tab strip's accessibility snapshot stops refreshing while the connection is busy. That second point may be a real defect of its own and is worth a look.Review
A cold-read review of the whole diff found six defects, all fixed here with regression tests: a failed statement clearing the MySQL session-lock flag (so the idle release could hand back a connection holding the user's lock), a second timeout statement's
ERROR 1193installing a client-side kill on top of a working server one, a Redis command on the streamed route throwing instead of reportingQUEUED, object copy wrapping a transaction the session already held, Stop removing a batch's cancellation handle before deciding the commit was uninterruptible, and a cancelled Fetch All never retiring its task entry. The last of those is pinned by a test verified to fail without the fix.Codex could not be used for the review: the account is over its usage limit until 22 September.