sql: rationalize some output events from the connection state machine #45515
Conversation
|
cc @knz |
|
This change is |
andreimatei
changed the title
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, 2 of 2 files at r2, 5 of 5 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)
pkg/sql/conn_fsm.go, line 506 at r3 (raw file):
}) // cleanupAndFinish rolls back the KV txn and finishes the SQL txn.
Four suggestions to make this easier to read:
- could you move this down right below
finishTxnbecause these are the two code paths that can produce thistxnRollbackevent? - could you rename this to
cleanupAndFinishOnError? - could you make this a method on
*txnStateto parallelnoTxnToOpenandfinishTxn? - could you pull out the actions that do the same thing except they don't
ts.finishSQLTxn()into a similar helper method?
pkg/sql/txn_state.go, line 381 at r1 (raw file):
// txnRestart means that the transaction is expecting a retry. The iteration // of the txn just finished will not commit. // This event is produced both when entering the RetryWait state and sometimes
We seem to like these comments around here, so consider updating it instead of deleting it.
pkg/sql/txn_state.go, line 373 at r3 (raw file):
// when leaving it. txnCommit // txnRollback means that the transaction has rolled back.
Could you include a bit more here? How far has the transaction rolled back? To a savepoint? All the way? Either?
Do you want to include something about when the event is produced?
The connection state machine returns events to the connExecutor distilling what happened to the SQL txn (these events are different from the input events of the state machine). One of these events is txnRestart. The connExecutor reacts to it by clearing some state that it was maintaining for the transaction (e.g. schema change stuff). Before this patch, txnRestart used to be returned both when entering and when exiting the restartWait state. This was redundant and confusing. More over, it's in my way for some savepoints work. This patch makes the event only be generated when the transaction is actually restarted (i.e. when exiting the restartWait state). Release note: None
This patch makes the connection state machine return a txnRestart when transitioning from the Aborted to the Open transition through a ROLLBACK TO SAVEPOINT cockroach_restart. This seems like the right thing to do because we want the state reset that comes with that event. I think we were getting away without it because we were generating the txnAborted event, with similar effects, when we were entering the Aborted state. But I'm preparing to get rid of that. Release note: None
The connection state machine returns an event to the connExecutor - txnAborted - telling it to clean up some state. This event was confusingly returned both when entering the Aborted state and sometimes when leaving it. This patch replaces txnAborted with a txnRollback event which is returned strictly on rollback. Entering the Aborted state no longer triggers this event; it triggers no event. This is in preparation for broader support for savepoints, which want to enter the Aborted state without much fuss. Release note: None
andreimatei
force-pushed
the
savepoint.rationalize-txnRestart
branch
from
483c170 to
d75f7c9
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_fsm.go, line 506 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Four suggestions to make this easier to read:
- could you move this down right below
finishTxnbecause these are the two code paths that can produce thistxnRollbackevent?- could you rename this to
cleanupAndFinishOnError?- could you make this a method on
*txnStateto parallelnoTxnToOpenandfinishTxn?- could you pull out the actions that do the same thing except they don't
ts.finishSQLTxn()into a similar helper method?
- done
- done
- I went the other way and made
noTxnToOpena free-standing function. And see 4) - done - also as a free-standing function that can be referenced directly in the state machine transitions, without having to write an inline function.
pkg/sql/txn_state.go, line 381 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
We seem to like these comments around here, so consider updating it instead of deleting it.
done
pkg/sql/txn_state.go, line 373 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Could you include a bit more here? How far has the transaction rolled back? To a savepoint? All the way? Either?
Do you want to include something about when the event is produced?
done
There was a problem hiding this comment.
Reviewed 5 of 5 files at r4, 2 of 2 files at r5, 5 of 5 files at r6.
Reviewable status:
Build succeeded |
See individual commits.
Release note: None