[DNM] storage/concurrency: introduce concurrency control package, prototype SFU

[nvanbenschoten]

Informs #41720.

The PR 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.

Select For Update Prototype Results

The last commit in the PR contains a hacked version of upgrade locking hooked into the new concurrency package. This allows us to run several experiments to explore its effectiveness in providing its anticipated benefits.

Below is a collection of tests using YCSB. All experiments were run on a 3 VM m5d.4xlarge cluster with local SSDs. The tests used a hacked version of YCSB which contains an additional workload "U", which is similar to workload "A" except it performs UPDATES 100% of the time. A new request distribution called "single" was also added, which causes all requests to operate on a single row, though they are still spread evenly across all 10 column families in that row.

NOTE that these numbers compare master against a completely unoptimized implementation of an in-memory lock table. The implementation uses a single top-level mutex and makes no attempt to avoid memory allocations or optimize for common cases. There is some low hanging fruit here that would improve its performance measurably.

Throughput Under Contention

Improving the throughput of heavily contended workloads is a recurring goal. The combination of the concurrency package and SFU locking (implicit for UPDATE statements) promises to move towards this goal for heavily contended UPDATE workloads because it avoids thrashing and other wasted work.

Here we compare master against this prototype with three different YCSB variants:

- workload U, single distribution,  64 threads
- workload U, zipfian distribution, 64 threads
- workload A, zipfian distribution, 64 threads

Each variant was tested with each binary over a collection of 3 two-minute trials and the throughput recorded in each trial was averaged together.

We see in the U / single / 64 workload that SFU significantly improves the throughput of highly contended workloads, in this case by 49%. This is likely because of more efficient queuing and fewer transaction retries. The improvement goes away with less contention and only writes (U / zipf / 64), but comes back to some degree (5%) when reads are added into the mix (A / zipf / 64). My best theory for why this is is that upgrade locking improves fairness between readers and writers (see below), which helps prevent starvation of writers.

We'd also expect this to have a larger impact on bigger machines that can tolerate more contention in general.

Fairness Under Contention (i.e. Tail Latencies)

An issue identified early in the SFU work was that the current state of contention handling in Cockroach has serious fairness issues. There is very little queueing so transactions typically race to lay down intents as they repeatedly hit WriteTooOld errors or run into each other's intents. The concurrency package was meant to improve this situation, and SFU improves on it further by avoiding thrashing between reading and writing during an UPDATE operation.

We can see the effect of this by comparing the tail latencies when running YCSB against master and this prototype:

In this comparison, we see that tail latencies currently explode as contention increases (U / single / 64 is the most contended variant). However, the use of SFU avoids this explosion in tail latencies, maintaining a much tighter spread in transaction latencies across the workload variants. This is a clear demonstration of the effectiveness of this new approach.

Transaction Retries

The third goal of the SELECT FOR UPDATE work is to provide users with the ability to avoid transaction retries in certain scenarios where they're performing a read and then an update to the same row. This is what UPDATE statements do internally, and it turns out that today, UPDATE statements on their own can cause transaction retries (see the "Transaction retries in TPC-C" Google group thread) if they read a value that changes before they write to it. We can again explore this situation using YCSB.

In this experiment, we ran YCSB-A (A / zipf / 64) without any modifications and monitored the number of transaction retries. Because YCSB runs single-statement transactions, all of these would be retries on the SQL gateway, but these statements could just as easily be run in explicit transactions, in which case these retries would make it back to the client. For practical reasons, I started running with the new SFU prototype and then switched back to master half-way through the test (around 5:25). Take note of the KV Transaction Restart graph.

We see essentially no transaction restarts when using the SFU-enhanced prototype. However, when we switch back to master, we see restarts jump up to around 140 per second across three classes of retry reasons. This confirms that in cases like these, SELECT FOR UPDATE will be an effective tool in reducing or eliminating transaction retries.

[cockroach-teamcity]

This change is 

[petermattis]

Exciting improvements here. How stable are these results? I ask because in recent experimentation with ycsb/A on Pebble, I noticed ± 10% throughput variation when running on master/RocksDB. I ended up needing to run each test 10-20 times, format the results in a manner compatible with benchstat and then use benchstat to tease out the delta. ycsb/A seemed more prone to this variation than some the kv0 and kv95 tests, presumably because of the contention (though I didn't verify that).

[nvanbenschoten]

How stable are these results?

The throughput numbers across the trials were varying by about ± 2-3%. The latency percentiles were more stable across trials, which looks to be because they're quantized. I'm actually hopeful that the fairness improvements here will help reduce variance and make YCSB more stable.

[tbg]

Only on success, right? If the request tries to write an intent but this somehow fails below Raft, the lock should go away? Or I guess it's fine to have an in-mem lock too many, then we'll just push more than we need to (hope that doesn't confuse deadlock detection somehow). Hmm, SFU will basically mean having in-mem locks that aren't backed by replicated locks, so it will all just have to work anyway.

[nvanbenschoten]

Only on success, right? If the request tries to write an intent but this somehow fails below Raft, the lock should go away?

Yes, documented.

Hmm, SFU will basically mean having in-mem locks that aren't backed by replicated locks, so it will all just have to work anyway.

Yeah, we'll have to make it work. For now, though, I'm not going to worry about inserting locks on failure paths.

[tbg]

When these methods return a new *Guard, does it mean that the original *Guard must no longer be used?

[nvanbenschoten]

Done.

[tbg]

pushed

[nvanbenschoten]

Done.

[tbg]

off

[nvanbenschoten]

Done.

[tbg]

Is this called if a waiter gives up? In normal operation we'll only ever dequeue in FIFO order, right?

[nvanbenschoten]

Yes, this can be called if a waiter gives up, so we'll need to handle out of order dequeue-ing. Under normal operation though, you're correct. Commented.

[tbg]

Curious why this is needed.

[nvanbenschoten]

It's not needed right now, but it will be needed once we pull the replicated portion of the lockTable under here. I already had the wiring hooked up to the rest of the Storage package, which I think is valuable. I'll remove this for now though.

[tbg]

Could this be wqgs (which I know isn't in scope here)? Just trying to confirm my understanding that at the beginning of each iter in the for loop g.wqgs has been WaitOn'ed already and is only carried with the guard for .. other reasons.

[nvanbenschoten]

Originally that would have worked, but not anymore because in #43740 introduced the notion of "breaking" reservations. Essentially this means that waiters can lose their place at the front of a wait-queue while not holding latches. It is only safe to conclude that a waiter is at the front of all wait-queues if it is holding latches.

The reason for this is pretty subtle, but @sumeerbhola does a good job documenting it in that PR. Because we don't hold range locks while waiting in per-key wait-queues, we run into situations like the following. It's possible that two ranged writes txn1 and txn2 over keys [a, d) might sequence such that req txn1 initially acquires latches first. It finds a lock on key c and begins waiting. A different txn3 might then get sequenced and perform a point write on key a. When txn2 is sequenced it will enter the wait-queue for a (@ front) and c (behind txn1). When txn1 re-acquires latches and re-scans the lockTable, it will find the lock on a and enter its wait-queue. We don't want it getting behind txn2 in the queue or we'd cause a deadlock.

So, the solution Sumeer came up with to address this was to give each request a monotonically increasing sequence number when it first scans the lockTable. This creates a total order over requests. The order does not tell us exactly what order requests will finish being sequenced by the concurrency manager. However, it does tell us how to order conflicting transactions in a given wait-queue. In other words, the policy for entering a wait-queue is not that requests automatically get in the back. Instead, requests scan from the back to find where they fit in the sequence-order list of reqs. This avoids the kinds of deadlocks discussed above.

However, to get it to work, we need to abandon the idea that once a request is at the front of a wait-queue, it stays there. Instead, requests much check all of their wait-queue entries on each iteration through the loop while holding latches. A request is only safe to proceed when it observes that it is at the front of all of its wait-queues during a single pass.

The correctness of this is subtle but I think it's mentioned in #43740. A consequence of it is that as a req with a low seq num, we need to be very careful about never breaking the reservation of another request whose latch set might be compatible with its, because latching is what's providing the mutual exclusion to atomically determine that a request holds a reservation for (is at the front of) all wait-queues that it's a part of. The fact that non-locking (lock.None) reads are never going to hold reservations in wait-queues makes this easier to think through.

[tbg]

Rather than trying to keep these "internal" request types side-by-side with the "public" KV API (Scan, Put, ...) we should establish a boundary between them. I feel that the flags have not been helping that, and they also tend to obfuscate understanding.

[nvanbenschoten]

I've also grown a little disillusioned about flags for something like this because they're defined so far away from where they're used. I'm curious what you're thinking in terms of a boundary.

[tbg]

I would've expected to see ResolveIntent{,Range} here.

[nvanbenschoten]

Somewhat surprisingly, ResolveIntent{,Range} is not roachpb.IsTransactional. Neither of the requests have the isTxn flag.

[tbg]

This is the case in which I find out about a lock only from the replicated key space, right? And there's the other case in which we discover locks directly from the lock table. I would've expected that we'd wait in SequenceReq in all cases, i.e. HandleWriterIntentError would be called and after the caller would pass the resulting guard to SequenceReq. (This would also explain why my assumption in that method about only having to wait on new wgqs is wrong).

[nvanbenschoten]

Yes, that's exactly how things are supposed to work. I think I just forgot to address this TODO and delete the code. Done.