concurrency: get rid of reservations in the lock table

[arulajmani]

First 3 commits from #103373

This patch removes the notion of reservations from the lock table.
Reservations served as a claim that prevented multiple requests from
racing when a lock was released. Typically, when a lock was released,
only the first transactional writer was released from the list of
queued writers. It would do so by claiming a "reservation" on the
lock.

All requests that are sequenced through the lock table are associated
with a sequence number based on arrival order. These sequence numbers
are used to uphold ~fairness as requests are sequenced. They also serve
a correctness purpose -- because all locks are not known upfront (as
uncontended replicated locks may be discovered during evaluation),
sequence numbers are used to break potential deadlocks that arise from
out of order locking. This motivated the concept of reservation
breaking, which could happen if a lower sequence number request
encountered a reservation by a request with a higher sequence number.
This would lead to somewhat complex state management, where requests
could move from being reservations to inactive waiters multiple times
during their lifetime. A lot of this can be simplified if we make no
distinction between a reservation and an inactive waiter.

This patch gets rid of reservations entirely. Instead, it offers a new
invariant:

The head of the list of waiting writers should always be an inactive,
transactional writer if the lock isn't held.

In practice, this works out functionally the same as how reservations
operated, albeit with less state transitions. Being an inactive waiter
at the head of the lock's wait-queue serves as the request's claim on
the key. As such, verbiage that referenced "reservations" previously is
now updated to talk about claims and claimant transactions. There's a
bit of comment churn as a result. There's also some datadriven test
churn as part of this patch -- but it should be helpful in convincing
ourselves that this just changes concepts, and not functionality. In
particular, what was previously a reservation holder, is now the first
inactive queued qwriter at the lock.

Closes #103361

Release note: None

[cockroach-teamcity]

This change is 

[arulajmani]

@nvanbenschoten I've rebased this and polished some of the commentary to talk about "claims" and "claimant transactions" , instead of reservations. There's some references to reservations left in tryActiveWait, but that'll go away very soon when the PR that refactors it is fixed.

Also, I think this waitForSelf and waitForDistinguished stuff won't extend well to multiple lock holders. Let's talk about it a bit more when we meet later today. I don't think it needs to necessarily block this PR, though.

[nvanbenschoten]

Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @jbowens)

---

-- commits line 32 at r1:
https://www.youtube.com/watch?v=G0zNWswcqMg

---

-- commits line 40 at r1:
"qwriter"

---

pkg/kv/kvserver/concurrency/lock_table.go line 67 at r1 (raw file):

	// notification without pushing anyone.
	//
	// By definition, the lock cannot be held at this point -- if it were, another

Thanks for calling this out in the comment.

---

pkg/kv/kvserver/concurrency/lock_table.go line 72 at r1 (raw file):

	//
	// TODO(arul): this waitSelf state + claimantTxn stuff won't extend well to
	// multiple lock holders. See TODO  informActiveWaiters.

"See TODO in ..."

---

pkg/kv/kvserver/concurrency/lock_table.go line 218 at r1 (raw file):

	// - Same as example 1 but lock at A is held by txn3 and lock at B is held
	//   by txn4.
	// - Lock at A is released so req1 acquires claims the lock at A and starts

"acquires claims"

---

pkg/kv/kvserver/concurrency/lock_table.go line 461 at r1 (raw file):

		// computation makes the waiters do this work themselves instead of making
		// the call to release locks or update locks or remove the request's claims
		// on (unheld) locks. This is proportional to number of

nit: rewrap "waiters"

---

pkg/kv/kvserver/concurrency/lock_table.go line 935 at r1 (raw file):

	// and in particular locking strengths that are compatible with each other
	// (read: shared locks), one could imagine a scheme where the head of the
	// queuedWriters (s/queuedWriters/queuedLockingRequests/g) that is compatible

nit: queuedLockers may be a bit more succinct.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1214 at r1 (raw file):

func (l *lockState) informActiveWaiters() {
	if l.waitingReaders.Len() == 0 && l.queuedWriters.Len() == 0 {
		return // no active readers to speak of; early return

"no active waiters to speak to"?

---

pkg/kv/kvserver/concurrency/lock_table.go line 1369 at r1 (raw file):

			// Only requests actively waiting at this lock should be considered for
			// the distinguished distinction.
			if qg.active {

Did we lose the protection against guards waiting on their own claimant txn? In other words, can a request that should be in waitSelf now become the distinguished waiter?

On a related note, I see that we don't use the distinguished waiter state when waiting on a claimant that does not hold the lock. That makes sense, as liveness pushes are not needed. Would it be helpful to only assign a distinguished waiter when the lock is held?

---

pkg/kv/kvserver/concurrency/lock_table.go line 1386 at r1 (raw file):

// Returns true iff the lockState is empty, i.e., there is no lock holder and no
// inactive writers.

Should we just say "no waiters"? "inactive writers" (the translation from "reservation") feels unnecessarily specific. It's enough to confuse a reader into thinking the method is doing more than it is.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1677 at r1 (raw file):

		qg := l.queuedWriters.Front().Value.(*queuedGuard)
		if qg.guard == g {
			// Already reserved by this request.

s/reserved/claimed/

---

pkg/kv/kvserver/concurrency/lock_table.go line 1680 at r1 (raw file):

			return false, false
		}
		// A non-transactional write request never makes or breaks reservations, and

More "reservation" terminology here and below.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1693 at r1 (raw file):

	// Incompatible with whoever is holding lock or reservation.

	if !l.holder.locked && l.queuedWriters.Len() > 0 && str == lock.Intent && l.tryBreakReservation(g.seqNum) {

I'm surprised that we still need this branch. I would have expected the logic it contains to come for free with the more general queuedWaiters insertion logic below (i.e. if qqg.guard.seqNum < qg.guard.seqNum {).

---

pkg/kv/kvserver/concurrency/lock_table.go line 1957 at r1 (raw file):

	}

	// The lock isn't held, so the request here must be an inactive waiting writer

What request is this comment talking about? Is the comment out of place with the new code structure?

---

pkg/kv/kvserver/concurrency/lock_table.go line 2426 at r1 (raw file):

}

// The lock has transitioned from locked to unlocked. There could be waiters.

I'm not sure about you, but I find this method's name and comment to be unnecessarily confusing. That's because the method's name makes it sound like it's read-only and the comment doesn't give any indication of the state transitions it may perform. Consider improving this, either in this PR or a follow-up PR.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2504 at r1 (raw file):

		qg.active = false // mark as inactive
		if g == l.distinguishedWaiter {
			l.distinguishedWaiter = nil

Are we handling the case where we remove the distinguishedWaiter here correctly in each of the callers? I don't think we are, but maybe that's fine (see my comment about this state up above).

---

pkg/kv/kvserver/concurrency/metrics.go line 36 at r1 (raw file):

	// The number of locks not held, but with reservations.
	// TODO(arul): this needs to be fixed now that we don't have reservations
	// anymore. File an issue about this metrics structure needing update as a

Reminder to file issues. Also, we can include the issue number in the TODO when you do.

[arulajmani]

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 67 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Thanks for calling this out in the comment.

💯

---

pkg/kv/kvserver/concurrency/lock_table.go line 1369 at r1 (raw file):
Good catch!

On a related note, I see that we don't use the distinguished waiter state when waiting on a claimant that does not hold the lock. That makes sense, as liveness pushes are not needed. Would it be helpful to only assign a distinguished waiter when the lock is held?

You're right. Like we spoke about earlier today, let's be dumb about this for now and assign a distinguished waiter status for now. We should keep this in mind as we evolve this code here, and maybe even consider if the concept of a distinguished waiter is worth keeping around. There's a fair bit of state transition involved here for liveness pushes. We could instead have all waiters perform separate liveness pushes.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1386 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we just say "no waiters"? "inactive writers" (the translation from "reservation") feels unnecessarily specific. It's enough to confuse a reader into thinking the method is doing more than it is.

Yeah, I agree. Done.

After coming back to this after a week, I realized the way I had refactored this thing was unnecessarily complicated as well -- reworked the implementation as well.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1680 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

More "reservation" terminology here and below.

Heh, I'd entirely skipped over the terminology in tryActiveWait because we're refactoring it anyway in a followup.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1693 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm surprised that we still need this branch. I would have expected the logic it contains to come for free with the more general queuedWaiters insertion logic below (i.e. if qqg.guard.seqNum < qg.guard.seqNum {).

We don't, but I chose to stick with as direct a translation of the current structure as possible given this function will be reworked in the next PR.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1957 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

What request is this comment talking about? Is the comment out of place with the new code structure?

It's talking about the request that's calling into this function, trying to acquire the lock. You're right, it does feel a bit out of place right above l.releaseWritersFromTxn.

I've improved it a bit, and written it as an NB. I could go either way here, so if it feels unnecessary, we can just remove it -- let me know.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2426 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm not sure about you, but I find this method's name and comment to be unnecessarily confusing. That's because the method's name makes it sound like it's read-only and the comment doesn't give any indication of the state transitions it may perform. Consider improving this, either in this PR or a follow-up PR.

Yeah, I've felt similarly before. I'll add a TODO and address it in a followup patch.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2504 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are we handling the case where we remove the distinguishedWaiter here correctly in each of the callers? I don't think we are, but maybe that's fine (see my comment about this state up above).

Is the hazard you're alluding to here that we aren't finding a new distinguishedWaiter to replace the one we're removing? If so, informActiveWaiters should be the one taking care of that. It's subtle, so I'll add a comment to that effect.

I may be misunderstanding the hazard you're alluding to -- let me know if that's the case.

[nvanbenschoten]

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 1693 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

We don't, but I chose to stick with as direct a translation of the current structure as possible given this function will be reworked in the next PR.

I think it would be good to get rid of it (and tryBreakReservation) if we can. Doing so provides more confidence that this refactor is sound. Is it as simple as deleting the branch?

---

pkg/kv/kvserver/concurrency/lock_table.go line 1957 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

It's talking about the request that's calling into this function, trying to acquire the lock. You're right, it does feel a bit out of place right above l.releaseWritersFromTxn.

I've improved it a bit, and written it as an NB. I could go either way here, so if it feels unnecessary, we can just remove it -- let me know.

Two spaces between "to" and "acquire".

But more importantly, this could benefit from one more sentence that explains why/how these rare cases can occur.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2504 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is the hazard you're alluding to here that we aren't finding a new distinguishedWaiter to replace the one we're removing? If so, informActiveWaiters should be the one taking care of that. It's subtle, so I'll add a comment to that effect.

I may be misunderstanding the hazard you're alluding to -- let me know if that's the case.

That was what I was referring to. Thanks for explaining.

---

pkg/kv/kvserver/concurrency/lock_table.go line 2570 at r3 (raw file):

		qg.active = false // mark as inactive
		if g == l.distinguishedWaiter {
			// We're only clearing hte distinguishedWaiter for now; a new one will be

"hte"

[arulajmani]

Thanks for the detailed review on this one! RFA(hopefully last)L.

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 1693 at r1 (raw file):

Is it as simple as deleting the branch?

~mostly. I did have to comment out an assertion that I'm slightly sad about, but we'll bring it back once the tryActiveWait refactor lands.

---

pkg/kv/kvserver/concurrency/lock_table.go line 1957 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Two spaces between "to" and "acquire".

But more importantly, this could benefit from one more sentence that explains why/how these rare cases can occur.

Added a few words

---

pkg/kv/kvserver/concurrency/metrics.go line 36 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Reminder to file issues. Also, we can include the issue number in the TODO when you do.

Done, and included it here as well.

[nvanbenschoten]

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

[arulajmani]

TFTR!

bors r=nvanbenschoten

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[arulajmani]

Flaked; retrying.

bors r=nvanbenschoten

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[sumeerbhola]

(late drive-by questions)

Reviewed 2 of 28 files at r1, 1 of 3 files at r2, 1 of 11 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/concurrency/lock_table.go line 72 at r4 (raw file):

	//
	// TODO(arul): this waitSelf state + claimantTxn stuff won't extend well to
	// multiple lock holders. See TODO in informActiveWaiters.

I'm assuming the claimantTxn stuff will need to be extended for the case mentioned in the following old comment:

	//   For dependencies, a waiter desiring an exclusive or upgrade lock always
	//   conflicts with the holder(s) or reserver(s) so that is the dependency
	//   that will be captured. A waiter desiring a shared lock may encounter a
	//   situation where it does not conflict with the holder(s) or reserver(s)
	//   since those are also shared lockers. In that case it will depend on the
	//   first waiter since that waiter must be desiring a lock that is
	//   incompatible with a shared lock.

That is, a shared lock waiter (that is not a claimant) does not depend on the claimant(s) if they are also acquiring (or have acquired) a shared lock, and instead depends on the first waiter in the queue that is after the claimants.

Is this what the TODO is referring to, or is there something I am missing?

---

pkg/kv/kvserver/concurrency/lock_table.go line 888 at r4 (raw file):

	// When a lock is not held, the head of the list should be comprised of an
	// inactive, transactional writer (if the list is non-empty). Keeping its
	// position as an inactive waiter at the head of the queue serves as a claim

The head of the queue being an inactive is an invariant, yes? But there can also be inactive waiters in other parts of the queue (like we had before), yes?
IIUC, the notion of when someone is active or inactive has been preserved by this PR without any semantic change.

---

pkg/kv/kvserver/concurrency/lock_table.go line 939 at r4 (raw file):

	// and in particular locking strengths that are compatible with each other
	// (read: shared locks), one could imagine a scheme where the head of the
	// queuedWriters (s/queuedWriters/queuedLockers/g) that is compatible with

was this text replacement forgotten?

---

pkg/kv/kvserver/concurrency/lock_table.go line 943 at r4 (raw file):

	//
	// Once we introduce joint claims, we'll also need to support partially
	// breaking such claims. This means that a request that was previously

Am I correct in understanding that breaking such a claim no longer requires any state changes in the data-structure?

[arulajmani]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/concurrency/lock_table.go line 72 at r4 (raw file):

Previously, sumeerbhola wrote…

I'm assuming the claimantTxn stuff will need to be extended for the case mentioned in the following old comment:

	//   For dependencies, a waiter desiring an exclusive or upgrade lock always
	//   conflicts with the holder(s) or reserver(s) so that is the dependency
	//   that will be captured. A waiter desiring a shared lock may encounter a
	//   situation where it does not conflict with the holder(s) or reserver(s)
	//   since those are also shared lockers. In that case it will depend on the
	//   first waiter since that waiter must be desiring a lock that is
	//   incompatible with a shared lock.

That is, a shared lock waiter (that is not a claimant) does not depend on the claimant(s) if they are also acquiring (or have acquired) a shared lock, and instead depends on the first waiter in the queue that is after the claimants.

Is this what the TODO is referring to, or is there something I am missing?

Yeah, it's this general theme of problems of "who should we be pushing". I drew out a very similar example in the TODO in informActiveWaiters.

Since I wrote this comment, @nvanbenschoten, @AlexTalks, and I discussed this a bit more. One alternative suggested was that a request could just push the claimant even if it was compatible with it. The insight there being that we're locally collapsing the dependency graph on a single range, before engaging distributed deadlock detection.

There's still a few cases, involving non-transactional writers and lock promotions, that we haven't fully thought through and solidified. For example, one interesting case Nathan and I spoke about was:

key:
- lock: txn1, shared
- queue: [txn2(intent), txn1(exclusive)] // head at index 0```

How do we avoid/detect such a deadlock, assuming the alternative scheme above?

---

I know that a lot of this wasn't obvious from the vague TODO I wrote here and may only be indirectly related to the question you were asking.

---

pkg/kv/kvserver/concurrency/lock_table.go line 888 at r4 (raw file):

Previously, sumeerbhola wrote…

The head of the queue being an inactive is an invariant, yes? But there can also be inactive waiters in other parts of the queue (like we had before), yes?
IIUC, the notion of when someone is active or inactive has been preserved by this PR without any semantic change.

Yeah, everything you say is correct.

---

pkg/kv/kvserver/concurrency/lock_table.go line 939 at r4 (raw file):

Previously, sumeerbhola wrote…

was this text replacement forgotten?

When we introduce shared locks, calling the list queuedWriters wouldn't make sense -- we'd instead want to store all locking requests in a single list (say queuedLockers). This was mostly a pointer that by the time joint claims are a thing, this list will/should be renamed. Now I'm thinking maybe this detail could be omitted entirely.

---

pkg/kv/kvserver/concurrency/lock_table.go line 943 at r4 (raw file):

Previously, sumeerbhola wrote…

Am I correct in understanding that breaking such a claim no longer requires any state changes in the data-structure?

Yeah. Breaking such a claim would entail slotting in the correct (as dictated by the sequence number) position.

The request who's claim has been broken might find out about it when re-scanning the lock table, in which case it'll decide to wait on a lock that it previously thought it had already claimed. Or, it may have already proceeded to evaluation and find out the claim was broken when it calls into the lock table to acquire a lock -- it'll be to acquire the lock in such cases because it's protected by latching (the claim must have been broken by a request not holding latches).

Semantically, getting rid of reservations doesn't change too much in this regard. Like you pointed out, the only thing that changes is we don't have to manage state between reservations and queued writers.

[sumeerbhola]

Reviewable status: complete! 1 of 0 LGTMs obtained

---

pkg/kv/kvserver/concurrency/lock_table.go line 72 at r4 (raw file):
Can you elaborate on that example? I didn't quite follow it. Also, it does not seem to have a non-transactional writer that you mentioned creates problems.

One alternative suggested was that a request could just push the claimant even if it was compatible with it. The insight there being that we're locally collapsing the dependency graph on a single range, before engaging distributed deadlock detection.

I don't quite follow. I suspect there is a more rigorous argument here, given that we are already pruning the dependencies (since we don't draw an edge to all the things ahead of a waiter). Perhaps something like: if we prune so that each waiter has exactly one directed edge to something ahead of it, then if the original unpruned graph had a cycle that prevented all progress (all txns were stuck, due to deadlock), then this pruned graph will also have a cycle. That is, we may delay realizing some cycle, but some progress can be made.

Do you have details regarding that insight?