adapter: add guarantees for AdapterClient::end_transaction #28549
Conversation
This file contains 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
Both the HTTP and pgwire protocols automatically commit implicit transactions by calling AdapterClient::end_transaction which sends a Command::Commit to the Coordinator. That command produced a Plan::CommitTransaction (equivalent to the user typing `COMMIT`). Since this was a normal plan, the same as if it came from a SQL statement, the RBAC checks run. One of the checks verifies that the user's role still exists (regardless of the statement being executed). It is thus possible for the Plan::CommitTransaction to fail and not sequence its plan (in this case `sequence_end_transaction`). However, both the AdapterClient (and HTTP and pgwire protocols) had assumed that Command::Commit would always clean up the session's transaction state. But since the cleanup action happens in `sequence_end_transaction` which isn't guaranteed to execute (if, say, the user's role was dropped), that assumption is not guaranteed. Change Command::Commit to guarantee that the Coordinator cleans up the txn state, and change AdapterClient::end_transaction to guarantee it cleans up the session's txn state. This leaves all usages of end_transaction as is by changing the implementation to do what the callers assumed it had been doing. No explicit test was added because the parallel DDL tests have reliably been finding this bug, so adding the soft panic should produce a panic if this bug is still present when running that nightly. Alternatives considered: - Add a Command::ClearTransaction command that AdapterClient::end_transaction calls which would cleanup the state. This seemed unneeded and another message sent to the Coordinator that could be avoided by adding the desired behavior to Commit and having the AdapterClient take care of the session's state. - Have callers of AdapterClient::end_transaction terminate the connection. This sounded nice but ended up being tricky because the higher level callers often support generic errors that get serialized and sent off to the user, leaving the connection intact. We'd have to plumb some new logic through that would indicate a termination was needed which didn't seem worth it. - Teach Command::Commit to produce a Plan that would skip the rbac checks. Skipping RBAC checks seemed dangerous. Fixes #28400
|
Nice find, @maddyblue! Thanks very much for tracking this one down. |
5 tasks
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.
Both the HTTP and pgwire protocols automatically commit implicit transactions by calling AdapterClient::end_transaction which sends a Command::Commit to the Coordinator. That command produced a Plan::CommitTransaction (equivalent to the user typing
COMMIT). Since this was a normal plan, the same as if it came from a SQL statement, the RBAC checks run. One of the checks verifies that the user's role still exists (regardless of the statement being executed). It is thus possible for the Plan::CommitTransaction to fail and not sequence its plan (in this casesequence_end_transaction). However, both the AdapterClient (and HTTP and pgwire protocols) had assumed that Command::Commit would always clean up the session's transaction state. But since the cleanup action happens insequence_end_transactionwhich isn't guaranteed to execute (if, say, the user's role was dropped), that assumption is not guaranteed.Change Command::Commit to guarantee that the Coordinator cleans up the txn state, and change AdapterClient::end_transaction to guarantee it cleans up the session's txn state. This leaves all usages of end_transaction as is by changing the implementation to do what the callers assumed it had been doing.
No explicit test was added because the parallel DDL tests have reliably been finding this bug, so adding the soft panic should produce a panic if this bug is still present when running that nightly.
Alternatives considered:
Fixes MaterializeInc/database-issues#8316
Motivation
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.