kv: enable replay protection for ambiguous writes on commits

[AlexTalks]

While previously, RPC failures were assumed to be retriable, as write operations (with the notable exception of EndTxn) were assumed to be idempotent, it has been seen in #67765 and documented in #103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where withCommit==true), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's WriteTimestamp being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded.

This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly.

The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the RecoverTxn operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a RETRY_SERIALIZABLE error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to transaction unexpectedly committed.

The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an AmbiguousResultError. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error.

Depends on #107680, #107323, #108154, #108001

Fixes: #103817

Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retriable failures and transaction unexpectedly committed assertions by detecting when writes cannot be retried idempotently, instead returning an AmbiguousResultError.

[cockroach-teamcity]

This change is 

[AlexTalks]

NB: It's perhaps worth noting that this fix may prove very difficult to backport, given that it requires modification to the signature of MVCCPut and variants across 450+ call sites throughout the code base.

[srosenberg]

NB: It's perhaps worth noting that this fix may prove very difficult to backport, given that it requires modification to the signature of MVCCPut and variants across 450+ call sites throughout the code base.

Isn't just the addition of EnableReplayProtection to the MVCCWriteOptions struct? I don't see why the signature of MVCCPut has to change.

[AlexTalks]

@srosenberg it's because we had to refactor the args to MVCCPut in #107680 in order to do this 😆

[srosenberg]

@srosenberg it's because we had to refactor the args to MVCCPut in #107680 in order to do this 😆

Oops, I missed that one. Most are one-line, mechanical changes. What's your confidence level on introducing a major regression if we were to backport? Presumably, type checker would have caught some of the typos, but definitely not all.

[AlexTalks]

@srosenberg I am fairly confident that we can do this if we want; I think the question is more about time investment and prioritization.

[nvanbenschoten]

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

---

pkg/kv/kvclient/kvcoord/dist_sender.go line 2087 at r3 (raw file):

func noMoreReplicasErr(ambiguousErr, lastAttemptErr error) error {
	if ambiguousErr != nil {
		return kvpb.NewAmbiguousResultErrorf("error=%s [exhausted] (last error: %v)",

nit here and below: we mix %s and %v in here. Does that format correctly?

---

pkg/kv/kvclient/kvcoord/dist_sender.go line 2268 at r3 (raw file):

		ba.AmbiguousReplayProtection = ambiguousError != nil
		if ba.AmbiguousReplayProtection && ba.CanForwardReadTimestamp {
			ba.CanForwardReadTimestamp = false

Let's give this a comment, explaining why we had to make this change.

Also, do we have a test that would fail without it?

---

pkg/storage/mvcc.go line 1659 at r3 (raw file):

// Ensure all intents are found and that the values are always accurate for
// transactional idempotency.
func replayTransactionalWrite(

We should add storage-level tests for this new flag. Ideally in TestMVCCHistories.

[AlexTalks]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @arulajmani, @jbowens, and @knz)

---

pkg/kv/kvclient/kvcoord/dist_sender.go line 2087 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit here and below: we mix %s and %v in here. Does that format correctly?

Good point - I think it does, but it doesn't look very clean to do this in a single statement and it would be better to use one or the other. I remember @knz mentioning we wanted to try to use %v for errors everywhere, I'm not sure if that's just style or something else however.

---

pkg/kv/kvclient/kvcoord/dist_sender.go line 2268 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Let's give this a comment, explaining why we had to make this change.

Also, do we have a test that would fail without it?

Will do - I'll add more comment here.

We do have a test already introduced in #108001, that's the recovery before retry with serverside refresh test case.

---

pkg/storage/mvcc.go line 1659 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We should add storage-level tests for this new flag. Ideally in TestMVCCHistories.

Will do - that seems like a good idea.

[RaduBerinde]

pkg/kv/kvclient/kvcoord/dist_sender.go line 2087 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

Good point - I think it does, but it doesn't look very clean to do this in a single statement and it would be better to use one or the other. I remember @knz mentioning we wanted to try to use %v for errors everywhere, I'm not sure if that's just style or something else however.

%v is like %s except it tolerates nil. It's fine to use %s when you know the error is not nil, but then again there's no downside to always using %v

[AlexTalks]

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

---

pkg/kv/kvserver/batcheval/cmd_conditional_put.go line 65 at r5 (raw file):

		LocalTimestamp:         cArgs.Now,
		Stats:                  cArgs.Stats,
		EnableReplayProtection: h.AmbiguousReplayProtection,

Unfortunately had missed this before but caught now!

[daniel-crlabs]

Affects this customer: https://cockroachdb.zendesk.com/agent/tickets/19344