This change is 

this seems pretty bad, nice catch! What led you to find this? Was an anomaly visible from the SQL layer?

Also, do you mind expanding the commit message to point out that this fixes a serializability violation with CPuts? Maybe also expand on what the implications of that violation were.

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/storage/replica.go, line 2055 at r1 (raw file):

			// Skip update if there's an error and it's not for this index
			// or the request doesn't update the timestamp cache on errors.
			if pErr != nil {

I'd restructure this so that the pErr != nil check is outside of the UpdatesTimestampCache so that commands can have the flag updatesTSCacheOnError without also needing to have the flag updatesTSCache.

Comments from Reviewable

Is this a bug fix or an improvement? As I understood, CPut ConditionFailed didn't need to update the timestamp cache because the transaction was always ended after a ConditionFailed error was detected.

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

My POC on improving serializable restarts caused me to take a closer look at how things must update the timestamp cache and this popped out as being obviously wrong. @jordanlewis, imagine the following example:

• Txn0 @ts=1 does an insert of key A, which does a CPut on primary key A being empty, which succeeds and A is inserted.
• Txn1 @ts=10 does an insert of key A, but gets a condition failed error because key A exists.
• Txn2 @ts=5 deletes key A (which is allowed @ts=5 because txn1 didn't update timestamp cache), rewriting history!

@nvanbencschoten, I added the above example to the commit message.

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

pkg/storage/replica.go, line 2055 at r1 (raw file):

The chance of that being something we need seems vanishingly unlikely.

Comments from Reviewable

Thanks for the clear example! That makes sense. Yikes! I wonder if any of our Jepsen tests include workloads that exercise conditional put failures. Would this kind of thing have been caught if they did include such tests?

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from Reviewable

I don't think we have any jepsen tests that exercise this, but it should be possible to construct a test that would catch it.

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/storage/replica.go, line 2074 at r1 (raw file):

				key := keys.TransactionKey(start, txnID)
				tc.Add(key, nil, ts, txnID, false /* readCache */)
			case *roachpb.ConditionalPutRequest:

It's unfortunate that we have both a flag in api.go and an entry in the switch statement here - the flags are less valuable if they just need to be synchronized with other code.

Of course, this switch case isn't strictly necessary - it would still be correct to update the tscache for any error from ConditionalPut (or even for any error on any command that updates the tscache). Should we just do that instead of trying to narrowly target certain errors from certain commands? We'd push the timestamp cache forward more than we otherwise would, but it doesn't seem like that would be much of a problem.

Comments from Reviewable

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

pkg/storage/replica.go, line 2074 at r1 (raw file):

I really think it's safer to avoid changes to existing behavior where possible. Given that, I really needed to add this case in order to examine the error in the context of the request.

Comments from Reviewable