fix(frontend): support MySQL session reset commands - #27736
fix(frontend): support MySQL session reset commands#27736iamlinjunhong wants to merge 10 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 9129a6a.
The partial temporary-table DROP issue is fixed: a reset error after physical table cleanup is now marked terminal and the wire path disconnects. Two blockers remain.
[P1] Disconnect COM_RESET_CONNECTION when transaction rollback fails after mutating the old generation.
closeForReset returns a rollbackWithContext error directly. But rollbackUnsafe always invalidates txnOp and clears the frontend transaction ID after txnOp.Rollback returns, including the error and timeout paths. handleSessionCommand disconnects a failed COM_RESET_CONNECTION only when the error contains errSessionResetConnectionMustClose, which this rollback error does not.
Concrete path: an active transaction enters COM_RESET_CONNECTION -> storage Rollback returns an error or the reset deadline fires -> the transaction handle is invalidated -> the handler sends ERR without Disconnect -> the old session remains usable with its transaction removed but its user variables, prepared statements, temporary tables, database, and session settings still retained. This is another partially reset generation. TestRoutineResetSessionFailureRestoresProtocolState currently codifies the reuse by issuing a second reset, but it does not prove that ordinary commands after the first ERR see an untouched generation.
Please mark rollback failure terminal once rollbackUnsafe has mutated the generation, and add a handler/wire regression proving Disconnect occurs for rollback error and timeout.
[Blocking validation gap] Exercise the actual client path named by #27644.
TestMySQLWireChangeUserAuthSwitchAndRepeatedBorrow is useful protocol-loop coverage, but it is a synthetic net.Pipe client with SkipCheckUser=true and covers only COM_CHANGE_USER. It does not run MysqlConnectionPoolDataSource, does not send the modern Connector/J COM_RESET_CONNECTION path, and does not prove pooled cleanup of an active transaction, temporary tables, user variables, prepared statements, and session defaults. The linked severity/s0 issue explicitly requires that client-level regression, and exact-head CI contains no equivalent JDBC/motr evidence.
Please add or attach exact-head Connector/J/motr evidence for both the modern reset path and the 8.0.15 change-user fallback, including repeated logical borrows, stable physical connection ID, state cleanup, and replacement after reset failure.
I reviewed all changed production/test hunks, packet parsing, auth-switch sequencing, generation admission/publication, account/session registration, metrics, cleanup ownership, timeout/cancel paths, and Q1-Q3. No additional confirmed blocker remains.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review on exact head 8cc0fa81cdd9e341ceebc0e7488ada4404f2225f. The previous rollback-error generation bug is fixed: rollback failure/timeout is now terminal and the wire handler disconnects. One protocol-correctness blocker remains, and it also demonstrates why the requested client-level validation is still mandatory.
- [P1]
COM_RESET_CONNECTIONmust preserve the selected database. The new handler calls the existing Proxy reset helper (pkg/frontend/routine.go:456-461), but that helper unconditionally snapshots the protocol DB and sets it to""before constructing the replacement (pkg/frontend/routine.go:902-907); on successful reset it never restorespreviousDB. This is not MySQLCOM_RESET_CONNECTIONbehavior. MySQL’s own reset regression doesUSE wl6797, resets, then executes unqualified table references inwl6797; Connector/J 8.4/9.7 sends COM_RESET and reapplies charset/session variables, but does not issueUSEafterward. With a JDBC URL such as/test, the first pooled borrow can therefore pass reset and then fail normal unqualified SQL with no database selected.
I reproduced this deterministically by setting the existing successful-reset test session DB to db1 and asserting the replacement DB: focused mo-cgo-test failed with expected db1, actual empty. Please preserve the current DB for the wire COM_RESET path (without accidentally changing Proxy reset semantics if Proxy intentionally needs an empty DB), and cover it in both the wire/client regression and UT.
- [Blocking validation gap] The severity/s0 client path from #27644 is still not exercised.
TestMySQLWireChangeUserAuthSwitchAndRepeatedBorrowis useful, but it usesnet.Pipe,SkipCheckUser=true, and onlyCOM_CHANGE_USER; there is still no real Connector/JMysqlConnectionPoolDataSourcerun and no wire test for modernCOM_RESET_CONNECTION. The DB-loss bug above passes every current test. Please add or attach exact-head Connector/J/motr evidence for 8.4/9.7 reset and 8.0.15 change-user, covering first/repeated borrows, stable physical connection ID, selected DB, rollback, temporary/user/prepared/default-state cleanup, and replacement after reset failure.
Focused current-head tests for rollback fail-close, change-user wire/auth-switch loop, repeated reset, temp-table retry, and packet parsing all pass. I also rechecked generation admission/publication, auth-state rollback, account/session registration, counters, timeout/cancellation, temporary-table cleanup ownership, and parser bounds; no other confirmed blocker remains.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep incremental re-review at exact head c7e7ed4. git range-diff proves all ten PR commits are patch-identical to the directly exercised 0dfca44 stack; this update only rebases over unrelated prepared-numeric and fulltext changes. The selected-database fix correctly separates wire COM_RESET_CONNECTION from Proxy reset semantics, and the prior rollback/temp-table fail-closed paths remain sound. One P1 interoperability/correctness blocker remains.
[P1] The checked-in real client regression fails for Connector/J 8.0.15, one of the three versions explicitly covered by #27644. I built the PR Java fixture and mo-service, then ran TestConnectorJConnectionPoolReset against the real wire path. Connector/J 8.4 and 9.7 pass; 8.0.15 fails on the second logical borrow at ConnectorJPoolResetProbe.java:80 because Connection.getAutoCommit() remains false. A review-only diagnostic on the same connection showed SELECT @@session.autocommit = 1, so reset creates a client/server transaction-state split: JDBC believes manual commit is active while MatrixOne actually autocommits statements. That can silently destroy multi-statement atomicity for the next pool borrower.
The green standard checks do not cover this path. TestConnectorJConnectionPoolReset skips when xtool/jstfu/target/jstfu.jar is absent, and the current workflows do not run make jstfu; the new three-version regression is therefore silently skipped in ordinary UT.
Please make the 8.0.15 COM_CHANGE_USER fallback leave Connector/J and the server in the same clean autocommit state, run all three exact driver versions against a real MatrixOne, and wire this S0 client regression into a job that builds the Java fixture and cannot report success by skipping prerequisites.
Evidence on the patch-identical stack:
- focused frontend reset/change-user tests: pass
make jstfu: pass- real
TestConnectorJConnectionPoolReset: 8.0.15 fail, 8.4 pass, 9.7 pass - diagnostic: JDBC autoCommit=false while server @@session.autocommit=1
- current exact-head
git diff --check: pass
I found no additional confirmed production correctness, auth, lifecycle, liveness, resource, or material performance blocker.
iamlinjunhong
left a comment
There was a problem hiding this comment.
The requested Connector/J 8.0.15 client/server autocommit equality is not a server behavior MatrixOne can provide while remaining MySQL-compatible. I ran the identical MysqlConnectionPoolDataSource probe against an isolated native MySQL instance with the exact 8.0.15 driver: it fails deterministically at the same second-borrow assertion. The driver reports getAutoCommit=false while the server-side session autocommit query returns 1.
The driver bytecode explains the reference result: ConnectionImpl.resetServerState only calls changeUser, and NativeServerSession keeps a separate autoCommit boolean that the change-user authentication path does not update from the terminal protocol status. MatrixOne already completes the command with a clean autocommit status; changing the server to retain manual-commit mode would diverge from MySQL reset semantics and preserve the wrong session default.
The 8.0.15 assertion therefore encodes behavior that stock MySQL itself does not satisfy. The real MatrixOne regression remains necessary for the modern reset path and server-side cleanup; the legacy driver-local state discrepancy needs a client-side workaround or a driver upgrade, not a nonstandard COM_CHANGE_USER response.
What type of PR is this?
Which issue(s) this PR fixes:
issue #27644
What this PR does / why we need it:
fix(frontend): support MySQL session reset commands