Fix client-visible transaction status flag desync after DDL implicit commit - #3709
Open
flux423 wants to merge 1 commit into
Open
Fix client-visible transaction status flag desync after DDL implicit commit#3709flux423 wants to merge 1 commit into
flux423 wants to merge 1 commit into
Conversation
…commit
ServerInTransaction (SERVER_STATUS_IN_TRANS) was reported based solely on
ctx.GetTransaction() != nil, which is also true for the engine's implicit,
per-statement transaction used for ordinary autocommit statements. Combined
with a second bug -- TransactionCommittingIter.Close() clearing the
transaction on an implicit commit (e.g. a DDL statement issued mid-explicit-
transaction) without also clearing ctx.GetIgnoreAutoCommit() -- the very
next ordinary autocommit statement after such a DDL was misreported to the
client as still being inside a transaction.
This desyncs any MySQL client whose own BEGIN/COMMIT bookkeeping mirrors the
server status flag (e.g. PHP's PDO_MySQL, whose PDO::inTransaction() and
PDO::beginTransaction() guard read this exact flag): the client believes no
transaction is open (correctly, since it never issued a matching COMMIT/
ROLLBACK for anything), then refuses its own next explicit BEGIN client-side
with "There is already an active transaction", even though the server was
never asked to begin one.
Reproduced against Drupal core's `drush site:install standard`: an
unmodified Drupal installation using Drupal's stock mysql PDO driver failed
late in install (site-configure step) with exactly this PDOException. MySQL
8.0.46 does not exhibit this: DDL correctly implicit-commits and the
transaction-status flag correctly resets for subsequent autocommit
statements.
Minimal reproduction (both statements executed via PDO against a session
with an explicit BEGIN already in effect):
BEGIN;
CREATE TABLE t (id INT PRIMARY KEY); -- implicit commit
INSERT INTO t VALUES (1); -- ordinary autocommit statement
MySQL 8.0: PDO::inTransaction() is false after both statements.
Dolt (before this fix): PDO::inTransaction() is true after the INSERT,
with no BEGIN ever issued for it.
Fixes:
- sql/rowexec/transaction_iters.go: TransactionCommittingIter.Close() now
clears ctx.SetIgnoreAutoCommit(false) when committing an implicit
transaction, mirroring what the explicit COMMIT/ROLLBACK handlers in
sql/rowexec/transaction.go already do.
- server/handler.go: setConnStatusFlags() now additionally requires
ctx.GetIgnoreAutoCommit() before reporting ServerInTransaction, so an
implicit per-statement transaction can never set the client-visible flag
even if a future code path reintroduces a similar gap.
Verified: full `server` and `sql/rowexec` package test suites pass, plus
targeted `enginetest` transaction/DDL-implicit-commit suites. Re-ran
Drupal's `drush site:install standard` end-to-end against a Dolt build with
this fix applied (unmodified Drupal core, stock mysql driver, zero Drupal
patches): install now completes successfully.
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.
Problem
ServerInTransaction(SERVER_STATUS_IN_TRANS) is reported based solely onctx.GetTransaction() != nilinserver/handler.go::setConnStatusFlags().That's also true for the engine's implicit, per-statement transaction used
for ordinary autocommit statements — not just for an explicit client
BEGIN.Combined with a second gap —
TransactionCommittingIter.Close()insql/rowexec/transaction_iters.goclears the transaction on an implicitcommit (e.g. a DDL statement issued mid-explicit-transaction) but never
clears
ctx.GetIgnoreAutoCommit(), unlike the explicitCOMMIT/ROLLBACKhandlers in
sql/rowexec/transaction.go, which do — the very next ordinaryautocommit statement issued right after such a DDL gets misreported to the
client as still being inside a transaction.
This desyncs any MySQL client whose own transaction bookkeeping mirrors the
server status flag — notably PHP's PDO_MySQL, whose
PDO::inTransaction()and the guard inside
PDO::beginTransaction()read exactly this flag. Theclient correctly believes no transaction is open (it never issued a
matching
COMMIT/ROLLBACKfor anything), then refuses its own nextexplicit
BEGINclient-side, before any query reaches the wire, with"There is already an active transaction".
Real-world repro
Found via Drupal core's
drush site:install standard: an unmodified Drupalinstallation, using Drupal's stock
mysqlPDO driver (no custom driver, noDrupal patches), failed late in install (the site-configure step, after all
~30
standard-profile modules had already installed their schemasuccessfully) with exactly this
PDOException. The identical installcompletes cleanly against MySQL 8.0.46 with the same Drupal codebase and
command.
Minimal reproduction
Fix
sql/rowexec/transaction_iters.go:TransactionCommittingIter.Close()now calls
ctx.SetIgnoreAutoCommit(false)when committing an implicittransaction, mirroring the explicit
COMMIT/ROLLBACKhandlers.server/handler.go:setConnStatusFlags()now additionally requiresctx.GetIgnoreAutoCommit()before reportingServerInTransaction, asdefense-in-depth so an implicit per-statement transaction can never set
the client-visible flag even if a future code path reintroduces a similar
gap.
Testing
TestSetConnStatusFlagsInTransaction(server/handler_test.go),covering no-transaction, implicit-per-statement-transaction, and
explicit-client-transaction cases.
go test ./server/... ./sql/rowexec/...: pass.go test ./enginetest/... -run "Transaction|Commit|Autocommit": pass,including the
create_table_queries_are_implicitly_committedand relatedDDL-implicit-commit suites.
drush site:install standardagainst an unmodified Drupal 11.4-devcore — install now completes successfully (previously failed at the same
point on unpatched Dolt 2.1.10, 2.3.0, and
main).Related, separately-filed issues from the same investigation (distinct root
causes, not fixed by this PR): #3706 (constraint-violation SQLSTATE
mapping), #3707 (
SHOW INDEX ... WHERE key_namefilter ignored).