kv: use LockSpanSets to declare lock spans

[arulajmani]

Prior to this patch, requests would use the same mechanism
(spanset.SpanSets) to declare both latch and lock spans.
This patch changes the latter to use lockspanset.LockSpanSets
instead. This allows us to declare lock spans for a request with its
intended lock strengths in the future. This will in-turn enable us to
support multiple locking strengths in the concurrency package.

This patch does not change any functionality. To that end, requests
that would have previously declared access for spanset.SpanReadOnly
use lock.None; requests that would have previously declared
spanset.SpanReadWrite use lock.Intent. The concurrency package
validates lock spans with no other lock strength are supplied to it.
It does so both in the lock table waiter and when scanning the lock
table. We will relax these assertions as we build towards shared locks.

One thing to note is that I decided not to change the lock table tests
in this patch, to reduce test churn. We do so by translating lock
strength back to SpanAccess when printing out the guard state. This
will get fixed in a followup, along with the datadriven input.

Informs #102008

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

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

---

pkg/kv/kvserver/concurrency/concurrency_control.go line 423 at r2 (raw file):

	// The maximal set of spans within which the request expects to have
	// isolation from conflicting transactions. Conflicting locks within

We should mention strength somewhere in this comment.

---

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

			findDistinguished = false
		}
		state.guardStrength = g.str

Does it make sense to move the assignment of guardStregth into lockTableGuardImpl.updateStateLocked?

---

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

		// NB: If we were not defer to the stronger lock strength and start waiting
		// here, we would end up doing so in the wrong wait queue (queuedReaders vs.
		// queuedWriters.

nit: missing closing parenthesis.

---

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

		// reservations, we can not detect a key is being accessed with both None
		// and Intent locking strengths, like we can for transactional requests. In
		// some rare cases, the is now held at a timestamp that is not compatible

"the is now held"

---

pkg/kv/kvserver/concurrency/lock/locking.go line 57 at r1 (raw file):

// NumLockStrength is the total number of lock strengths in the Strength enum.
const NumLockStrength = MaxStrength + 1

Consider adding an init-time validation for this constant. E.g. that NumLockStrength == len(Strength_name).

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 79 at r1 (raw file):

func (l *LockSpanSet) String() string {
	var buf strings.Builder
	for st := range l.spans {

for st, spans := range l.spans {
    for _, span := range spans {

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 113 at r2 (raw file):

		return
	}
	copy(l.spans[str], existing)

Are we missing the l.spans[str] = make([]roachpb.Span, len(existing), n+len(existing)) here?

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 119 at r2 (raw file):

// invalid.
func (l *LockSpanSet) Validate() error {
	for st := range l.spans {

Same comment about for st, spans := range l.spans that was mentioned in the prior patch.

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 120 at r2 (raw file):

func (l *LockSpanSet) Validate() error {
	for st := range l.spans {
		for _, sp := range l.spans[st] {

nit: s/sp/span/, for consistency with the rest of this file.

[arulajmani]

Addressed everything except for the comment about updateStateLocked. Let me know how you feel about the proposal there.

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

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does it make sense to move the assignment of guardStregth into lockTableGuardImpl.updateStateLocked?

I thought about this a bit, and I don't think it makes sense in the current structure because of this call:

cockroach/pkg/kv/kvserver/concurrency/lock_table.go

Line 719 in 2924cc5

g.updateStateLocked(waitingState{kind: doneWaiting})

It's not a incorrect to do so, but it feels slightly weird that we're setting guardStrength to the value of the last lock we scanned if no conflicts were found.

How do you feel about pulling that call and the following piece of code:

cockroach/pkg/kv/kvserver/concurrency/lock_table.go

Lines 728 to 733 in 2924cc5

	if len(g.toResolve) > 0 {
	// Force caller to release latches and resolve intents. The first
	// state it will see after releasing latches is doneWaiting, which
	// will cause it to resolve intents.
	g.mu.startWait = true
	}

into it's own function (say: updateStateToDoneWaiting). This is fine, because startWait=true does not need to be conditional on the value of notify.

If we do that, then we could move guardStrength into updateStateLocked. Maybe even rename that function to updateStateToWaiting and ensure it's never called with wait kind of doneWaiting.

How do you feel about all this?

---

pkg/kv/kvserver/concurrency/lock/locking.go line 57 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider adding an init-time validation for this constant. E.g. that NumLockStrength == len(Strength_name).

Done.

---

pkg/kv/kvserver/lockspanset/lockspanset.go line 113 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Are we missing the l.spans[str] = make([]roachpb.Span, len(existing), n+len(existing)) here?

Yeah, thanks for catching.

[nvanbenschoten]

Reviewed 50 of 50 files at r3, 48 of 49 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

---

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

Previously, arulajmani (Arul Ajmani) wrote…

I thought about this a bit, and I don't think it makes sense in the current structure because of this call:

cockroach/pkg/kv/kvserver/concurrency/lock_table.go

Line 719 in 2924cc5

g.updateStateLocked(waitingState{kind: doneWaiting})

It's not a incorrect to do so, but it feels slightly weird that we're setting guardStrength to the value of the last lock we scanned if no conflicts were found.

How do you feel about pulling that call and the following piece of code:

cockroach/pkg/kv/kvserver/concurrency/lock_table.go

Lines 728 to 733 in 2924cc5

	if len(g.toResolve) > 0 {
	// Force caller to release latches and resolve intents. The first
	// state it will see after releasing latches is doneWaiting, which
	// will cause it to resolve intents.
	g.mu.startWait = true
	}

into it's own function (say: updateStateToDoneWaiting). This is fine, because startWait=true does not need to be conditional on the value of notify.

If we do that, then we could move guardStrength into updateStateLocked. Maybe even rename that function to updateStateToWaiting and ensure it's never called with wait kind of doneWaiting.

How do you feel about all this?

All of what you said makes sense to me. It feels wrong to set the guardStrength for kind: doneWaiting.

However, splitting theupdateStateLocked may prove to be difficult for callers like tryClearLock. We could try it. If it does not work well, we could set guardStrength conditionally in updateStateLocked, based on waitingState.kind.

[arulajmani]

RFAL.

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 1288 at r2 (raw file):
Let me know how you feel about the changes to tryClearLock. I like where we ended up, but in case you're not a fan, the change is in its own commit so should be easy enough to reverse.

This is fine, because startWait=true does not need to be conditional on the value of notify.

After coming back to this, I ended up preferring a different approach. See the last commit.

[nvanbenschoten]

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 548 at r5 (raw file):

		panic(errors.AssertionFailedf("unexpected waiting state kind: %d", newState.kind))
	}
	newState.guardStrength = g.str // copy over the strength which cased the conflict

"caused"

---

pkg/kv/kvserver/concurrency/lock_table.go line 2117 at r5 (raw file):

	lockHolderTxn, _ := l.getLockHolder() // only needed if this is a replicated lock
	replicatedHeld := l.holder.locked && l.holder.holder[lock.Replicated].txn != nil
	transitionWaiter := func(g *lockTableGuardImpl) {

Does this closure introduce any new heap allocations?

[arulajmani]

TFTR!

bors r=nvanbenschoten

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

---

pkg/kv/kvserver/concurrency/lock_table.go line 2117 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this closure introduce any new heap allocations?

I tested by wrapping a call to tryClearLocks in a testing.AllocsPerRun and it confirmed that this function doesn't perform any allocations, so we should be good. Ideally, we would have been able to test using TestGCAssert, but that seems broken from a bit of slack searching.

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)