kv: don't let pusher win when pushing STAGING txn with equal priority#107882
kv: don't let pusher win when pushing STAGING txn with equal priority#107882craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
miraradeva
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/concurrency/lock_table_waiter.go line 1304 at r1 (raw file):
// to push. A push may determine that the pushee is STAGING or has already // been finalized. pusheeStatus := roachpb.PENDING
Is it worth querying the actual pushee status here? If it's already finalized or staging, it can save us a push request?
pkg/kv/kvserver/txnwait/queue_test.go line 227 at r1 (raw file):
func testCanPushWithPriorityPushTimestamp(t *testing.T) { t.Run("pusheeStatus="+roachpb.PENDING.String(), testCanPushWithPriorityPushTimestampPusheePending) t.Run("pusheeStatus="+roachpb.STAGING.String(), testCanPushWithPriorityPushTimestampPusheeStaging)
These capture a lot of the cases from your spreadsheet. I think only the abort push type is missing, but maybe that's already done elsewhere? I think it would be great to have one massive test for all cases.
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/concurrency/lock_table_waiter.go line 1304 at r1 (raw file):
Previously, miraradeva (Mira Radeva) wrote…
Is it worth querying the actual pushee status here? If it's already finalized or staging, it can save us a push request?
A push request and a query request will cost the same, so I don't think there's much to be gained from querying for the status in order to avoid a push.
pkg/kv/kvserver/txnwait/queue_test.go line 227 at r1 (raw file):
Previously, miraradeva (Mira Radeva) wrote…
These capture a lot of the cases from your spreadsheet. I think only the abort push type is missing, but maybe that's already done elsewhere? I think it would be great to have one massive test for all cases.
The abort push type is captured in testCanPushWithPriorityPushAbort, which is another subtest of TestCanPushWithPriority.
3d49079 to
e663388
Compare
Informs cockroachdb#105330. This commit adds a new unit test named `TestTxnRecoveryFromStagingWithoutHighPriority`, which reproduces the error described in cockroachdb#105330. Release note: None
e663388 to
5107cff
Compare
|
@miraradeva I've improved the testing in this PR by adding an integration test that reproduces #105330 and then demonstrating that the fix here corrects the behavior in the test. This should now be good for a look. |
miraradeva
left a comment
There was a problem hiding this comment.
Reviewed 1 of 8 files at r2, 7 of 8 files at r3.
Reviewable status:
-- commits line 30 at r3:
too -> to
pkg/kv/kvserver/batcheval/cmd_push_txn.go line 296 at r3 (raw file):
// transaction recovery procedure always finalizes target transactions, even // if initiated by a PUSH_TIMESTAMP. if pusheeStaging /* => pusheeStagingFailed */ && pushType == kvpb.PUSH_TIMESTAMP {
How does pusheeStaging => pusheeStagingFailed? Isn't it the opposite: pusheeStagingFailed => pusheeStaging? So maybe here we want if pusheeStagingFailed && ...? It matches the comment and the previous code. Also, it would be nice if this logic can be closer to the other STAGING logic 10-ish lines above.
Is this logic tested anywhere?
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, miraradeva (Mira Radeva) wrote…
too -> to
Done.
pkg/kv/kvserver/batcheval/cmd_push_txn.go line 296 at r3 (raw file):
How does
pusheeStaging => pusheeStagingFailed?
pusheeStaging implies pusheeStagingFailed because pusheeStaging && !pusheeStagingFailed would have hit one of the two previous branches. Either pusherWins, is which case we would have returned an IndeterminateCommitError, or !pusherWins, in which case we would have returned a TransactionPushError.
I wrote it like this because I want it to be clear to readers that if a push does succeed on a STAGING txn, it will have a pushType of PUSH_ABORT.
Is this logic tested anywhere?
It is tested by TestTxnRecordLifecycleTransitions and TestTxnRecoveryFromStagingWithHighPriority.
miraradeva
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/batcheval/cmd_push_txn.go line 296 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
How does
pusheeStaging => pusheeStagingFailed?
pusheeStagingimpliespusheeStagingFailedbecausepusheeStaging && !pusheeStagingFailedwould have hit one of the two previous branches. EitherpusherWins, is which case we would have returned anIndeterminateCommitError, or!pusherWins, in which case we would have returned aTransactionPushError.I wrote it like this because I want it to be clear to readers that if a push does succeed on a STAGING txn, it will have a pushType of
PUSH_ABORT.Is this logic tested anywhere?
It is tested by
TestTxnRecordLifecycleTransitionsandTestTxnRecoveryFromStagingWithHighPriority.
Ok, I see. It's not super easy to understand with all the cases , in particular if someone wants to add an extra condition to one of these cases (like when I played around with wait policies). This is also not great but a bit more explicit:
Code snippet:
if pusheeStaging && !pusheeStagingFailed && (pusherWins || recoverOnFailedPush) {
// kvpb.NewIndeterminateCommitError
}
if pusheeStagingFailed && pusherWins && pushType == kvpb.PUSH_TIMESTAMP {
pushType = kvpb.PUSH_ABORT
}
if !pusherWins {
// kvpb.NewTransactionPushError
}
miraradeva
left a comment
There was a problem hiding this comment.
Just one potential alternative but otherwise
Reviewed 1 of 8 files at r3, all commit messages.
Reviewable status:
5107cff to
412bda8
Compare
Fixes cockroachdb#105330. Fixes cockroachdb#101721. This commit updates the transaction push logic to stop pushes from completing successfully when the pushee is STAGING and the pusher has equal priority. Prior to this change, the pusher would win in these cases when using a PUSH_TIMESTAMP if at least one of the two transactions involved used a weak isolation level. This had two undesirable effects: - if the pushee was still possibly committable and requiring recovery (`!(knownHigherTimestamp || knownHigherEpoch)` in the code) then the pusher would immediately begin parallel commit recovery, attempting to disrupt the commit and abort the pushee. This is undesirable because the pushee may still be in progress and in cases of equal priority, we'd like to wait for the parallel commit to complete before kicking off recovery, deferring recovery to only the cases where the pushee/committers's heartbeat has expired. - if the pushee was known to be uncommittable (`knownHigherTimestamp || knownHigherEpoch` in the code), then txn recovery was not kicked off but the pusher still could not perform the PUSH_TIMESTAMP (see e40c1b4), so it would return a `TransactionPushError`. This confused logic in `handleTransactionPushError`, allowing the error to escape to the client. This commit resolves both issues by considering the pushee's transaction status in `txnwait.ShouldPushImmediately`. Release note: None
412bda8 to
90516db
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r=miraradeva
Reviewable status:
pkg/kv/kvserver/batcheval/cmd_push_txn.go line 296 at r3 (raw file):
Previously, miraradeva (Mira Radeva) wrote…
Ok, I see. It's not super easy to understand with all the cases , in particular if someone wants to add an extra condition to one of these cases (like when I played around with wait policies). This is also not great but a bit more explicit:
Good idea! I adopted the structure and made one small change to your suggestion using Erik's new must assertion library.
|
This PR was included in a batch that was canceled, it will be automatically retried |
|
Build succeeded: |
Fixes #105330.
Fixes #101721.
This commit updates the transaction push logic to stop pushes from completing successfully when the pushee is STAGING and the pusher has equal priority. Prior to this change, the pusher would win in these cases when using a PUSH_TIMESTAMP if at least one of the two transactions involved used a weak isolation level.
This had two undesirable effects:
!(knownHigherTimestamp || knownHigherEpoch)in the code) then the pusher would immediately begin parallel commit recovery, attempting to disrupt the commit and abort the pushee. This is undesirable because the pushee may still be in progress and in cases of equal priority, we'd like to wait for the parallel commit to complete before kicking off recovery, deferring recovery to only the cases where the pushee/committers's heartbeat has expired.knownHigherTimestamp || knownHigherEpochin the code), then txn recovery was not kicked off but the pusher still could not perform the PUSH_TIMESTAMP (see e40c1b4), so it would return aTransactionPushError. This confused logic inhandleTransactionPushError, allowing the error to escape to the client.This commit resolves both issues by considering the pushee's transaction status in
txnwait.ShouldPushImmediately.Release note: None