Reviewed 3 of 3 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

pkg/kv/kvserver/concurrency/lock_table.go line 1640 at r2 (raw file):

			var ok bool
			replicatedFinalizedTxn, ok = g.lt.finalizedTxnCache.get(l.holder.holder[lock.Replicated].txn.ID)
			assert(ok, "expected txn to be finalized")

Is the cache guaranteed to still contain the transaction at this point? It's a small LRU cache that can be manipulated concurrently.

pkg/kv/kvserver/concurrency/lock_table.go line 1660 at r2 (raw file):

	// a claim like normal. That is, it should add itself to the lock's wait
	// queue as an inactive writer.
	if !l.holder.locked && l.queuedWriters.Len() == 0 {

Is this the same condition as if transitionedToFree { return false, true }?

pkg/kv/kvserver/concurrency/lock_table.go line 1683 at r2 (raw file):

			// prepareActiveWait and returning true here; there won't really be any
			// wait here.
			l.prepareActiveWait(g, notify, clock)

Doesn't this also call into adjustWaitingWritersWaitQueue? I'm surprised that we need this second handling of isMaxQueueLengthExceededError here.

pkg/kv/kvserver/concurrency/lock_table.go line 1712 at r2 (raw file):

	// it is).
	guardLockMode := lock.MakeModeIntent(g.ts)
	for e := l.queuedWriters.Front(); e != nil; e = e.Next() {

I wonder whether there's room to unify this logic with the incremental handling of lock releasing. There's some duplicated logic between this function and maybeReleaseFirstTransactionalWriter.

I think one of the biggest sources of complexity in this code is the multiple, non-idempotent code paths that attempt to maintain the same invariants while handling different state transitions. That's only warranted if the redundancy is necessary for efficiency. But is it in this case? Could we treat queuing as the combination of:

1. adding to the end of the wait queue, assuming we're going to have to wait
2. releasing the first transactional writer, which may be us

pkg/kv/kvserver/concurrency/lock_table.go line 1714 at r2 (raw file):

	for e := l.queuedWriters.Front(); e != nil; e = e.Next() {
		qqg := e.Value.(*queuedGuard)
		if qqg.guard == g {

Let's call out what's happened here:

// We found our request while scanning from the front before finding any conflicting waiters. There is no one for us to wait on.

pkg/kv/kvserver/concurrency/lock_table.go line 1721 at r2 (raw file):

			}
			// Transactional writer.
			return false, transitionedToFree

Is it safe to return transitionedToFree = true here if we just added our own waiter?

pkg/kv/kvserver/concurrency/lock_table.go line 1725 at r2 (raw file):

		waiterLockMode := lock.MakeModeIntent(qg.guard.ts)
		if lock.Conflicts(waiterLockMode, guardLockMode, &g.lt.settings.SV) {
			l.prepareActiveWait(g, notify, clock)

Same question. Are we inserting into the queue twice by first calling adjustWaitingWritersWaitQueue directly and then calling prepareActiveWait?

pkg/kv/kvserver/concurrency/lock_table.go line 1850 at r2 (raw file):

}

// updateWaitingState updates the waiting state for the supplied request to wait

The comment on this function is incorrect.

pkg/kv/kvserver/concurrency/lock_table.go line 3323 at r2 (raw file):

}

type maxQueueLengthExceededError struct{}

Let's give this a comment. I'm also not sure how useful it is to have an error return path that can only return one kind of error. Up to you.

Flushing replies to a few comments.

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

pkg/kv/kvserver/concurrency/lock_table.go line 1583 at r2 (raw file):

// resolution (or done it), so this request does not need to do the resolution.
//
// TODO(XXX): The paragraph above is now stale. We're letting writers accumulate

Wanted to call out this part for discussion.

pkg/kv/kvserver/concurrency/lock_table.go line 1640 at r2 (raw file):

Good point. The only reason I needed this was because I didn't want to I need a roachpb.Transaction here. I could just modify conflictsWithLockHolder to return the replicatedFinalizedTxn instead of belongsToFinalizedTxn.

pkg/kv/kvserver/concurrency/lock_table.go line 1660 at r2 (raw file):

Yeah, it is. I can make the switch if we feel that's easier to reason about.

pkg/kv/kvserver/concurrency/lock_table.go line 1683 at r2 (raw file):

Yeah, but handling the the isMaxQueueLengthExceededError involves the same changes prepareForActiveWait makes when it hits this error. I wasn't sure what's clearer -- duplicating those changes here, or reusing prepareActiveWait. We could instead handle with this as well:

--- a/pkg/kv/kvserver/concurrency/lock_table.go
+++ b/pkg/kv/kvserver/concurrency/lock_table.go
@@ -1680,7 +1680,15 @@ func (l *lockState) tryActiveWait(
                        // based on their waiting state. The mechanism by which this happens is
                        // prepareActiveWait and returning true here; there won't really be any
                        // wait here.
-                       l.prepareActiveWait(g, notify, clock)
+                       g.key = l.key
+                       g.mu.startWait = true
+                       g.mu.curLockWaitStart = clock.PhysicalTime()
+                       ws := l.constructWaitingState(g)
+                       ws.kind = waitQueueMaxLengthExceeded
+                       g.updateWaitingStateLocked(ws)
+                       if notify {
+                               g.notify()
+                       }
                        return true, transitionedToFree
                }
                panic(err) // should not happen

Is this clearer?

pkg/kv/kvserver/concurrency/lock_table.go line 1712 at r2 (raw file):

Yeah. Now that you mention it, I think we need a call to informActiveWaiters here in the future -- we may have inserted this guy at the front of the queue, and the claimant txn may have changed as a result.

pkg/kv/kvserver/concurrency/lock_table.go line 1721 at r2 (raw file):

It should be safe, because we check if a lock is non-empty before GC-ing it. It might result in an unnecessary call to tryGCLocks, but it shouldn't be a correctness issue. I could also see why we should just return false instead. I could go either way here -- I actually did switch this locally right before pushing. Let me know if you have a preference either way.

pkg/kv/kvserver/concurrency/lock_table.go line 1725 at r2 (raw file):

adjustWaitingWritersWaitQueue has logic to handle the case where a request already exists. In this case, it'll effectively mark the request as an active waiter (we tentatively inserted it as an inactive waiter above).

Thanks, RFAL.

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

pkg/kv/kvserver/concurrency/lock_table.go line 1640 at r2 (raw file):

I meant finalizedTxn here. Updated.

pkg/kv/kvserver/concurrency/lock_table.go line 1660 at r2 (raw file):

(haven't done this)

pkg/kv/kvserver/concurrency/lock_table.go line 1683 at r2 (raw file):

Made this change for now. Let me know how you feel about this.

pkg/kv/kvserver/concurrency/lock_table.go line 1712 at r2 (raw file):

I sat with this for a bit, and I'm leaning towards not making the change. A source of complexity in tryActiveWait is the somewhat surprising side effects of this function. For example, the calls to clearLockHolder and lockIsFree which can uncork unrelated requests, like I was talking to you about on Slack. Longer term, I think it'll be easier to reason about tryActiveWait if it only performs state changes that directly fall out of whether the supplied request should wait or not.

In the future, maybeReleaseFirstTransactionalWriter will probably be replaced by maybeReleaseAllCompatibleRequestsFromHeadOfTheQueue[1]. Such a function is more general than what we need here. It begs the same question as above, why is tryActiveWait uncorking other requests?

What's even more subtle is that it'll actually never uncork any other request at all! It'll either mark itself as inactive or be a no-op [2]. So making this change feels like we're needlessly expanding the scope of states we can be in at this point.

I did update this to call informActiveWaiters though. This motivated adding a new commit (2 of 3).

[1] maybeReleaseAllCompatibleRequestsFromHeadOfTheQueue will traverse the head of the queued locking requests list, removing any non-transactional write requests it encounters, and marking all compatible requests as inactive waiters and nudging them to proceed with their scan. It'll stop when it discovers an incompatible request.

[2] That's because if there were a request that was compatible with the head of the lock's wait queue (and potentially the lock holder), it shouldn't have been an active waiter to begin with.

pkg/kv/kvserver/concurrency/lock_table.go line 1850 at r2 (raw file):

Thanks, fixed.

pkg/kv/kvserver/concurrency/lock_table.go line 3323 at r2 (raw file):

Added some comments. Mild preference to keep these using errors to indicate cases where we didn't modify the wait queues instead of returning a boolean, so keeping this as is.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

-- commits line 11 at r3:
What do you think about keeping this "elide signaling" change separate? It feels self-contained and probably deserves more scrutiny than it will get if grouped into this PR. A few questions to ask ourselves about it:

• what kind of bugs can occur from under-eliding? What kind of bugs can occur from over-eliding?
• is the logic in canElideWaitingStateUpdate prone to rot as new fields are added to waitingState?
• does the logic in canElideWaitingStateUpdate need to consider every field? Even observability-related fields like queuedWriters?
• can we push this into updateWaitingStateLocked so that we don't add complexity to existing methods?

pkg/kv/kvserver/concurrency/lock_table.go line 1660 at r2 (raw file):

Yeah, I think it is easier to reason about. It makes it clear to readers why this lock is now unheld and with an empty wait queue. In fact, maybe move it up to where transitionedToFree is set so that it doesn't mix with other paths through this function.

+1 to removing the special case (in a future PR), by the way.

pkg/kv/kvserver/concurrency/lock_table.go line 1721 at r2 (raw file):

Related to the thread above, it's mildly confusing that we set transitionedToFree way up above and then keep using the variable throughout. It raises questions about whether any of the state transitions that we've performed have invalidated this stale piece of information. I'd suggest only using transitionedToFree on the one path that sets it and returning false (the literal) everywhere else.