storage/concurrency: define concurrency control interfaces

[nvanbenschoten]

Informs #41720.

The commit creates a new concurrency package that provides a concurrency manager structure that encapsulates the details of concurrency control and contention handling for serializable key-value transactions. Interested readers should start at concurrency_control.go and move out from there.

The new package has a few primary objectives:

1. centralize the handling of request synchronization and transaction contention handling in a single location, allowing for the topic to be documented, understood, and tested in isolation.
2. rework contention handling to react to intent state transitions directly. This simplifies the transaction queueing story, reduces the frequency of transaction push RPCs, and allows waiters to proceed immediately after intent resolution.
3. create a framework that naturally permits "update" locking, which is required for kv-level SELECT FOR UPDATE support (sql: explicit lock syntax (SELECT FOR {SHARE,UPDATE} {skip locked,nowait}) #6583).
4. provide stronger guarantees around fairness when transactions conflict, to reduce tail latencies under contended sceneries.
5. create a structure that can extend to address the long-term goals of a fully centralized lock-table laid out in storage: separated lock-table keyspace #41720.

This commit pulls over a lot of already reviewed code from #43775. The big differences are that it updates the lockTable interface to match the one introduced in #43740 and it addresses the remaining TODOs to document the rest of the concurrency control mechanisms in CockroachDB. At this point, a reader of this file should get a good idea of how KV transactions interact in CRDB... now we just need to make the system actually work this way.

[cockroach-teamcity]

This change is 

[sumeerbhola]

Reviewed 11 of 13 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @nvanbenschoten, @sumeerbhola, and @tbg)

---

pkg/storage/concurrency/concurrency_control.go, line 63 at r3 (raw file):

// that a request that is part of a transaction which has already acquired a
// lock does not need to wait on that lock during sequencing, and can therefore
// ignore any queue that has formed on the lock.

nit: Perhaps add: For other exceptions, see the later comment for lockTable.

---

pkg/storage/concurrency/concurrency_control.go, line 84 at r3 (raw file):

//  |             | [ lockTable ]                                     |  |  |
//  |             | [    key1   ]    -------------+-----------------+ |  ^  |
//  |             | [    key2   ]  /  lock:       | lockWaitQueue:  |--<------<---+

s/lock:/lockState:/

---

pkg/storage/concurrency/concurrency_control.go, line 163 at r3 (raw file):

	// sequencing attempts. If provided, the guard should not be holding latches
	// already. This typically means that the guard was acquired through a call
	// to a contentionHandler method.

I didn't quite understand the last sentence.

---

pkg/storage/concurrency/concurrency_control.go, line 171 at r3 (raw file):

	// directly, in which case it will return a Response for the request. If it
	// does so, it will not return a request guard.
	SequenceReq(context.Context, *Guard, Request) (*Guard, Response, *Error)

A go question: requestSequencer is an unexported interface so does it matter whether this method is named SequenceReq or sequenceReq?

---

pkg/storage/concurrency/concurrency_control.go, line 187 at r3 (raw file):

	// from its lock table. After doing so, it enqueues the request that hit the
	// error in the lock's wait-queue (but does not wait) and releases the
	// guard's latches. It returns an updated guard reflecting this change.

The latching situation is a bit unclear in these interfaces since it happens as a side-effect of method calls. It sounds like both the methods in contentionHandler are called after SequenceReq(), which has acquired latches and both of these release them.
Also, there are many interfaces: is it possible to have a rough usage comment that explains how to use them together.

---

pkg/storage/concurrency/concurrency_control.go, line 204 at r3 (raw file):

	// OnLockAcquired informs the concurrency manager that a transaction has
	// acquired a new lock or updated an existing lock that it already held.
	OnLockAcquired(context.Context, roachpb.Intent)

should this be OnLockUpdate() for consistency with lockTable?

---

pkg/storage/concurrency/concurrency_control.go, line 325 at r3 (raw file):

//  | [ lockTable ]                                     |
//  | [    key1   ]    -------------+-----------------+ |
//  | [    key2   ]  /  lock:       | lockWaitQueue:  | |

s/lock:/lockState:/

---

pkg/storage/concurrency/concurrency_control.go, line 332 at r3 (raw file):

//  +---------------------------------------------------+
//
// The database is read and written using "requests". Transactions are composed

Are there any substantive changes to the long comments related to lockTable -- i.e., should I reread?

---

pkg/storage/concurrency/concurrency_control.go, line 423 at r3 (raw file):

	// For replicated locks, this must be called after the corresponding write
	// intent has been applied to the replicated state machine.
	AcquireLock(*enginepb.TxnMeta, roachpb.Key, lock.Strength, lock.Durability) error

can you add a code comment that *TxnMeta must be the same as the one used in Request

---

pkg/storage/concurrency/concurrency_control.go, line 502 at r3 (raw file):

// lockTableGuardState represents the current state of a request as it waits on
// conflicting locks in the lockTable.
type lockTableGuardState interface {

the actual state is hidden since it is irrelevant to the user of this package?

---

pkg/storage/concurrency/lock_table.go, line 209 at r3 (raw file):

// thread.
//
// Recommended usage:

this usage comment has been omitted when copying -- I assume that is because it has outlived its usefulness since there will soon be code that roughly mirrors it?

---

pkg/storage/concurrency/lock_table.go, line 291 at r3 (raw file):

	g.table.findNextLockAfter(g, false /* notify */)
	g.mu.Lock() // Unlock deferred
	return &g.mu.state

We can't return a pointer to state since it can be modified after this method returns.
I suspect this need to use a pointer was motivated by using a pointer receiver for the marker guardState() method -- is that correct?

---

pkg/storage/concurrency/lock_table.go, line 812 at r3 (raw file):

			return nil, errors.Errorf("caller violated contract")
		}
		if l.holder.holder[durability].txn != nil && l.holder.holder[durability].txn.Epoch < updateTxn.Epoch {

ouch. I really need to fix that test TODO -- the epoch and ignored sequence code paths are completely untested.

---

pkg/storage/concurrency/lock_table.go, line 843 at r3 (raw file):

	// Not already held, so may be reserved by this request.
	if l.reservation != nil {
		if l.reservation.txn.ID != txn.ID {

Hmm. This would have worried me more prior to my other change related to reservation breaking. By not passing a guard to this method it may be the case that the guard holding the reservation is a different one from the same transaction.

---

pkg/storage/concurrency/lock_table.go, line 1414 at r3 (raw file):

	if err != nil {
		return err
	}

I preferred the more defensive nature of the prior code since it processes anything in doneWaiting before returning error. In general, it would be ideal if all functions did all error checking first, before doing any state changes and so an error were a guarantee that nothing had changed but I am not sure this code is always so disciplined.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @sumeerbhola, and @tbg)

---

pkg/storage/concurrency/concurrency_control.go, line 63 at r3 (raw file):

Previously, sumeerbhola wrote…

nit: Perhaps add: For other exceptions, see the later comment for lockTable.

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 84 at r3 (raw file):

Previously, sumeerbhola wrote…

s/lock:/lockState:/

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 163 at r3 (raw file):

Previously, sumeerbhola wrote…

I didn't quite understand the last sentence.

Improved.

---

pkg/storage/concurrency/concurrency_control.go, line 171 at r3 (raw file):

Previously, sumeerbhola wrote…

A go question: requestSequencer is an unexported interface so does it matter whether this method is named SequenceReq or sequenceReq?

requestSequencer is embedded in Manager though, so in this case, it does matter.

For purely unexported interfaces, the only difference is that exporting the methods allows objects from other packages to satisfy the interface. We see this with txnWaitQueue, which is implemented by txnwait.Queue.

---

pkg/storage/concurrency/concurrency_control.go, line 187 at r3 (raw file):

The latching situation is a bit unclear in these interfaces since it happens as a side-effect of method calls. It sounds like both the methods in contentionHandler are called after SequenceReq(), which has acquired latches and both of these release them.

Yes, that's all true. I added to their comments and updated the diagram above.

Also, there are many interfaces: is it possible to have a rough usage comment that explains how to use them together.

These are all exposed as part of the same concurrency.Manager interface. Other packages will only interact with that. I just split up these sub-interfaces to tease apart the different roles that the concurrency.Manager plays.

---

pkg/storage/concurrency/concurrency_control.go, line 204 at r3 (raw file):

Previously, sumeerbhola wrote…

should this be OnLockUpdate() for consistency with lockTable?

Yes! Good catch.

---

pkg/storage/concurrency/concurrency_control.go, line 325 at r3 (raw file):

Previously, sumeerbhola wrote…

s/lock:/lockState:/

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 332 at r3 (raw file):

Previously, sumeerbhola wrote…

Are there any substantive changes to the long comments related to lockTable -- i.e., should I reread?

No, this is mostly a clean copy with just a few edits to make it fit into the rest of this structure.

---

pkg/storage/concurrency/concurrency_control.go, line 423 at r3 (raw file):

Previously, sumeerbhola wrote…

can you add a code comment that *TxnMeta must be the same as the one used in Request

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 502 at r3 (raw file):

Previously, sumeerbhola wrote…

the actual state is hidden since it is irrelevant to the user of this package?

Yes, the state is hidden to anyone other than a lockTableWaiter impl. No one else should need to know about it, but it's useful to include here opaquely for the sake of understanding how it fits in with the rest of the package.

---

pkg/storage/concurrency/lock_table.go, line 209 at r3 (raw file):

Previously, sumeerbhola wrote…

this usage comment has been omitted when copying -- I assume that is because it has outlived its usefulness since there will soon be code that roughly mirrors it?

Yeah, we could have kept it, but the next step here is to implement the lockTableWaiter which will mirror this code closely, so I didn't think we need to keep it going forward.

---

pkg/storage/concurrency/lock_table.go, line 291 at r3 (raw file):

Previously, sumeerbhola wrote…

We can't return a pointer to state since it can be modified after this method returns.
I suspect this need to use a pointer was motivated by using a pointer receiver for the marker guardState() method -- is that correct?

Good point, I guess I'll revert this lockTableGuardState part for now. It wasn't really getting us all that much.

---

pkg/storage/concurrency/lock_table.go, line 843 at r3 (raw file):

Previously, sumeerbhola wrote…

Hmm. This would have worried me more prior to my other change related to reservation breaking. By not passing a guard to this method it may be the case that the guard holding the reservation is a different one from the same transaction.

Discussed in person. This should be ok (pending the change Sumeer's making in #44791) because down below we'll do the same thing for any other requests for the same transaction. Furthermore, we may look to get rid of the deleting from g.mu.locks entirely from this function, as it doesn't seem necessary to eagerly remove txns from queues where they hold the lock.

---

pkg/storage/concurrency/lock_table.go, line 1414 at r3 (raw file):

Previously, sumeerbhola wrote…

I preferred the more defensive nature of the prior code since it processes anything in doneWaiting before returning error. In general, it would be ideal if all functions did all error checking first, before doing any state changes and so an error were a guarantee that nothing had changed but I am not sure this code is always so disciplined.

Reverted in both places.

[sumeerbhola]

seems like the changes outside the concurrency package are cosmetic -- if not, someone else would need to review those.
Rest

How do you want to sequence merging this and #44791

Reviewed 1 of 13 files at r3, 3 of 3 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @nvanbenschoten, @sumeerbhola, and @tbg)

---

pkg/storage/concurrency/concurrency_control.go, line 204 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Yes! Good catch.

the comment is updated but the method is still OnLockReleased()

[nvanbenschoten]

Thanks for the review!

How do you want to sequence merging this and #44791

Let's get that in first.

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

---

pkg/storage/concurrency/concurrency_control.go, line 204 at r3 (raw file):

Previously, sumeerbhola wrote…

the comment is updated but the method is still OnLockReleased()

Done.

---

pkg/storage/spanset/spanset.go, line 140 at r4 (raw file):

		for ss := SpanScope(0); ss < NumSpanScope; ss++ {
			s.spans[sa][ss], _ /* distinct */ = mergeSpans(s.spans[sa][ss])
			// TODO(nvanbenschoten): dedup across accesses.

@sumeerbhola I was thinking about this more and I wonder if we can avoid this. If we iterate over span accesses in decreasing strength order (read-write, then read-only) then we know that if a weaker strength access conflicts with a lock and that lock also overlap with a stronger strength access, then the request must already be waiting on the lock. So can't we easily detect this in the lockTable using the lockTableGuardImpl.mu.locks set to avoid waiting on the same lock twice?

It would be nice not to place extra obscure constraints on the spanset provided to the lockTable/latchManager if we don't need to.

[sumeerbhola]

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

---

pkg/storage/spanset/spanset.go, line 140 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

@sumeerbhola I was thinking about this more and I wonder if we can avoid this. If we iterate over span accesses in decreasing strength order (read-write, then read-only) then we know that if a weaker strength access conflicts with a lock and that lock also overlap with a stronger strength access, then the request must already be waiting on the lock. So can't we easily detect this in the lockTable using the lockTableGuardImpl.mu.locks set to avoid waiting on the same lock twice?

It would be nice not to place extra obscure constraints on the spanset provided to the lockTable/latchManager if we don't need to.

Yes, we can use lockTableGuardImpl.mu.locks to remove this need. A lockState is in that map when (a) request has a reservation or it is in the queued writers (active or inactive), (b) request is in waiting readers, which only happens when actively waiting at this lock. So if a read access finds the lockState in the map when doing the scan it must be also doing a write.

[tbg]

but do consider this a non-expert review.

Reviewed 10 of 13 files at r3, 2 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @nvanbenschoten, and @sumeerbhola)

---

pkg/storage/concurrency/concurrency_control.go, line 171 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

requestSequencer is embedded in Manager though, so in this case, it does matter.

For purely unexported interfaces, the only difference is that exporting the methods allows objects from other packages to satisfy the interface. We see this with txnWaitQueue, which is implemented by txnwait.Queue.

Not sure if we care, but look at the opaque godoc the current code causes:

type Manager interface {
    // contains filtered or unexported methods
}

if you embedded exported interfaces, it would render better. As is, all of the method comments are hidden.

---

pkg/storage/concurrency/concurrency_control.go, line 187 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The latching situation is a bit unclear in these interfaces since it happens as a side-effect of method calls. It sounds like both the methods in contentionHandler are called after SequenceReq(), which has acquired latches and both of these release them.

Yes, that's all true. I added to their comments and updated the diagram above.

Also, there are many interfaces: is it possible to have a rough usage comment that explains how to use them together.

These are all exposed as part of the same concurrency.Manager interface. Other packages will only interact with that. I just split up these sub-interfaces to tease apart the different roles that the concurrency.Manager plays.

The new guard won't hold any latches, right? So the caller will use it to call into SequenceReq? Does this preserve fairness (e.g. by putting the txn at the head of the wait queues for all latches it held)?

---

pkg/storage/concurrency/concurrency_control.go, line 118 at r5 (raw file):

// interface and inner-workings.
//
// At a high-level, a request enter the concurrency manager and immediately

a request -> requests

---

pkg/storage/concurrency/concurrency_control.go, line 199 at r5 (raw file):

	// HandleTransactionPushError consumes a TransactionPushError by informing
	// the concurrency manager about a transaction record that could not be
	// pushed during request evaluation (while holding latches). After doing so,

Is the command running into this necessarily a PushTxn? If not, what's an example that calls into this? Having trouble rationalizing what this is for. (I'm paging this all back in, so I might be too good of an approximation to someone who just walked into this package for the first time)

---

pkg/storage/concurrency/concurrency_control.go, line 215 at r5 (raw file):

	// OnLockUpdated informs the concurrency manager that a transaction has
	// updated or released a lock or range of locks that it previously held.
	OnLockUpdated(context.Context, roachpb.Intent)

The comment on OnLockAcquired also states that it's called when "a txn has ... updated an existing lock it already held". How is this different? Also the word "previously" means "already" here, right (like in the comment above)? Not that it "used to hold this lock but no longer".

---

pkg/storage/concurrency/concurrency_control.go, line 222 at r5 (raw file):

type transactionManager interface {
	// OnTransactionUpdated informs the concurrency manager that a transaction's
	// status was updated by a successful transaction state transition.

what's a "successful transaction state transition"? Or rather, what constitutes an "unsuccessful" one? Maybe just removed everything after "updated".

---

pkg/storage/concurrency/concurrency_control.go, line 386 at r5 (raw file):

	//
	// The first call to ScanAndEnqueue for a given request uses a nil
	// lockTableGuard and the subsequent calls reuse the previously returned

Why are there subsequent calls? Just when running into intents?

---

pkg/storage/concurrency/concurrency_control.go, line 428 at r5 (raw file):

	// lock's timestamp and strength, if necessary.
	//
	// For replicated locks, this must be called after the corresponding write

In steady state operation, we thus expect calls to AddDiscoveredLock to be rare, correct?

---

pkg/storage/concurrency/concurrency_control.go, line 501 at r5 (raw file):

	// not retrieved, since it is not necessary for the waiter to see every
	// state transition.
	NewStateChan() <-chan struct{}

Why New?

Have you considered StateChan() (<-chan struct{}, bool) which subsumes ShouldWait (returning a nil chan if the bool is false)?

---

pkg/storage/concurrency/concurrency_control.go, line 551 at r5 (raw file):

// txnWaitQueue holds a collection of wait-queues for transaction records.
// Conflicting transactions, known as "pushers", sit in a queue associated with
// a extant transaction that they conflict with, known as the "pushee", and wait

an extant

---

pkg/storage/concurrency/concurrency_control.go, line 584 at r5 (raw file):

// the lock acquisition patterns of two or more transactions interact in such a
// way that a cycle emerges in the "waits-for" graph of transactions. To break
// this cycle, one of the transactions much be aborted or it is impossible for

must

---

pkg/storage/concurrency/concurrency_control.go, line 713 at r5 (raw file):

var _ = lockTableWaiter(nil)
var _ = txnWaitQueue(nil)
var _ = Guard{req: Request{}, lag: nil, ltg: nil}

any reason you're spelling out the struct initializers?

---

pkg/storage/concurrency/lock_table.go, line 183 at r5 (raw file):

//   reservations), while the other request keeps waiting. This other request is
//   waiting on its own transaction and it is even possible for it to be the
//   distinguished waiter. We rely on code layers above lockTable to recognize

I can't claim to fully understand what's going on here, but it doesn't sound good.

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @ajwerner, @sumeerbhola, and @tbg)

---

pkg/storage/concurrency/concurrency_control.go, line 171 at r3 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Not sure if we care, but look at the opaque godoc the current code causes:

type Manager interface {
    // contains filtered or unexported methods
}

if you embedded exported interfaces, it would render better. As is, all of the method comments are hidden.

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 187 at r3 (raw file):

The new guard won't hold any latches, right? So the caller will use it to call into SequenceReq?

Yes, exactly.

Does this preserve fairness (e.g. by putting the txn at the head of the wait queues for all latches it held)?

The new guard will still hold its place in lock wait-queues and will preserve its lockTable seqnum, which preserves fairness.

---

pkg/storage/concurrency/concurrency_control.go, line 118 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

a request -> requests

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 199 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Is the command running into this necessarily a PushTxn? If not, what's an example that calls into this? Having trouble rationalizing what this is for. (I'm paging this all back in, so I might be too good of an approximation to someone who just walked into this package for the first time)

Done. Added examples usages of these two methods as well.

---

pkg/storage/concurrency/concurrency_control.go, line 215 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The comment on OnLockAcquired also states that it's called when "a txn has ... updated an existing lock it already held". How is this different? Also the word "previously" means "already" here, right (like in the comment above)? Not that it "used to hold this lock but no longer".

Changed the word "updated" to "re-acquired". The difference here is that the first method is used during writes and the second method is used during intent resolution.

---

pkg/storage/concurrency/concurrency_control.go, line 222 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

what's a "successful transaction state transition"? Or rather, what constitutes an "unsuccessful" one? Maybe just removed everything after "updated".

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 386 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why are there subsequent calls? Just when running into intents?

No, this also gets called after waiting in lock wait-queues. A request is only safe to proceed when it performs an entire scan through the lockTable and only finds conflicting lockStates which are released and which it is already at the head of the queue for.

---

pkg/storage/concurrency/concurrency_control.go, line 428 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

In steady state operation, we thus expect calls to AddDiscoveredLock to be rare, correct?

Yes, as long as we decide to put all replicated locks into the lockTable immediately even while they're still stored outside of it.

---

pkg/storage/concurrency/concurrency_control.go, line 501 at r5 (raw file):
This is meant to be read as "new state" chan, not new "state chan".

Have you considered StateChan() (<-chan struct{}, bool) which subsumes ShouldWait (returning a nil chan if the bool is false)?

That's not a bad idea, although it doesn't really fit with #44885.

---

pkg/storage/concurrency/concurrency_control.go, line 551 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

an extant

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 584 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

must

Done.

---

pkg/storage/concurrency/concurrency_control.go, line 713 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

any reason you're spelling out the struct initializers?

I was getting "unused field" errors without this. This should all be gone soon.

---

pkg/storage/concurrency/lock_table.go, line 183 at r5 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I can't claim to fully understand what's going on here, but it doesn't sound good.

It's actually all stale. Sumeer removed the comment in #44791.

[nvanbenschoten]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)