storage/concurrency: changes to lock table to handle requests that

[sumeerbhola]

have the same key as ReadWrite and ReadOnly

• This new functionality is covered by the dup_access test and the
  randomized test.
• The existing basic test lost some coverage of inactive waiters so
  there is now a separate non_active_waiter test.
• The comment on future extensions to shared and upgrade locks now
  includes a discussion on non-transactional requests, and how active
  and inactive wait states will work.
• There was a bug in the code that handles a discovered lock, when
  the lock was discovered by a reader, which was triggered by changes
  to the basic test. The lockTableGuardImpl.mu.locks was being
  incorrectly updated to add the *lockState.

Release note: None

[cockroach-teamcity]

This change is 

[nvanbenschoten]

nice testing.

Reviewed 5 of 5 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @sumeerbhola)

---

pkg/storage/concurrency/lock_table.go, line 771 at r1 (raw file):

		}
		g.mu.Lock()
		_, alsoWriter := g.mu.locks[l]

nit: consider changing this name so that it would also be correct when we switch to more than 2 locking strengths.

---

pkg/storage/concurrency/lock_table.go, line 774 at r1 (raw file):

		g.mu.Unlock()

		// If already have this lock in the locks map it also be writing to

"If already" is missing a word. So is "is also be".

---

pkg/storage/concurrency/lock_table.go, line 1039 at r1 (raw file):

	g.mu.Lock()
	_, presentHere := g.mu.locks[l]
	if !presentHere && sa == spanset.SpanReadWrite {

Could you define a variable as !presentHere && sa == spanset.SpanReadWrite and then check whether that is set instead of using the same condition twice? Something like:

shouldQueue := !presentHere && sa == spanset.SpanReadWrite
if shouldQueue {
    ...
}
g.mu.Unlock()

if shouldQueue {
    ...
}

---

pkg/storage/concurrency/lock_table.go, line 1040 at r1 (raw file):

	_, presentHere := g.mu.locks[l]
	if !presentHere && sa == spanset.SpanReadWrite {
		// Since will place itself in queue as inactive waiter below.

"Singe g will"?

---

pkg/storage/concurrency/lock_table.go, line 1380 at r1 (raw file):

}

var accessDecreasingStrength = [spanset.NumSpanAccess]spanset.SpanAccess{spanset.SpanReadWrite, spanset.SpanReadOnly}

Do we need this, given that the contract of spanset.SpanAccess says Higher-valued accesses imply lower-level ones.? Do you like that it's more explicit than just keeping g.sa but initializing it at spanset.NumSpanAccess and iterating in reverse?

---

pkg/storage/concurrency/testdata/lock_table/dup_access, line 293 at r1 (raw file):

# Test: Non-transactional req8 accesses "b" as both write and read. After is has stopped waiting
# at "b" the reservation for "b" is acquired by a request with a lower seqnum. req8 does not ignore

at "b",

[sumeerbhola]

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

---

pkg/storage/concurrency/lock_table.go, line 771 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: consider changing this name so that it would also be correct when we switch to more than 2 locking strengths.

changed to alsoHasStrongerAccess.

---

pkg/storage/concurrency/lock_table.go, line 774 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"If already" is missing a word. So is "is also be".

Fixed.

---

pkg/storage/concurrency/lock_table.go, line 1039 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Could you define a variable as !presentHere && sa == spanset.SpanReadWrite and then check whether that is set instead of using the same condition twice? Something like:

shouldQueue := !presentHere && sa == spanset.SpanReadWrite
if shouldQueue {
    ...
}
g.mu.Unlock()

if shouldQueue {
    ...
}

Done

---

pkg/storage/concurrency/lock_table.go, line 1040 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"Singe g will"?

Done

---

pkg/storage/concurrency/lock_table.go, line 1380 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we need this, given that the contract of spanset.SpanAccess says Higher-valued accesses imply lower-level ones.? Do you like that it's more explicit than just keeping g.sa but initializing it at spanset.NumSpanAccess and iterating in reverse?

I was guessing that we are sending the SpanAccess values over the wire, and when we add accesses for shared and upgrade locks this monotonicity of both strength and index would no longer be true. Hence made it explicit. What is the plan for adding those accesses?

---

pkg/storage/concurrency/testdata/lock_table/dup_access, line 293 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

at "b",

Done

[nvanbenschoten]

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)

---

pkg/storage/concurrency/lock_table.go, line 1380 at r1 (raw file):

Previously, sumeerbhola wrote…

I was guessing that we are sending the SpanAccess values over the wire, and when we add accesses for shared and upgrade locks this monotonicity of both strength and index would no longer be true. Hence made it explicit. What is the plan for adding those accesses?

The plan is to get rid of spanset. SpanAccess (which are never sent over the wire) entirely and to use lock.Strength values instead. That's just going to require a bit of refactoring, but it's pretty desirable to unify those concepts.

[sumeerbhola]

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

---

pkg/storage/concurrency/lock_table.go, line 1380 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The plan is to get rid of spanset. SpanAccess (which are never sent over the wire) entirely and to use lock.Strength values instead. That's just going to require a bit of refactoring, but it's pretty desirable to unify those concepts.

Ok. Removed accessDecreasingStrength.

[nvanbenschoten]

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

[sumeerbhola]

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

---

pkg/storage/concurrency/lock_table.go, line 984 at r4 (raw file):

				g.doneWaitingAtLock(false, l)
			} else {
				g.mu.Lock()

The randomized test uncovered this bug -- it took me a while to track it down since it manifested much later than its occurrence, and needed "tracing" of the history to figure it out. It was incorrectly notifying on a passive waiter, which meant the guard would move on from a place where it was actively waiting as a reader, without cleaning up its state, which meant when it later waited as a writer at that place it had been actively waiting, it would violate the invariant of expecting a passive queuedGuard.

[nvanbenschoten]

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

---

pkg/storage/concurrency/lock_table.go, line 984 at r4 (raw file):

Previously, sumeerbhola wrote…

The randomized test uncovered this bug -- it took me a while to track it down since it manifested much later than its occurrence, and needed "tracing" of the history to figure it out. It was incorrectly notifying on a passive waiter, which meant the guard would move on from a place where it was actively waiting as a reader, without cleaning up its state, which meant when it later waited as a writer at that place it had been actively waiting, it would violate the invariant of expecting a passive queuedGuard.

Nice find. I'm assuming you confirmed that there aren't other bugs in the same class as this lurking in here.

[sumeerbhola]

bors r+

[sumeerbhola]

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

---

pkg/storage/concurrency/lock_table.go, line 984 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Nice find. I'm assuming you confirmed that there aren't other bugs in the same class as this lurking in here.

Yes -- all the calls to doneWaitingAtLock are now correct, and the randomized test has run for over an hour with no failures (it used to fail after ~5 min before).

[nvanbenschoten]

Bors crashed.

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[nvanbenschoten]

Flaked on #45131.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)