kv: don't let pusher win when pushing STAGING txn with equal priority

[nvanbenschoten]

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:

• 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

[cockroach-teamcity]

This change is 

[miraradeva]

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.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @miraradeva)

---

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.

[nvanbenschoten]

@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]

Reviewed 1 of 8 files at r2, 7 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

-- 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?

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @miraradeva)

---

-- commits line 30 at r3:

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]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

pkg/kv/kvserver/batcheval/cmd_push_txn.go line 296 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

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.

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]

Just one potential alternative but otherwise

Reviewed 1 of 8 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

[nvanbenschoten]

TFTR!

bors r=miraradeva

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani and @miraradeva)

---

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.

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)