storage: integrate Concurrency Manager into Replica request path

[nvanbenschoten]

Related to #41720.
Related to #44976.

This commit integrates the new concurrency package into the storage package. Each Replica is given a concurrency manager, which replaces its existing latch manager and txn wait queue. The change also uses the concurrency manager to simplify the role of the intent resolver. The intent resolver no longer directly handles WriteIntentErrors. As a result, we are able to delete the contention queue entirely.

With this change, all requests are now sequenced through the concurrency manager. When sequencing, latches are acquired and conflicting locks are detected. If any locks are found, the requests wait in lock wait-queues for the locks to be resolved. This is a major deviation from how things currently work because today, even with the contention queue, requests end up waiting for conflicting transactions to commit/abort in the txnWaitQueue after at least one RPC. Now, requests wait directly next to the intents/locks that they are waiting on and react directly to the resolution of these intents/locks.

Once requests are sequenced by the concurrency manager, they are theoretically fully isolated from all conflicting requests. However, this is not strictly true today because we have not yet pulled all replicated locks into the concurrency manager's lock table. We will do so in a future change. Until then, the concurrency manager maintains a notion of "intent discovery", which is integrated into the Replica-level concurrency retry loop.

Performance numbers will be published shortly. This will be followed by performance numbers using the SELECT FOR UPDATE locking (#40205) improvements that this change enables.

[cockroach-teamcity]

This change is 

[sumeerbhola]

Looks good -- only some minor comments.

Reviewed 12 of 12 files at r1, 12 of 12 files at r2, 3 of 3 files at r3, 40 of 40 files at r4, 30 of 30 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 1 of 1 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)

---

pkg/storage/replica.go, line 1153 at r5 (raw file):

	// Release the latches acquired by the request back to the spanlatch
	// manager. Must be done AFTER the timestamp cache is updated. ec.g is only
	// when the Raft proposal has assumed responsibility for releasing latches.

word missing after "only"?

---

pkg/storage/replica_read.go, line 112 at r5 (raw file):

	if lResult.WrittenIntents != nil {
		// These will all be unreplicated locks.

are reads that acquire unreplicated locks only in a read-only batch?

And how does this relate to the TODO earlier in this file about releasing latches early. If the read wants to acquire unreplicated locks it can't release latches, yes? So that TODO only applies to non-locking reads?

---

pkg/storage/replica_send.go, line 134 at r5 (raw file):

// 2. is continuing to execute asynchronously and needs to maintain isolation
//    from conflicting requests throughout the lifetime of its asynchronous
//    processing.

what is an example of this asynchronous processing?

---

pkg/storage/batcheval/result/result.go, line 35 at r2 (raw file):

	// to asynchronous intent processing on the proposer, so that an attempt to
	// resolve them is made.
	EncounteredIntents []roachpb.Intent

I got mired in plumbing code and couldn't find where these are actually used -- could you point me to it?
And why are we trying to do anything with intents that it did not conflict with -- isn't it wasteful given that those transactions may be ongoing and there is no conflict?

---

pkg/storage/batcheval/result/result.go, line 39 at r2 (raw file):

	WrittenIntents []roachpb.LockUpdate
	// ResolvedIntents stores any resolved intent spans, either with finalized
	// or pending statuses. Unlike WrittenIntents and EncounteredIntents, values

what exactly does "finalized or pending statuses" mean in this case -- is it referring to the TransactionStatus?

---

pkg/storage/concurrency/concurrency_control.go, line 318 at r4 (raw file):

	// The maximal set of spans that the request will access. Latches
	// will be acquired for these spans.
	// TODO(nvanbenschoten): don't allocate these SpanSet objects.

how will this "don't allocate" be accomplished?

---

pkg/storage/concurrency/concurrency_manager.go, line 344 at r7 (raw file):

	// state transition, it is safer to simply clear the lockTable and rebuild
	// it from persistent intent state.
	m.lt.Clear()

The concurrency manager exists only on the leaseholder for the range, yes?
Is this the case where the raft leader is at a different node?

---

pkg/storage/concurrency/lock_table_waiter.go, line 161 at r1 (raw file):

				// and quickly resolve abandoned intents then we might be able to
				// get rid of this state.
				delay := LockTableLivenessPushDelay.Get(&w.st.SV)

Doesn't the comment about waiting "for each" contradict this change to add a delay?

---

pkg/storage/concurrency/lock_table_waiter.go, line 245 at r3 (raw file):

		obsTS, ok := h.Txn.GetObservedTimestamp(w.nodeID)
		if !ok {
			// This was set earlier, so it's completely unexpected to

why is !ok not an error anymore?

---

pkg/storage/concurrency/lock_table_waiter_test.go, line 67 at r1 (raw file):

	ir := &mockIntentResolver{}
	st := cluster.MakeTestingClusterSettings()
	LockTableLivenessPushDelay.Override(&st.SV, 1*time.Millisecond)

is there a reason this is overridden to 1ms and not 0ms in tests?

---

pkg/storage/intentresolver/intent_resolver.go, line 277 at r5 (raw file):

// MaybePushTransactions tries to push the conflicting transaction(s):
// either moving their timestamp forward on a read/write conflict, abortint

s/abortint/aborting/

[nvanbenschoten]

Thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)

---

pkg/storage/replica.go, line 1153 at r5 (raw file):

Previously, sumeerbhola wrote…

word missing after "only"?

Done.

---

pkg/storage/replica_read.go, line 112 at r5 (raw file):

are reads that acquire unreplicated locks only in a read-only batch?

Yes, although they will acquire write latches to synchronize properly with other SFU reads. This is a little strange, given that we're in executeReadOnlyBatch. I think we need to reframe this function and executeWriteBatch to make it clear that the most fundamental distinction between them is that the latter can propose to Raft and the former cannot.

And how does this relate to the TODO earlier in this file about releasing latches early. If the read wants to acquire unreplicated locks it can't release latches, yes? So that TODO only applies to non-locking reads?

Great point, I added to the comment.

---

pkg/storage/replica_send.go, line 134 at r5 (raw file):
Added to the comment:

The most prominent example of asynchronous processing is
with requests that have the "async consensus" flag set. A more subtle
case is with requests that are acknowledged by the Raft machinery after
their Raft entry has been committed but before it has been applied to
the replicated state machine. In all of these cases, responsibility
for releasing the concurrency guard is handed to Raft.

---

pkg/storage/batcheval/result/result.go, line 35 at r2 (raw file):

could you point me to it?

They are passed to the IntentResolver here.

And why are we trying to do anything with intents that it did not conflict with -- isn't it wasteful given that those transactions may be ongoing and there is no conflict?

CleanupIntentsAsync uses a PUSH_TOUCH push RPC, so it simply checks whether the intents transaction is alive or not. If it is, it doesn't wait around for it to finish like a PUSH_ABORT style push does.

---

pkg/storage/batcheval/result/result.go, line 39 at r2 (raw file):

is it referring to the TransactionStatus?

Yes, and we call committed or aborted statuses "finalized". See TransactionStatus.IsFinalized.

---

pkg/storage/concurrency/concurrency_control.go, line 318 at r4 (raw file):

Previously, sumeerbhola wrote…

how will this "don't allocate" be accomplished?

I just don't want to store them by reference on this object. I'd rather store them by value.

---

pkg/storage/concurrency/concurrency_manager.go, line 344 at r7 (raw file):

The concurrency manager exists only on the leaseholder for the range, yes?

Well, it exists on all replicas, but it's drained on all replicas but the leaseholder.

Is this the case where the raft leader is at a different node?

Yes, exactly. It's rare but fundamentally possible for the leaseholder to receive a Raft snapshot.

---

pkg/storage/concurrency/lock_table_waiter.go, line 161 at r1 (raw file):

Previously, sumeerbhola wrote…

Doesn't the comment about waiting "for each" contradict this change to add a delay?

The "for each" part of this is referring to waiting out the longer deadlock detection delay. I made this more explicit.

---

pkg/storage/concurrency/lock_table_waiter.go, line 245 at r3 (raw file):

Previously, sumeerbhola wrote…

why is !ok not an error anymore?

It wasn't worth asserting this and breaking a number of tests.

---

pkg/storage/concurrency/lock_table_waiter_test.go, line 67 at r1 (raw file):

Previously, sumeerbhola wrote…

is there a reason this is overridden to 1ms and not 0ms in tests?

No reason. Done.

---

pkg/storage/intentresolver/intent_resolver.go, line 277 at r5 (raw file):

Previously, sumeerbhola wrote…

s/abortint/aborting/

Done.

[tbg]

However, if the EndTxn is in a batch with a Get request, that
Get request does still want to be isolated.

To be fair, it couldn't (we don't support mixed batches) but it doesn't change your point (use a CPut).

split read and write timestamp when scanning lockTable

Could you spell out the example in detail (including how the txn's read and write ts drift)? That test runs inserts only, right? Am morally right that readtimestamp=writetimestamp+maxoffset in practice, even though the code suggests that transactions may do the snapshot thing of reading at T and writing at a later timestamp?

I'm very excited to see this land. Consider this basically an LGTM except I should look at it a little more after you've addressed feedback. I support merging this quickly so that you don't catch skew.

Reviewed 3 of 12 files at r1, 69 of 69 files at r9, 12 of 12 files at r10, 3 of 3 files at r11, 40 of 40 files at r12, 30 of 30 files at r13, 1 of 1 files at r14, 3 of 3 files at r15, 1 of 1 files at r16, 4 of 4 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten and @sumeerbhola)

---

pkg/storage/replica.go, line 224 at r13 (raw file):

	leaseHistory *leaseHistory

	// concMgr sequences incoming requests and provides isoaltion between

isolation

---

pkg/storage/replica_raft.go, line 150 at r13 (raw file):

	quotaSize := uint64(proposal.command.Size())
	if maxSize := uint64(MaxCommandSize.Get(&r.store.cfg.Settings.SV)); quotaSize > maxSize {
		return nil, nil, 0, g, roachpb.NewError(errors.Errorf(

Is it important that g is returned in all of these error cases? I sure hope not... definitely the method comment says it shouldn't. If it is necessary (it has to be, why else would you have done this) return a type errWithGuard struct { *roachpb.Error \n *roachpb.Guard } instead of individual values.

See also

cockroach/pkg/storage/replica_write.go

Line 56 in fff4003

// Returns exactly one of a response, an error or re-evaluation reason.

for another rotted comment

Ok, I see now that this is a general pattern:

cockroach/pkg/storage/replica_send.go

Lines 215 to 222 in fff4003

	br, g, pErr = fn(r, ctx, ba, g)
	switch t := pErr.GetDetail().(type) {
	case nil:
	// Success.
	return br, nil
	case *roachpb.WriteIntentError:
	// Drop latches, but retain lock wait-queues.
	g.AssertLatches()

I really dislike us returning errors along with nonzero values. Not for this PR, but I strongly encourage you to consider the suggestion above (or any other one that avoids this problem).

---

pkg/storage/replica_read.go, line 112 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

are reads that acquire unreplicated locks only in a read-only batch?

Yes, although they will acquire write latches to synchronize properly with other SFU reads. This is a little strange, given that we're in executeReadOnlyBatch. I think we need to reframe this function and executeWriteBatch to make it clear that the most fundamental distinction between them is that the latter can propose to Raft and the former cannot.

And how does this relate to the TODO earlier in this file about releasing latches early. If the read wants to acquire unreplicated locks it can't release latches, yes? So that TODO only applies to non-locking reads?

Great point, I added to the comment.

I had always assumed that having split up the read/rw paths as we do today was mostly an artifact of the past, and that we could send reads into the write path. Is this fundamentally not true? You could imagine wanting to lift the restriction that mixed batches aren't allowed inside of KV, without implementing splitting at the Replica level - mixed batches would have to hit the rw path, so that better be the only path.

---

pkg/storage/replica_send.go, line 139 at r13 (raw file):

//    their Raft entry has been committed but before it has been applied to
//    the replicated state machine. In all of these cases, responsibility
//    for releasing the concurrency guard is handed to Raft.

ditto about returning both a guard and an error

---

pkg/storage/batcheval/cmd_end_transaction.go, line 522 at r10 (raw file):

						log.Fatalf(ctx, "expected resolve allowance to be exactly 0 resolving %s; got %d", intent.Span, resolveAllowance)
					}
					intent.EndKey = resumeSpan.Key

Does this case have a test?

---

pkg/storage/batcheval/cmd_resolve_intent_range.go, line 59 at r10 (raw file):

	reply.NumKeys = numKeys
	if resumeSpan != nil {
		update.EndKey = resumeSpan.Key

ditto

---

pkg/storage/batcheval/result/intent.go, line 41 at r10 (raw file):

// FromWrittenIntents creates a Result communicating that the intents were
// written or re-written by the given request and should be handled.
func FromWrittenIntents(txn *roachpb.Transaction, keys []roachpb.Key) Result {

Why do you have two of these methods and not just a single variadic one? Perf?

---

pkg/storage/batcheval/result/result.go, line 35 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

could you point me to it?

They are passed to the IntentResolver here.

And why are we trying to do anything with intents that it did not conflict with -- isn't it wasteful given that those transactions may be ongoing and there is no conflict?

CleanupIntentsAsync uses a PUSH_TOUCH push RPC, so it simply checks whether the intents transaction is alive or not. If it is, it doesn't wait around for it to finish like a PUSH_ABORT style push does.

For additional color, encountering but not conflicting with intents basically only (?) happens when doing inconsistent reads. I don't remember where exactly we use those but I think in the past we used them on the meta ranges (do we still?) and this code makes sure intents are cleaned up quickly.

---

pkg/storage/batcheval/result/result.go, line 40 at r10 (raw file):

	// ResolvedIntents stores any resolved intent spans, either with finalized
	// or pending statuses. Unlike WrittenIntents and EncounteredIntents, values
	// in this slice can represent spans of intents that were resolved.

will, not can, right? If I'm only looking at WrittenIntents and ResolvedIntents I might wonder why a PENDING intent could be in both.

---

pkg/storage/concurrency/concurrency_control.go, line 276 at r15 (raw file):

	// OnRangeSnapshot informs the concurrency manager that its replica has
	// received a snapshot from another replica in its range.
	OnRangeSnapshot()

nit: why Range and not Replica? Something like OnReplicaAppliedSnapshot()

---

pkg/storage/concurrency/concurrency_manager.go, line 344 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The concurrency manager exists only on the leaseholder for the range, yes?

Well, it exists on all replicas, but it's drained on all replicas but the leaseholder.

Is this the case where the raft leader is at a different node?

Yes, exactly. It's rare but fundamentally possible for the leaseholder to receive a Raft snapshot.

Mind adding that to the comment here?

How does the "rebuilding" work? We start discovering the replicated locks again as they are being read, and we simply lost all the unreplicated locks. This could go in the comment as well.

---

pkg/storage/concurrency/lock_table.go, line 32 at r9 (raw file):

// Default upper bound on the number of locks in a lockTable.
const defaultLockTableSize = 10000

What happens above this? Also, shouldn't this be bounding the number of bytes?

---

pkg/storage/concurrency/lock_table_waiter.go, line 32 at r9 (raw file):

	"kv.lock_table.coordinator_liveness_push_delay",
	"the delay before pushing in order to detect coordinator failures of conflicting transactions",
	10*time.Millisecond,

Isn't this too low? I'd expect it would be ~txn timeout.

---

pkg/storage/concurrency/lock_table_waiter.go, line 40 at r9 (raw file):

	"kv.lock_table.deadlock_detection_push_delay",
	"the delay before pushing in order to detect dependency cycles between transactions",
	100*time.Millisecond,

Some justification for this number would be good, even if it's just a guess.

[sumeerbhola]

Reviewed 1 of 69 files at r9, 6 of 30 files at r13, 4 of 4 files at r17.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten, @sumeerbhola, and @tbg)

---

pkg/storage/concurrency/lock_table.go, line 32 at r9 (raw file):

Also, shouldn't this be bounding the number of bytes?

Yes, that is a TODO.
Do we have a general library that does this via reflection, or do we hand-roll the memory accounting for each data-structure? And are there existing examples in the codebase to look at?

---

pkg/storage/concurrency/lock_table.go, line 849 at r17 (raw file):

		}
		// Locked by some other txn.
		if g.readTS.Less(waitForTs) {

can you add a TODO somewhere that we need to add tests to test when readTS != writeTS?

---

pkg/storage/concurrency/lock_table.go, line 1121 at r17 (raw file):

	}

	switch sa {

I liked this change -- made it easier to understand.

---

pkg/storage/concurrency/lock_table_waiter.go, line 245 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It wasn't worth asserting this and breaking a number of tests.

I'm confused -- some tests take the !ok path? Is that risky (wrt tests not finding bugs)?

[nvanbenschoten]

TFTRs! After we get to a state where we like this, I'm going to wait to merge this until #45567 is also ready to go in, then I'll merge them together.

Performance numbers will be published shortly.

I benchmarked this with a suite of microbenchmarks and full workload benchmarks. The full workload benchmarks look fine:

tpccbench/nodes=3/cpu=16 before	tpccbench/nodes=3/cpu=16 after
1895	1913
1913	1913
1885	1913

The pkg/sql/tests microbenchmarks took a little bit of a hit. Here's the benchdiff output from a few days ago: https://docs.google.com/spreadsheets/d/1W0PFHOw08NXBWgLHyhGc5Zky2oFpOHXCz5ZTV-Qm60o/edit#gid=4. The Bank workload's massive drop in performance is actually a deadlock that was being broken accidentally because we don't launch heartbeat loops for 1PC txns so a txn in the deadlock was expiring. I have a PR to update the kv/contention roachtest to disable this "optimization" so that we never accidentally allow such deadlocks to be resolved - we'd rather them deadlock indefinitely so we can catch the underlying issue, at least in tests. The issue had to do with a CPut forgetting to include WrittenIntents on an error path. The error was being consumed (see #45562) and the CPut was succeeding but the lockTable was not being told about this. This led to a case where a lock was being discovered and added to a lockState that contained the same transaction as a queuedWriter. We weren't handling this case correctly. I fixed the WrittenIntents bug in the second commit and have a PR for that coming for the AddDiscoveredLock issue, which may or may not still be possible to hit.

I also have a series of micro-optimizations around the lockTable and concurrency manager to pool memory. This closes the gap on a lot of the other benchmarks. I'll wait until this PR goes in to continue tuning.

To be fair, it couldn't (we don't support mixed batches) but it doesn't change your point (use a CPut).

Done.

Could you spell out the example in detail (including how the txn's read and write ts drift)? That test runs inserts only, right?

No, the test runs a series of reads and writes in a transaction, all with a fixed commit timestamp: https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/tests/monotonic_insert_test.go#L138

Am morally right that readtimestamp=writetimestamp+maxoffset in practice, even though the code suggests that transactions may do the snapshot thing of reading at T and writing at a later timestamp?

In that test, that's exactly correct because the commit timestamp is fixed at the beginning of the transaction due to the use of cluster_logical_timestamp().

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)

---

pkg/storage/replica.go, line 224 at r13 (raw file):

Previously, tbg (Tobias Grieger) wrote…

isolation

Done.

---

pkg/storage/replica_raft.go, line 150 at r13 (raw file):
Yeah, I don't love this either, but I had a hard time figuring out how to conditionally pass ownership of the concurrency guard (and the responsibility of freeing it at the right time) without this structure. We want Raft to assume ownership of the guard if the proposal makes it to Raft, but we want to continue holding on to the guard if the proposal doesn't make it to Raft. I played around with a few strategies here with communicating back to the caller of evalAndPropose whether it still had ownership of the guard - such as passing a bool to signify this or trying to correlate the error value with whether the guard still needed to be freed or not. Unless I'm missing something, the errWithGuard idea would run into the same kinds of issues. In the end, I decided that trying to hide ownership of the guard just made things more complex.

I'm very open to improving things, but I think we'll need to more aggressively restructure this logic to make any meaningful improvements. For instance, if we split evaluation and Raft proposal into two separate stages from the perspective of executeWriteBatch then some of the complexity around no-op proposals and evaluation errors and how that maps to guard ownership would go away.

for another rotted comment

Done.

---

pkg/storage/replica_read.go, line 112 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I had always assumed that having split up the read/rw paths as we do today was mostly an artifact of the past, and that we could send reads into the write path. Is this fundamentally not true? You could imagine wanting to lift the restriction that mixed batches aren't allowed inside of KV, without implementing splitting at the Replica level - mixed batches would have to hit the rw path, so that better be the only path.

It should be true and we're continuing to narrow the gap between these two code paths. I think the largest remaining difference between them is that we use a more efficient LSM iterator on the read path instead of a full write batch. There's also some complexity around which kinds of retries are appropriate for reads vs. writes because writes can arbitrarily increase their timestamps without violating their latches while reads have to be more careful about doing so. Finally, I think we'd want to pull the evaluation/proposal split (currently in evalAndPropose) up a level if we were to do so.

All of these would be good changes though, so I'm in support of such a refactor.

---

pkg/storage/replica_send.go, line 139 at r13 (raw file):

Previously, tbg (Tobias Grieger) wrote…

ditto about returning both a guard and an error

Let's discuss in the previous comment.

---

pkg/storage/batcheval/cmd_end_transaction.go, line 522 at r10 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Does this case have a test?

No, not yet. It's not actually necessary for correctness, just performance. I'd like to add a lot more testing around batch evaluation and the result.Result that we get out of it, but I'd rather not include that testing in this already very bloated PR.

---

pkg/storage/batcheval/cmd_resolve_intent_range.go, line 59 at r10 (raw file):

Previously, tbg (Tobias Grieger) wrote…

ditto

Same response as above.

---

pkg/storage/batcheval/result/intent.go, line 41 at r10 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why do you have two of these methods and not just a single variadic one? Perf?

No reason, escape analysis can prove that the array passed to the variadic function won't escape when given a constant number of args, so using variadics wouldn't force any new heap allocations or anything. Done.

---

pkg/storage/batcheval/result/result.go, line 35 at r2 (raw file):

do we still?

We scan using a READ_UNCOMMITTED isolation level, which as the same behavior.

---

pkg/storage/batcheval/result/result.go, line 40 at r10 (raw file):

Previously, tbg (Tobias Grieger) wrote…

will, not can, right? If I'm only looking at WrittenIntents and ResolvedIntents I might wonder why a PENDING intent could be in both.

Done. "Can" was referring to the possibility of them being spans, not of them being resolved. But the wording was confusing.

---

pkg/storage/concurrency/concurrency_control.go, line 276 at r15 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: why Range and not Replica? Something like OnReplicaAppliedSnapshot()

Done.

---

pkg/storage/concurrency/concurrency_manager.go, line 344 at r7 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Mind adding that to the comment here?

How does the "rebuilding" work? We start discovering the replicated locks again as they are being read, and we simply lost all the unreplicated locks. This could go in the comment as well.

Done.

---

pkg/storage/concurrency/lock_table.go, line 32 at r9 (raw file):

Do we have a general library that does this via reflection, or do we hand-roll the memory accounting for each data-structure?

We typically hand-roll memory accounting for each data structure. git grep 'unsafe.Sizeof' pkg/storage will give you some examples of this approach in our codebase, such as the timestamp cache and the raftentry cache. This isn't too burdensome in practice, but I can't say we're particularly in love with the approach either, so if you've used nicer patterns in the past, this might be a good time to experiment with introducing them to the codebase.

---

pkg/storage/concurrency/lock_table.go, line 849 at r17 (raw file):

Previously, sumeerbhola wrote…

can you add a TODO somewhere that we need to add tests to test when readTS != writeTS?

Done.

---

pkg/storage/concurrency/lock_table_waiter.go, line 245 at r3 (raw file):

Previously, sumeerbhola wrote…

I'm confused -- some tests take the !ok path? Is that risky (wrt tests not finding bugs)?

Some tests are below the level where transactions get assigned observed timestamps and so they hit the !ok path. I wouldn't say that's particularly risky, just that it's a bit unrealistic. But this is simply a heuristic to avoid extra transaction retries, so it's not very concerning either.

Still, I'll update this to handle the missing observed timestamp case more gracefully, which also helps drive home the point of the heuristic. Done in new commit.

---

pkg/storage/concurrency/lock_table_waiter.go, line 32 at r9 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Isn't this too low? I'd expect it would be ~txn timeout.

This is discussed in the comment on case waitForDistinguished. If we had a cache for aborted txn IDs then I'd feel better about bumping this up, but we want to avoid waiting out a long delay on each abandoned intent, so for now, we keep it fairly short.

I also have an upcoming change that explains this even further.

EDIT: actually, I added a commit to explain this here as well.

---

pkg/storage/concurrency/lock_table_waiter.go, line 40 at r9 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Some justification for this number would be good, even if it's just a guess.

This is what the number is today in the contentionQueue.

Added a commit explaining this and the previous value.

[nvanbenschoten]

I have a PR to update the kv/contention roachtest to disable this "optimization" so that we never accidentally allow such deadlocks to be resolved - we'd rather them deadlock indefinitely so we can catch the underlying issue, at least in tests.

This is #45568.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola and @tbg)

[craig]

Canceled (will resume)

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)