kvclient: disallow savepoint rollback after most errors

[andreimatei]

This patch changes the handling of ConditionFailedErrors (after failed
CPuts). It used to be the case that a txn could not continue sending
requests after such an error (like after any other error). It is now
allowed for a transaction to continue sending requests after this
particular error because, in particular, we want to allow rollback to
savepoints after it (see below). More broadly, there's no particular
reason to not allow the client to continue, as long as it knows what
it's doing. In fact, it's long been debatable whether
ConditionFailedErrors should be errors at all, rather than another type
of result.
One caveat is that clients chosing to swallow this error need to be
cautious of mixed success conditions, if that's applicable to the batch
they have sent.
Note that at the SQL level, a CPut error will still cause connection to
be in an error state until a ROLLBACK.

This patch also disallows rolling back to savepoints after a request
returned any other error but ConditionFailedError. It used to be the
case that rolling back to a savepoint was allowed after most errors, on
the argument that our rollback mechanism (the ignored seq nums) is
powerful enough to undo any effects of requests evaluated since the
savepoint. Unfortunately, it wasn't safe. The way in which we overwrite
intents and update their timestamp in the process, coupled with the
transaction coordinator not being aware that an intent's timestamp was
updated after different ambiguous errors, can lead to partial commits.
See #47587. We could more narrowly disallow rollbacks after ambiguous
errors, but the ambiguous cases are not trivial to isolate. I also want
to backport something, so this seems safer.

Fixes #47587

Release note (sql change): ROLLBACK TO SAVEPOINT is no longer permitted
after misc internal errors.

[cockroach-teamcity]

This change is 

[tbg]

Many errors are common and benign and yet they will be unsafe to special-case here.
The real reason we get to special-case ConditionFailedError is that a ConditionFailedError is already treated as a successful read: it populates the timestamp cache just the same. Also, presumably when we combine errors from multiple concurrent DistSender batches a ConditionFailedError can only be passed up if there weren't any other errors or timestamp pushes. I sure hope that's true and I didn't forget anything! This commit sure doesn't explain to me why it's safe.
Honestly since I have little context here I'm a bit uneasy about all of this. As long as this is an error, I will need verbose justification of why everything will work out correctly.

As someone who came into this review only with broad context, I was confused by what's happening. I thought the main thing would be fixing the bug that arises from allowing savepoints to "swallow" arbitrary errors, which we've found out can give you consistency anomalies. The fix I expected would've been to allow rollbacks only if the error was a unique constraint violation (and even then, some of my earlier questions apply, since in our txn model this boils down to continuing through a condition failed error in some cases).
Instead, the PR mostly harps about how it's OK to swallow CPut (but without real justification) in general and makes that change. If we're going to backport something, then certainly not that, right? Also, Postgres does not allow that at all:

postgres=# create table kv(k int primary key, v int);
CREATE TABLE
postgres=# insert into kv values(1,1);
INSERT 0 1
postgres=# begin;
BEGIN
postgres=# insert into kv values(1,2);
ERROR:  duplicate key value violates unique constraint "kv_pkey"
DETAIL:  Key (k)=(1) already exists.
postgres=# insert into kv values(2,2);
ERROR:  current transaction is aborted, commands ignored until end of transaction block

and the same is true if the constraint violation is incurred in savepoint (have to roll back the savepoint).

Since we're just trying to fix the bug (right), can we stick to that? I'd rather not willy-nilly introduce behavior that isn't possible in Postgres, especially since I don't see a thorough argument for why this isn't just giving is a few new bugs. (Maybe this is all sound and the experts know, but then the verbiage here needs more work still).

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, @knz, @nvanbenschoten, and @tbg)

---

pkg/kv/kvclient/kvcoord/testdata/savepoints, line 361 at r3 (raw file):

cput k v bogus_expected
----
(*roachpb.ConditionFailedError) unexpected value

Are there ConditionFailedErrors that arise from use of the session txn outside of the user's statements (for example to grab leases, etc)? Is there any danger to confuse these?

[knz]

I agree with Tobias that the last commit message is confusing.

Knowing the background, I'd like to assuage the other reviewers that the behavior is OK, but the commit message does not say this. The problem is that the explanation fails to sufficiently distinguish the KV txn (kv.Txn) and the SQL txn (that held by sql.connExecutor).

The way I'm reading the code, the tldr for this commit is this:

• in kv.Txn, all errors but ConditionFailed change the state of the txn to txnError
• all errors including ConditionFailed are reported from KV to SQL and cause a SQL txn error in any case
• in SQL, any error condition can be papered over using a savepoint rollback as long as the KV txn is not in state txnError

(Andrei please confirm)

Because of this:

1. any error but ConditionFailed in kv is a hard error and prevents anything further in SQL, including rollbacks
2. ConditionFailed causes a SQL error, but one that can be recoved by rollback - this is compatible with pg

(So this does not do anything worse semantic-wise than what we have today in the 20.1 code)

I'd also like to rephrase / improve that release note, perhaps as follows:

Release note (sql change): rolling back a nested transaction (via ROLLBACK TO SAVEPOINT)
is now only possible after either:
- errors that don't stem from checks during persistent data stores and retrievals.
  This includes computational errors (division by error, etc), CHECK violations, privilege errors, etc.
- unique index / primary key violations caused by duplicate rows.
Other internal errors that emanate from CockroachDB's transaction and storage (assertion failures,
consistency check errors, ambiguous results, etc.) cause the SQL transaction to fail completely
in a way that cannot be erased by rolling back a nested transaction.

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 4 of 4 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @nvanbenschoten)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 786 at r3 (raw file):

	// rollback.
	if errTxn := pErr.GetTxn(); errTxn != nil {
		// Every error exceptConditionFailedError causes the transaction to refuse

nit: missing space

[tbg]

Thanks Raphael! That sounds more reasonable. I'm still wondering if we've thought about everything regarding rolling back through ConditionFailedError (I know we did it before this PR as well). Did we write that out somewhere? Because we need to. It looks like we're relying on invariants that - as far as I can tell - aren't well-documented. For example, that DistSender will always return an ambiguous result error (or otherwise distinctly non-ConditionFailedIsh error) if it wasn't able to get responses for all concurrent RPCs (maybe excluding read-only ops). And that if it does return a more concrete error, it has a transaction that is guaranteed to reflect the true timestamp of the transaction at that point (mod pushes to the txn record that were not yet discovered by the client). And of course that we treat ConditionFailedError like a read (timestamp cache).

[knz]

Yeah it sounds reasonable that if ConditionFailed becomes a special KV non-error, there should be more testing that asserts that the txn is completely healthy after it occurs.

[andreimatei]

Added a note in the commit about the SQL connection still being an error state after a CPut error. Sorry it wasn't clear before.

I've done some changes to the DistSender's merging of errors, and some tests. See if you find them convincing.

I've also prepended a commit ranting more about DistSender's handling of ambiguity - a topic that might well cause me to try my luck at a different career any minute now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @knz, @nvanbenschoten, and @tbg)

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 786 at r3 (raw file):

Previously, knz (kena) wrote…

nit: missing space

done

---

pkg/kv/kvclient/kvcoord/testdata/savepoints, line 361 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Are there ConditionFailedErrors that arise from use of the session txn outside of the user's statements (for example to grab leases, etc)? Is there any danger to confuse these?

Don't think so; the KV txn is not used for grabbing leases. Even if it were, as far as the transaction is concerned, it's still be OK to rollback to a savepoint.
And from SQL's perspective too, as long as there's no problems at the kv layer, rollbacks are fine after various errors. Modulo schema change modifications, which are handled at the SQL level.

[andreimatei]

Oh Rafa, I didn't take your suggestion for the release note. I think the fewer words, the better. Simply talking about "misc internal errors" will hopefully cause most people's eyes to gloss over, which is what I really want. Anything more I think is too confusing.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @knz, @nvanbenschoten, and @tbg)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 76e8c9bd.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

@rmloveland can you study the back-and-forth between Andrei and I above on the release note? Can you chime in and share your advice?

[tbg]

, this looks good given the circumstances. I wish we had a better KV API and I'm sure so do you. Thanks for the additional comments and test cases.

Reviewed 5 of 5 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 6 of 6 files at r7.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andreimatei, @bdarnell, and @nvanbenschoten)

---

pkg/kv/kvclient/kvcoord/dist_sender.go, line 1661 at r6 (raw file):

forget it for

---

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 2725 at r7 (raw file):

	// We're also going to check that the highest bumped WriteTimestamp makes it
	// to the merged error.
	writeTimestamp1 := txn.WriteTimestamp.Add(100, 0)

err{1,2}WriteTimestamp so that it's clear that these are the timestamps that will be put into err1/2 respectively

---

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 2859 at r7 (raw file):

				expWriteTimestamp := txn.WriteTimestamp
				if tc.err1 != nil {

How does this work with reverse? In that case you've switched the errors but not the write timestamps.
Ah - it assigns the timestamp after switching. Ok nevermind.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 783 at r7 (raw file):

	}

	// This is the non-retriable error case. The client is expected to send a

Rotted a bit

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 786 at r7 (raw file):

further requests

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 793 at r7 (raw file):

			tc.mu.storedErr = roachpb.NewError(&roachpb.TxnAlreadyEncounteredErrorError{
				PrevError: pErr.String(),
			})

So if we're not taking that branch, at the KV level, we allow using the transaction in any way, right? So we could do

txn.CPut(...) // condition failed
txn.Get()
txn.Put()
txn.CPut() // condition failed
...
txn.Commit

but the SQL layer will never use it for anything without a prior ROLLBACK before?

That seems fine but we should document the new KV behavior somewhere. Nobody should have to read the impl ErrPriority to learn what our KV API supports. Can you add something to ConditionalPutRequest and the various .CPut methods? Also file a placeholder issue for us to remember to add this kind of testing to kvnemeses.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go, line 117 at r7 (raw file):

	// https://github.com/cockroachdb/cockroach/issues/47587.
	//
	// TODO(andrei): Black-list fewer errors.

nit: White-list more errors.

---

pkg/roachpb/errors.go, line 69 at r7 (raw file):

	// ErrorScoreUnambiguousError is used for errors which are known to return a
	// transaction reflecting the highest timestamp of any intent that was
	// written. We allow savepoint rollbacks after such errors, and with savepoint

I might still have misunderstood this PR, but I thought at the KV level you allow just continuing to use the txn however you want. It's just the SQL level that does the rollback before it lets the user use the txn again?

[rmloveland]

@rmloveland can you study the back-and-forth between Andrei and I above on the release note? Can you chime in and share your advice?

I think our docs (well, in-progress PR) already say essentially this (on the updates to the SAVEPOINT page):

• ROLLBACK TO SAVEPOINT can recover from logical errors in your SQL code (index/PK errors, etc.)
• ROLLBACK TO SAVEPOINT can not recover from a "database error", which would seem to cover "internal errors that emanate from CockroachDB's transaction and storage"

This seems to match the spirit of both Raphael's longer release note and Andrei's shorter one.

From my user-level POV, I think the shorter note is fine.

[andreimatei]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell, @nvanbenschoten, and @tbg)

---

pkg/kv/kvclient/kvcoord/dist_sender.go, line 1661 at r6 (raw file):

Previously, tbg (Tobias Grieger) wrote…

forget it for

done

---

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 2725 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

err{1,2}WriteTimestamp so that it's clear that these are the timestamps that will be put into err1/2 respectively

done

---

pkg/kv/kvclient/kvcoord/dist_sender_test.go, line 2859 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

How does this work with reverse? In that case you've switched the errors but not the write timestamps.
Ah - it assigns the timestamp after switching. Ok nevermind.

ack

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 783 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Rotted a bit

done

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 786 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

further requests

done

---

pkg/kv/kvclient/kvcoord/txn_coord_sender.go, line 793 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

So if we're not taking that branch, at the KV level, we allow using the transaction in any way, right? So we could do

txn.CPut(...) // condition failed
txn.Get()
txn.Put()
txn.CPut() // condition failed
...
txn.Commit

but the SQL layer will never use it for anything without a prior ROLLBACK before?

That seems fine but we should document the new KV behavior somewhere. Nobody should have to read the impl ErrPriority to learn what our KV API supports. Can you add something to ConditionalPutRequest and the various .CPut methods? Also file a placeholder issue for us to remember to add this kind of testing to kvnemeses.

You're correct.
I've added comments to ConditionalPutRequest and txn.CPut. I've also added a test for this behavior.

Btw, the reason why I'm allowing the client to continue after a ConditionFailedError is not because I particularly care about allowing it, but simply because it was the easiest way to let txn.RollbackToSavepoint() work. Beyond that, I do think allowing the transaction to continue makes sense, but I wouldn't have made a narrow exception for ConditionFailedError like this patch is doing. And nobody uses this capability. So that's why I was not enthusiastic about codifying it in a test and talking about it to much, but seems that being coy about it is causing more confusion, so I've shifted.

---

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go, line 117 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: White-list more errors.

done

---

pkg/roachpb/errors.go, line 69 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I might still have misunderstood this PR, but I thought at the KV level you allow just continuing to use the txn however you want. It's just the SQL level that does the rollback before it lets the user use the txn again?

I was just being coy by talking strictly about savepoints. I've expanded the comment.

[andreimatei]

TFTRs

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @bdarnell, @nvanbenschoten, and @tbg)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 65e8045b.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[craig]

Build succeeded

• GitHub CI (Cockroach)