kv: don't error on pushed intent during QueryIntent, increase write timestamp

[nvanbenschoten]

Fixes #95225.

This commit updates QueryIntent's behavior when ErrorIfMissing to only return an error if no matching intent is found. Notably, the request no longer returns an error if a matching intent is found, but has been pushed.

Previously, this case would result in a RETRY_SERIALIZABLE ("intent pushed") error, causing the querying transaction to immediately refresh and retry. At the time, we thought that this behavior was "an optimization" because it allowed a transaction to notice that it had been pushed and may abort earlier, instead of waiting until commit time. In practice, this does not seem to be a meaningful improvement. Meanwhile, it makes any solution to #95227 more difficult because it allows a stream of high-priority reading transaction to starve a low-priority writing transaction before that writing transaction even reaches its commit phase. We'd like to solve these starvation cases through some form of commit-time coordination (either blocking pushers temporarily or refreshing into the future). This change helps unlock those solutions.

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

One minor comment about possibly renaming these fields, but otherwise looks good.

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @nvanbenschoten)

---

pkg/kv/kvpb/api.proto line 1250 at r1 (raw file):

// A QueryIntentRequest is arguments to the QueryIntent() method. It visits
// the specified key and checks whether an intent is present for the given
// transaction. If so, it checks whether the intent has a timestamp at or

The "at or below" phrasing isn't very clear how we treat intents that have been pushed when read in conjunction with the next line. Maybe consider something like:

If the intent is found, it checks whether the intent has a timestamp at or below the given transaction's timestamp to determine full vs. partial matches. If, on the other hand, the intent is found to be missing ... 

---

pkg/kv/kvpb/api.proto line 1274 at r1 (raw file):

  // The intent is considered to be "pushed" and a "partial match" if it
  // satisfies all conditions above except the timestamp condition. In these
  // cases, the intent could not be committed by the provided transaction at its

nit: s/could not/cannot/?

---

pkg/kv/kvpb/api.proto line 1302 at r1 (raw file):

  // with a timestamp that is equal to or less than that in the provided
  // transaction - a "full match" as defined above.
  bool found_intent_matching_txn_and_timestamp = 2;

It's subtle that this field is called MatchingTxnAndTimestamp but in fact it means the timestamp can be less than or equal to. How do you feel about calling these two fields either:

• FoundFullMatch and FoundPartialMatch
• or FoundIntent and FoundPushedIntent

instead?

---

pkg/kv/kvpb/api.proto line 1305 at r1 (raw file):

  // Whether an intent matching the expected transaction was found at the key,
  // regardless of the intent's timestamp — a "partial match" as defined above.
  // found_intent_matching_txn implies found_intent_matching_txn_and_timestamp.

Should this be the other way around? found_intent_matching_txn_and_timestamp implies found_intent_matching_txn?

Edit: It seems like this is trying to capture the 23.1 <-> 23.2 mixed version situation. Could we reword this comment to be explicit?

---

pkg/kv/kvserver/replica_test.go line 6120 at r1 (raw file):

			// Query the intent with a smaller timestamp. Should not see an
			// intent unless we're querying our own intent, in which case

nit: this comment sounds stale now, should we reword?

[nvanbenschoten]

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

---

pkg/kv/kvpb/api.proto line 1250 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

The "at or below" phrasing isn't very clear how we treat intents that have been pushed when read in conjunction with the next line. Maybe consider something like:

If the intent is found, it checks whether the intent has a timestamp at or below the given transaction's timestamp to determine full vs. partial matches. If, on the other hand, the intent is found to be missing ... 

Done.

---

pkg/kv/kvpb/api.proto line 1274 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: s/could not/cannot/?

Done.

---

pkg/kv/kvpb/api.proto line 1302 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

It's subtle that this field is called MatchingTxnAndTimestamp but in fact it means the timestamp can be less than or equal to. How do you feel about calling these two fields either:

• FoundFullMatch and FoundPartialMatch
• or FoundIntent and FoundPushedIntent

instead?

I went back and forth on whether to promote the "full match" and "partial match" semantics to be exposed on the API. There are benefits to that, but I found that it made the client code that used these fields (e.g. txnPipeliner.updateLockTrackingInner) difficult to read. Those terms aren't intuitive, so readers of the code would need to jump to the definitions here, even if they weren't specifically concerned with the subtleties of this API.

I did also consider something like FoundIntent and FoundPushedIntent, but I wanted one of the two fields to imply the other so that clients would not need logic like if resp.FoundIntent || resp.FoundPushedIntent { ... } to handle cases where they don't care whether the intent was pushed.

An alternative that achieves this (and preserves backwards compat) is FoundUnpushedIntent and FoundIntent. What do you think?

---

pkg/kv/kvpb/api.proto line 1305 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Should this be the other way around? found_intent_matching_txn_and_timestamp implies found_intent_matching_txn?

Edit: It seems like this is trying to capture the 23.1 <-> 23.2 mixed version situation. Could we reword this comment to be explicit?

You are right, this was backwards. Fixed.

---

pkg/kv/kvserver/replica_test.go line 6120 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: this comment sounds stale now, should we reword?

Done.

[arulajmani]

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

---

pkg/kv/kvpb/api.proto line 1302 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I went back and forth on whether to promote the "full match" and "partial match" semantics to be exposed on the API. There are benefits to that, but I found that it made the client code that used these fields (e.g. txnPipeliner.updateLockTrackingInner) difficult to read. Those terms aren't intuitive, so readers of the code would need to jump to the definitions here, even if they weren't specifically concerned with the subtleties of this API.

I did also consider something like FoundIntent and FoundPushedIntent, but I wanted one of the two fields to imply the other so that clients would not need logic like if resp.FoundIntent || resp.FoundPushedIntent { ... } to handle cases where they don't care whether the intent was pushed.

An alternative that achieves this (and preserves backwards compat) is FoundUnpushedIntent and FoundIntent. What do you think?

You're right, structuring this as FoundUnpushedIntent and FoundIntent is going to be much more readable on the client. Let's go with that?

---

pkg/kv/kvserver/replica_test.go line 6120 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Done.

nit: once we switch to FoundIntent and FoundUnpushedIntent, this might need another update.

[nvanbenschoten]

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

---

pkg/kv/kvpb/api.proto line 1302 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

You're right, structuring this as FoundUnpushedIntent and FoundIntent is going to be much more readable on the client. Let's go with that?

Done.

---

pkg/kv/kvserver/replica_test.go line 6120 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: once we switch to FoundIntent and FoundUnpushedIntent, this might need another update.

Done

[nvanbenschoten]

bors r=arulajmani

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[nvanbenschoten]

TestLogic_udf flake.

bors r=arulajmani

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)