kv/kvserver: don't bump ReadTimestamp on EndTxn batch with bounded read latches

[nvanbenschoten]

This commit prevents an error-prone situation where requests with an
EndTxn request that acquire read-latches at bounded timestamps were
allowed to move their ReadTimestamp to their WriteTimestamp without
re-acquiring new latches. This could allow the request to read at an
unprotected timestamp and forgo proper synchronization with writes with
respect to its lock-table scan and timestamp cache update.

This relates to 11bffb2, which made a similar fix to server-side
refreshes, but not to this new (as of this release) pre-emptive
ReadTimestamp update mechanism.

This bug was triggering the following assertion when stressing
TestKVNemesisSingleNode:

F200326 15:56:11.350743 1199 kv/kvserver/concurrency/concurrency_manager.go:261  [n1,s1,r33/1:/{Table/50/"60…-Max}] caller violated contract: discovered non-conflicting lock

The batch that hit this issue looked like:

Get [/Table/50/"a1036fd0",/Min),
Get [/Table/50/"e54a51a8",/Min),
QueryIntent [/Table/50/"aafccb40",/Min),
QueryIntent [/Table/50/"be7f37c9",/Min),
EndTxn(commit:true tsflex:true)

This commit adds two subtests to TestTxnCoordSenderRetries that
create this scenario.

Release note (bug fix): A rare bug causing an assertion failure was
fixed. The assertion error message was "caller violated contract:
discovered non-conflicting lock".

Release justification: fixes a series bug that could crash a server.
Additionally, the bug could have theoretically allowed isolation
violations between transactions without hitting the assertion, though
this was never observed in practice.

cc. @danhhz

[cockroach-teamcity]

This change is 

[danhhz]

Cool! Are there any ops that this allows us to reenable in kvnemesis?

cockroach/pkg/kv/kvnemesis/generator.go

Lines 173 to 189 in 4bce96a

	func NewDefaultConfig() GeneratorConfig {
	config := newAllOperationsConfig()
	// TODO(dan): This fails with a WriteTooOld error if the same key is Put twice
	// in a single batch. However, if the same Batch is committed using txn.Run,
	// then it works and only the last one is materialized. We could make the
	// db.Run behavior match txn.Run by ensuring that all requests in a
	// nontransactional batch are disjoint and upgrading to a transactional batch
	// (see CrossRangeTxnWrapperSender) if they are. roachpb.SpanGroup can be used
	// to efficiently check this.
	//
	// TODO(dan): Make this `config.Ops.Batch.Ops.PutExisting = 0` once #46081 is
	// fixed.
	config.Ops.Batch = BatchOperationConfig{}
	// TODO(dan): Remove when #45586 is addressed.
	config.Ops.ClosureTxn.CommitBatchOps.GetExisting = 0
	config.Ops.ClosureTxn.CommitBatchOps.GetMissing = 0
	return config

[andreimatei]

LGTM

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

---

pkg/kv/kvserver/replica_write.go, line 312 at r1 (raw file):

// retryable error.
func (r *Replica) evaluateWriteBatch(
	ctx context.Context, idKey storagebase.CmdIDKey, ba *roachpb.BatchRequest, spans *spanset.SpanSet,

can we pls sprinkle some comments about this spans field everywhere? And can we rename it to latches?

Even better, let's combine it with ba in a struct (serializedRequest? or just request). It can embed a *roachpb.BatchRequest).

[nvanbenschoten]

TFTRs!

Are there any ops that this allows us to reenable in kvnemesis?

I don't think so. This was resolving a crash that was possible with transactional batches, not a silent error.

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

---

pkg/kv/kvserver/replica_write.go, line 312 at r1 (raw file):

can we pls sprinkle some comments about this spans field everywhere? And can we rename it to latches?

Done.

Even better, let's combine it with ba in a struct (serializedRequest? or just request). It can embed a *roachpb.BatchRequest).

Added a TODO.

[nvanbenschoten]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[otan]

is the test failure in ^ (https://teamcity.cockroachdb.com/viewLog.html?buildId=1837699&tab=buildResultsDiv&buildTypeId=Cockroach_UnitTests_Test) related to this PR?

seems scary either way to happen in CI :o

[nvanbenschoten]

is the test failure in ^ (https://teamcity.cockroachdb.com/viewLog.html?buildId=1837699&tab=buildResultsDiv&buildTypeId=Cockroach_UnitTests_Test) related to this PR?

No, that looks like #21444.

[craig]

Build succeeded

• GitHub CI (Cockroach)