cockroachdb / cockroachdb/cockroach
sql, kv: support non-default key locking with leaf txns
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
With #94399 we no longer choose to use the leaf txn (which would allow us to use the streamer API and parallelize local scans in some cases) if we find non-default key locking strength. This stems from the fact that we don't propagate lock spans for leaf txns, and even if we did, we'd run into another complication (quoting Nathan):
> The other complication that we would have run into with this approach is that only the root txn coordinator runs a heartbeat loop to keep its locks alive. If we only acquired locks in leaf txns, we would never have started the txn heartbeat and the txn could have been aborted after 5s. We could have launched the heartbeat when merging the `LeafTxnFinalState`, but that still only works if we call `UpdateRootWithLeafFinalState` within 5s of the initial lock being acquired.
> I think this again hints at our implementation of explicit row-level locking and pushing that locking directly into scans (in all cases) being a little funky (see #75457 and #57031). If we had a separate `RowLock` execution node that lived above a scan/filter/limit/etc. then we could run that using the RootTxn and distribute the rest of the query as much as we'd like. We're going to have to do part of that work for read-committed.
We should figure out how to lift this restriction.
Original issue description
*Edit from @jordanlewis: See https://github.com/cockroachdb/cockroach/issues/94290#issuecomment-1366343646 for a trivial repro*
*To work around this problem in 22.2.0 and 22.2.1, run the following*
```
SET CLUSTER SETTING sql.distsql.use_streamer.enabled = false;
```
----
**Describe the problem**
It looks like `v22.2.1` has some regression with `FOR UPDATE`. It looks like that a lock is being held on the row for 5 seconds after the transaction finishes. Several of my e2e tests are now failing due to timeout. I drilled into one the tests.
Doing explain analyze on the select query after the transaction finishes shows `cumulative time spent in KV: 4.9s` and `cumulative time spent due to contention: 4.9s`. Find attached also a debug zip from explain ANALYZE(debug) from the query.
Removing `for update` fixes the issue, OR reverting to `v22.1.8` fixes the issue.
**To Reproduce**
The test on my codebase fails pretty consistently. From what I can tell, the sequence of events:
1. Transaction starts.
2. Transaction runs `select * from tbl where id = 1 for update`
3. The row in `tbl` is NOT updated inside of the transaction, but other data in unrelated tables is updated/inserted.
4. Transaction completes successfully.
5. (only run after transaction is complete) `select * from tbl` now takes 5 seconds to run.
**Expected behavior**
Running `SELECT * FROM tbl;` when there is no other workload on the database to take a few milliseconds to run.
**Additional data / screenshots**
[stmt-bundle-825387661464305665.zip](https://github.com/cockroachdb/cockroach/files/10300490/stmt-bundle-825387661464305665.zip)
**Environment:**
- CockroachDB version v22.2.1
- Server OS: Linux on docker
- Client app: Sequelize
**Additional context**
E2E (including clicking on buttons) Tests are failing due to timeout.
Jira issue: CRDB-22800
Contributor guide
Research direction
Start by reading #94399 and the referenced issues #75457 and #57031, then trace the streamer API, LeafTxnFinalState, and UpdateRootWithLeafFinalState paths. Determine how leaf transactions can preserve non-default lock spans and transaction heartbeats without the five-second timeout; done means the restriction is lifted while locking remains correct, including for read-committed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- databases, distributed-systems
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 28/100