rfc: kv lock observability via `crdb_locks` by AlexTalks · Pull Request #75541 · cockroachdb/cockroach

AlexTalks · 2022-01-26T07:48:53Z

This RFC proposes a feature for observing locks (and contention on
locks) via a new virtual table called crdb_internal.crdb_locks.

Release note: None

cockroach-teamcity · 2022-01-26T07:49:00Z

This change is

AlexTalks · 2022-01-26T07:49:53Z

Formatted view: https://github.com/AlexTalks/cockroach/blob/rfc_crdb_locks/docs/RFCS/20220104_crdb_locks.md

AlexTalks

Reviewable status: complete! 0 of 0 LGTMs obtained

docs/RFCS/20220104_crdb_locks.md, line 137 at r2 (raw file):

short-lived and only held as long as a single request, rather than for the life 
of a transaction, and thus will be less useful in visualizing contention.
4. **We do not track the queries or statements that obtain locks.**  This is a

Tagging @cockroachdb/sql-observability as I want to make sure this assumption is valid given current capabilities. IIUC, we don't expose the statements in a given transaction currently, but given that the crdb_locks table is being added, perhaps this is something that could be useful going forward, in order to visualize the statement that caused a transaction to obtain a given lock?

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks and @maryliag)

docs/RFCS/20220104_crdb_locks.md, line 137 at r2 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

Tagging @cockroachdb/sql-observability as I want to make sure this assumption is valid given current capabilities. IIUC, we don't expose the statements in a given transaction currently, but given that the crdb_locks table is being added, perhaps this is something that could be useful going forward, in order to visualize the statement that caused a transaction to obtain a given lock?

Your assumption is correct. Currently we don't track which statement caused a lock.

erikgrinaker · 2022-02-02T13:54:53Z

Great to see this!

Just a drive-by comment: I've been wanting a tree view of contention, since we often have one long-running txn that's blocking a bunch of others, and those in turn are blocking others again, etc. Some way to easily visualize the entire dependency chain and identify the root transaction causing the pileup seems like it'd be really useful.

I think that should be doable with this proposal using a recursive CTE? The only issues I see is that there could be holes in the chain -- both due to the fact that we'll have an inconsistent view of the contention (since we're independently querying the in-memory state of several nodes at different times), and possibly because we perhaps aren't tracking the entire set of involved transactions in the lock table. Thoughts on this?

Azhng

Nice!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, and @maryliag)

docs/RFCS/20220104_crdb_locks.md, line 50 at r3 (raw file):

| Term                     | Explanation |
| ------------------------ | ----------- |
| Sequencing               | Processing a request through Currency Control.       |

nit: s/Currency/Concurrency/

docs/RFCS/20220104_crdb_locks.md, line 58 at r3 (raw file):

| Replicated Lock          | A durable lock represented in persistent storage by what is commonly known as an “intent” (`TxnMeta`).  This type of lock may or may not be tracked in the in-memory lock table - as mentioned above, a replicated lock is tracked in the lock table if there are wait queues made up of other transactions waiting to acquire this lock.         |
| Unreplicated Lock        | A lock that is only represented in the lock table, and will not persist in case of node failure or lease transfer.  Used in `[Get/Scan]ForUpdate`.        |
| Replicated Lock Keyspace | The separate keyspace in which replicated locks, commonly known as “separated intents”, are stored; in the Local keyspace of a range.  This is entirely separate from the Lock Table.  Example intent key in the replicated lock keyspace: `/Local/Lock/Intent/Table/56/1/1169/5/3054/0`.        |

Is it correct to understand that even thought it's called a "Local" keyspace, the lock is still replicated ?

docs/RFCS/20220104_crdb_locks.md, line 141 at r3 (raw file):

since we do not keep track of the statement history in a given transaction

Are we talking about the current active transaction? Or historical transaction? If it's latter, then we do track list of statements in that transaction.

For former, currently there's no interface to query that interface, but we do have that information stored somewhere (e.g. in connExecutor we keep track of list of statement fingerprint ID of an active txn so far) and it can be possibly exposed via a SQL interface.

docs/RFCS/20220104_crdb_locks.md, line 237 at r3 (raw file):

	repeated LockWaiter lock_waiters = 7;
}

small style nits: seems like tabs and spaces are mixed up in here

Azhng · 2022-02-02T22:04:48Z

Another quick question just for my own understanding. When the lease moves from one node to another, what happens to the in-memory lockTable? Does it get serialized and sent to the new leaseholder?

AlexTalks

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, and @maryliag)

docs/RFCS/20220104_crdb_locks.md, line 50 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

nit: s/Currency/Concurrency/

HA, good catch!

docs/RFCS/20220104_crdb_locks.md, line 58 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Is it correct to understand that even thought it's called a "Local" keyspace, the lock is still replicated ?

That is correct. "Local" in this context means that this key is only addressable within the context of this (replicated) range.

docs/RFCS/20220104_crdb_locks.md, line 141 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

since we do not keep track of the statement history in a given transaction

Are we talking about the current active transaction? Or historical transaction? If it's latter, then we do track list of statements in that transaction.

For former, currently there's no interface to query that interface, but we do have that information stored somewhere (e.g. in connExecutor we keep track of list of statement fingerprint ID of an active txn so far) and it can be possibly exposed via a SQL interface.

I was speaking about the statement history of the current active transaction - can clarify that.

As for the ability to possibly expose it, we should schedule some time soon to discuss this as it would be a great enhancement.

docs/RFCS/20220104_crdb_locks.md, line 237 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

small style nits: seems like tabs and spaces are mixed up in here

thanks!

AlexTalks · 2022-02-03T02:49:37Z

Replying to @Azhng:

Another quick question just for my own understanding. When the lease moves from one node to another, what happens to the in-memory lockTable? Does it get serialized and sent to the new leaseholder?

tl;dr it does not get serialized and sent to the leaseholder, rather things eventually get redirected and queues re-form.

Longer explanation: It does not get serialized and sent to the new leaseholder - rather, in the case of a lease transfer, all locks are cleared from the in-memory lock table and the lock table is disabled. On clearing the locks, the waiters are (in order) notified that they no longer need to wait and can continue - however, as they attempt to proceed to command evaluation, any conflicting reads/writes will end up encountering the replicated intent, thus resulting in a WriteIntentError which means the request needs to be re-sequenced through Concurrency Control. After re-sequencing the request (which will trivially pass without waiting, since the lock table is disabled), before attempting to evaluate the lease is re-checked, and upon discovering that this replica is no longer the leaseholder, the request should thus be redirected to the proper leaseholder replica for the range. When this happens, the transaction will re-enter the wait queue if necessary.

One illustrative example exists in the range_state_listener test.

Lastly, as this explanation may already imply, this means that unreplicated locks - that is, those used by [Get|Scan]ForUpdate, or SELECT...FOR UPDATE - are dropped. This is by design, as they are intended to be best-effort locks.

AlexTalks · 2022-02-03T18:29:38Z

In response to @erikgrinaker:

I think that should be doable with this proposal using a recursive CTE? The only issues I see is that there could be holes in the chain -- both due to the fact that we'll have an inconsistent view of the contention (since we're independently querying the in-memory state of several nodes at different times), and possibly because we perhaps aren't tracking the entire set of involved transactions in the lock table. Thoughts on this?

I think that the issue about the potential for inconsistency is unavoidable, as I don't think we'd want to block anything to render this table. As for tracking the entire set of involved transactions, I think any that any waiters on a blocking transaction should have that blocking transaction as the lock holder for that key - so if txn1 holds key A but waits on key B, and txn2 holds key B but waits on key C (held by txn3), we should be able to visualize the full dependency chain through the lockholders txn1 -> txn2 -> txn3 (if this sounds incorrect, please let me know!)

AlexTalks

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

docs/RFCS/20220104_crdb_locks.md, line 237 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

thanks!

OK

nvb

nice job on this Alex!

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @Azhng, @maryliag, and @nvanbenschoten)

docs/RFCS/20220104_crdb_locks.md, line 123 at r4 (raw file):

be acceptable given the use case of this feature.

1. **Inability to display replicated locks without waiters**.  Replicated locks

This is framed as a limitation, but that's quite right. This is an intentional design decision. We could scan the persistent lock table and include locks all locks in the response to a QueryLocksRequest if we wanted to. In fact, we very well may want to include that as an option on the request. But for the same reason as in PG, we've deemed the IO cost of doing so to be too expensive for the crdb_internal.crdb_locks table.

docs/RFCS/20220104_crdb_locks.md, line 128 at r4 (raw file):

storage engine.  Given that this feature is intended to visualize contention, 
this may be of minimal concern.
2. **Only read-write transactional lockholders can be displayed.**  While

Isn't this also a design decision? We could return the requests in a lockWaitQueue.waitingReaders set if we wanted to. We decided not to do this for pg_locks because it would be confusing to see non-locking operations in that table, given its contract and based on how concurrency control works in Postgres. But are we sure we want to make that decision for crdb_locks, where we have more flexibility to write the script? If read-only requests still block on locks, should they be represented somehow in crdb_locks?

docs/RFCS/20220104_crdb_locks.md, line 170 at r4 (raw file):

CREATE TABLE crdb_internal.crdb_locks (
  range_id INT,           -- the ID of the range that contains the lock
  table_id INT,           -- the id of the table to which the range with this lock belongs

Should we include database_id and index_id in this table?

docs/RFCS/20220104_crdb_locks.md, line 210 at r4 (raw file):

nit: "```proto"

docs/RFCS/20220104_crdb_locks.md, line 220 at r4 (raw file):

  bool active_waiter = 2;
  SpanAccess access = 3; // [ReadOnly, ReadWrite]
  uint64 seq_num = 4;

I don't think we should include seq_num in this struct. It's an implementation detail of the lockTable data structure that does not make sense to expose.

docs/RFCS/20220104_crdb_locks.md, line 232 at r4 (raw file):

  SpanScope span_scope = 2; // [GLOBAL, LOCAL]
  Strength lock_strength = 3;
  Durability = 4;

Durability durability = 4;

docs/RFCS/20220104_crdb_locks.md, line 236 at r4 (raw file):

  TxnMeta lockholder_txn_meta = 6;

  repeated LockWaiter lock_waiters = 7;

Is this list in a specific order?

docs/RFCS/20220104_crdb_locks.md, line 435 at r4 (raw file):

| **For Users/DB Admins** | Contention Events (via SQL, Dashboards) | `crdb_locks` (via SQL) |

## Examples

These are cool to see written out!

docs/RFCS/20220104_crdb_locks.md, line 518 at r4 (raw file):

# Open Questions

* Special considerations for tenant SQL pods in Serverless

What is the question here? Do we expect things to behave differently in serverless?

erikgrinaker · 2022-02-03T19:30:57Z

I think that the issue about the potential for inconsistency is unavoidable, as I don't think we'd want to block anything to render this table.

I think that's true. We may want to mention this as a limitation in the RFC and any documentation, but I don't think it's likely to reduce its usefulness -- more of a potential gotcha.

As for tracking the entire set of involved transactions, I think any that any waiters on a blocking transaction should have that blocking transaction as the lock holder for that key - so if txn1 holds key A but waits on key B, and txn2 holds key B but waits on key C (held by txn3), we should be able to visualize the full dependency chain through the lockholders txn1 -> txn2 -> txn3 (if this sounds incorrect, please let me know!)

I seem to recall something about the lock table having a bounded size and evicting certain locks when it's full, but I don't remember the details. It's possible (probable even) that we'll pull contended locks into memory even if the lock table is otherwise full. Just worth pointing it out if there is a limitation here.

Thanks again working on this -- RFC looks great, and I'm thrilled to see progress on this.

AlexTalks

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

docs/RFCS/20220104_crdb_locks.md, line 123 at r4 (raw file):