concurrency: allow multiple txns to acquire shared locks on a single key

[arulajmani]

See individual commits for details.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

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

---

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

					pluralSuffix = "s"
				}
				sb.Printf("  holder%s: txn: %v epoch: %d, iso: %s, ts: %v, info: ",

Do you think it would be worthwhile to eliminate the redundancy in formatting each lock? It would be nice not to have to change the formatting in two places. Maybe something like:

var prefix redact.SafeString
if first {
    first = false
    if kl.holders.Len() > 1 {
        prefix = "  holders: "
    } else {
        prefix = "  holder: "
    }
} else {
    prefix = "\n           "
}
sb.Printf("%stxn: %v epoch: %d, iso: %s, ts: %v, info: ", prefix, ...)

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 27 at r3 (raw file):

# ------------------------------------------------------------------------------
# Ensure conflict resolution semantics for shared locks are sane -- that is,
# if a shared lock is held on a key, {shared, non} locking requests are allowed

s/non/none/

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 38 at r3 (raw file):

start-waiting: false

new-request r=req3 txn=txn2 ts=10 spans=shared@a

I confused myself for a moment and thought this wasn't actually performing any requests. But it is, this is the TestLockTableBasic, not TestConcurrencyManagerBasic. Do we want to add a concurrency manager-level test for shared locks as well at some point? The will be especially helpful to test which requests push which other requests when shared locks are involved.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 62 at r3 (raw file):

  holder: txn: 00000000-0000-0000-0000-000000000001 epoch: 0, iso: Serializable, ts: 10.000000000,0, info: unrepl [(str: Shared seq: 0)]
   queued writers:
    active: false req: 2, txn: 00000000-0000-0000-0000-000000000002

This tripped me up. "req: 2" is actually referring to req3 and "req: 3" is actually referring to req4. Do you mind adding on a commit to scan r=req1 up above so that it gets sequenced in the lock table and corrects the indexing?

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 72 at r3 (raw file):

           txn: 00000000-0000-0000-0000-000000000002 epoch: 0, iso: Serializable, ts: 10.000000000,0, info: unrepl [(str: Shared seq: 0)]

# Re-acquisition of the shared lock by req4 should work as well.

nit: remind the reader that req4 is part of the same txn as req3. They're both part of txn2.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 126 at r3 (raw file):

           txn: 00000000-0000-0000-0000-000000000002 epoch: 0, iso: Serializable, ts: 10.000000000,0, info: unrepl [(str: Shared seq: 0)]
   queued writers:
    active: true req: 4, txn: 00000000-0000-0000-0000-000000000002

Should we include the queuedGuard strength in this output?

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 154 at r3 (raw file):

strength until

nit: double space between words.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 154 at r3 (raw file):

# ------------------------------------------------------------------------------
# Ensure a key is locked with SHARED locking strength  until the all

Should we add a test for the opposite? That if an exclusive lock is released with multiple shared locks behind it, all of them are allowed to grab claims? Or does that case not work yet? I think actually not, based on maybeReleaseFirstTransactionalWriter.

Do you have a list of the state transitions that we don't yet support? We wouldn't want to forget one. If not, we could add a TODO at the bottom of this file that enumerates each case that we need to add support for.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 252 at r3 (raw file):

# Now that all shared locks have been released, the exclusive locking request is
# free to proceed.

👏

[arulajmani]

TFTR, this should be good for another look.

I added 2 more commits -- one to sequence req1 like you suggested, and another to print a queuedWriter's strength as well. The latter churned a lot of tests, so apologies for that.

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

---

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

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do you think it would be worthwhile to eliminate the redundancy in formatting each lock? It would be nice not to have to change the formatting in two places. Maybe something like:

var prefix redact.SafeString
if first {
    first = false
    if kl.holders.Len() > 1 {
        prefix = "  holders: "
    } else {
        prefix = "  holder: "
    }
} else {
    prefix = "\n           "
}
sb.Printf("%stxn: %v epoch: %d, iso: %s, ts: %v, info: ", prefix, ...)

Good call, thanks for the suggestion! I ended up keeping the optPlural stuff around to save an else.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 38 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I confused myself for a moment and thought this wasn't actually performing any requests. But it is, this is the TestLockTableBasic, not TestConcurrencyManagerBasic. Do we want to add a concurrency manager-level test for shared locks as well at some point? The will be especially helpful to test which requests push which other requests when shared locks are involved.

I was planning on adding concurrency manager-level tests once we've addressed #102264. That's because TestConcurrencyManagerBasic uses the actual latch manager.

The will be especially helpful to test which requests push which other requests when shared locks are involved.

I agree!

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 62 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

This tripped me up. "req: 2" is actually referring to req3 and "req: 3" is actually referring to req4. Do you mind adding on a commit to scan r=req1 up above so that it gets sequenced in the lock table and corrects the indexing?

Added a commit to fix the indexing.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 126 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we include the queuedGuard strength in this output?

We should. I added a commit that does that.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 154 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

strength until

nit: double space between words.

🦅 👀

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 154 at r3 (raw file):

That if an exclusive lock is released with multiple shared locks behind it, all of them are allowed to grab claims? Or does that case not work yet? I think actually not, based on maybeReleaseFirstTransactionalWriter.

Yeah, it's not. The plan is to address that case as part of #102272.

Do you have a list of the state transitions that we don't yet support? We wouldn't want to forget one. If not, we could add a TODO at the bottom of this file that enumerates each case that we need to add support for.

I added a few off the top of my head, but maybe let's brainstorm these together later today and add to this TODO.

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 252 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

👏

It's getting there!!

[nvanbenschoten]

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

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 275 at r7 (raw file):

# Once the exclusive lock is released, only the head of the queue that is
# compatible (the first three shared lock requests) should be able to acquire
# a joint claim.

Some other cases off the top of my head:

• lock holder: none, wait queue: (shared, shared), some other shared lock breaks reservations and enters the front of the queue
• lock holder: none, wait queue: (shared, shared), some other shared lock breaks reservations and enters the middle of the queue
• lock holder: none, wait queue: (shared, shared), some other exclusive lock breaks reservations and enters the front of the queue
• lock holder: none, wait queue: (shared, shared), some other exclusive lock breaks reservations and enters the middle of the queue
• lock holder: none, wait queue: (exclusive, shared, shared), exclusive acquires lock and exits queue
• lock holder: none, wait queue: (exclusive, shared, shared), exclusive exits queue without acquiring lock
• various lock promotion cases where the lock is acquired/held in shared and is being acquired by the same txn in exclusive further back in the queue
• various lock promotion cases where the lock is acquired/held in exclusive and is being acquired by the same txn in shared further back in the queue
• various cases with non-transactional (intent strength) waiters

Some of these might already be supported.

[arulajmani]

Thanks!

bors r=nvanbenschoten

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

[arulajmani]

bors r-

[craig]

Canceled.

[arulajmani]

I missed the comment about other test cases. I'll bors again once I've added the one's that haven't been already tested in the TODO.

[arulajmani]

TFTR!

bors r=nvanbenschoten

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

---

pkg/kv/kvserver/concurrency/testdata/lock_table/shared_locks line 275 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Some other cases off the top of my head:

• lock holder: none, wait queue: (shared, shared), some other shared lock breaks reservations and enters the front of the queue
• lock holder: none, wait queue: (shared, shared), some other shared lock breaks reservations and enters the middle of the queue
• lock holder: none, wait queue: (shared, shared), some other exclusive lock breaks reservations and enters the front of the queue
• lock holder: none, wait queue: (shared, shared), some other exclusive lock breaks reservations and enters the middle of the queue
• lock holder: none, wait queue: (exclusive, shared, shared), exclusive acquires lock and exits queue
• lock holder: none, wait queue: (exclusive, shared, shared), exclusive exits queue without acquiring lock
• various lock promotion cases where the lock is acquired/held in shared and is being acquired by the same txn in exclusive further back in the queue
• various lock promotion cases where the lock is acquired/held in exclusive and is being acquired by the same txn in shared further back in the queue
• various cases with non-transactional (intent strength) waiters

Some of these might already be supported.

These are all great! I added all of these and added to some of them. B

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)