-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql, kv: support non-default key locking with leaf txns #94290
Comments
This comment was marked as outdated.
This comment was marked as outdated.
FOR UPDATE
unexpected holds a lock for longer than expectedFOR UPDATE
inexpertly holds a lock for longer than expected
FOR UPDATE
inexpertly holds a lock for longer than expectedFOR UPDATE
unexpectedly holds a lock for longer than expected
Thanks for the report! Have you tried with v22.2.0? |
@rafiss Thanks for the response. I should have tried that. I tried with
|
Hello @rafiss, I was able to create a minimum viable repro of the issue. Note that each command needs to be run on separate connections. This is running (v22.2.1) Create table: -- drop table if exists dealers cascade;
CREATE TABLE dealers (
"organizationId" UUID NOT NULL,
"conventionId" INT8 NOT NULL,
id INT8 NOT NULL,
"userId" INT8 NULL,
CONSTRAINT dealers_pkey PRIMARY KEY ("organizationId" ASC, id ASC),
UNIQUE INDEX "dealers_userId_conventionId_key" ("userId" ASC, "conventionId" ASC),
UNIQUE INDEX "dealers_conventionId_key" ("conventionId" ASC, id ASC)
); Connection 1 (10ms): insert into dealers values ('00000000-0000-0000-0000-000000000000', 5427958, 1, 1); Connection 2 (Takes 5100ms to run): START TRANSACTION;
select * from dealers where "conventionId" = 5427958 and id = 1 for update;
COMMIT;
select * from dealers; Oddly, removing the Please let me know if you are able to repro the issue. |
I can't repro this on 22.2.1, what is the exact order/interleaving of the commands you're running on each connection? |
Ok, I could reproduce it. The 2 connection thing is a red herring, you can repro on 1 connection like this:
Once you have the select for update in the shell, just hit up/enter quickly and you'll see the slow query. I think this has to do with the EXPLAIN ANALYZE output on the slow iteration looks like this:
Note the 4.5 second "contention time". |
FOR UPDATE
unexpectedly holds a lock for longer than expectedFOR UPDATE
unexpectedly holds a lock for longer than expected
Bisected to b72c109 (#77878), which unfortunately doesn't tell us much. Is the streamer using APIs incorrectly, or is there bug a bug in the API that the streamer is using? cc @yuzefovich Bisect log
|
Sure enough, disabling the streamer cluster setting removes the problem:
EXPLAIN ANALYZE:
|
FOR UPDATE
unexpectedly holds a lock for longer than expectedFOR UPDATE
unexpectedly holds a lock for longer than expected
I think the difference here is that the streamer is using The behavior is such that it seems that the locks with the leaf txn are only released after about 4.5s have passed after the txn that acquired them was committed. Maybe this was already mentioned elsewhere, but the problematic stmt is not the second statement that blocks (which can actually use either the streamer or the non-streamer code path), but the first stmt that used the streamer. I think we need KV expertise here. Perhaps @nvanbenschoten can share some wisdom. |
@jordanlewis what's your take on the severity of this issue? Should we block 22.2.2 release? Should we consider disabling the streamer in 22.2.2? I'm tentatively adding the corresponding release blocker labels. |
Nvm, it can place lock spans. But they need to be "imported" into the RootTxn at the end. |
That should happen during metadata draining here where |
|
Probably the best quick fix is to examine the plan to see whether non-default key-locking is used and to not use the streamer if so. I'll work on that today. |
Is it not simpler to extend GetLeafTxnFinalState to also include the lock spans if there's any? |
Thanks. Indeed, that seems simple enough. |
maybe double check that UpdateRootWithLeafFinalState merges the lock spans too |
94399: sql: don't use streamer for local flows and non-default key locking r=yuzefovich a=yuzefovich This commit makes it so that we don't use the streamer API when running fully-local plans if some processors in that flow require non-default key locking. This change allows us to fix a regression with `SELECT FOR UPDATE` where the acquired locks by that stmt would not be properly cleaned up on the txn commit (because we currently don't propagate lock spans for leaf txns, and the streamer requires us to use the leaf txns). The initial attempt to fix this propagation exposed an issue with multi-tenant setups, so for now we choose to simply not use the streamer in such cases. Additionally, this commit disables the parallelization of local scans when non-default key locking strength is found. The parallelization of local scans also requires us to use the leaf txns, and it was introduced in 21.2 version; however, we haven't had any user reports. Still, it seems reasonable to update that code path with the recent learnings. Addresses: #94290. Addresses: #94400. Epic: None Release note (bug fix): Previously, CockroachDB could delay the release of the locks acquired when evaluating SELECT FOR UPDATE statements in some cases. This delay (up to 5s) could then block the future readers. The bug was introduced in 22.2.0, and the temporary workaround without upgrading to a release with this fix is to set undocumented cluster setting `sql.distsql.use_streamer.enabled` to `false`. Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@Nican thanks for filing the issue, and @jordanlewis thanks for determining the concise reproduction. We have merged the fixes to master and 22.2 branches, and it should be included in the 22.2.2 release. This issue now becomes about figuring out how to lift the restriction introduced in #94399. In particular, we either need to propagate the lock spans across leaf txns or re-implement how we do row-level locking. I'll update the issue description accordingly. |
FOR UPDATE
unexpectedly holds a lock for longer than expectedPreviously, as noted in cockroachdb#94290, it was possible for a LeafTxn to issue locking requests as part of SELECT FOR UPDATE. This behavior was unexpected and the RootTxn wasn't properly cleaning up the locks, resulting in others waiting for those locks to be released. The issue was resolved, in cockroachdb#94399, by ensuring non-default locking strength transactions don't use the streamer API and always run as RootTxn. This patch adds an assertion on the kv side to prevent other existing or future attempts of LeafTxn issuing locking requests. We don't expect that there are such existing cases, so we don't expect this assertion to fail, but will keep an eye on the nightly tests to make sure. Fixes: cockroachdb#97817 Release note: None
Previously, as noted in cockroachdb#94290, it was possible for a LeafTxn to issue locking requests as part of SELECT FOR UPDATE. This behavior was unexpected and the RootTxn wasn't properly cleaning up the locks, resulting in others waiting for those locks to be released. The issue was resolved, in cockroachdb#94399, by ensuring non-default locking strength transactions don't use the streamer API and always run as RootTxn. This patch adds an assertion on the kv side to prevent other existing or future attempts of LeafTxn issuing locking requests. We don't expect that there are such existing cases, so we don't expect this assertion to fail, but will keep an eye on the nightly tests to make sure. Fixes: cockroachdb#97817 Release note: None
Previously, as noted in cockroachdb#94290, it was possible for a LeafTxn to issue locking requests as part of SELECT FOR UPDATE. This behavior was unexpected and the RootTxn wasn't properly cleaning up the locks, resulting in others waiting for those locks to be released. The issue was resolved, in cockroachdb#94399, by ensuring non-default locking strength transactions don't use the streamer API and always run as RootTxn. This patch adds an assertion on the kv side to prevent other existing or future attempts of LeafTxn issuing locking requests. We don't expect that there are such existing cases, so we don't expect this assertion to fail, but will keep an eye on the nightly tests to make sure. Fixes: cockroachdb#97817 Release note: None
98741: ci: update bazel builder image r=rickystewart a=cockroach-teamcity Release note: None Epic: None 98878: backupccl: fix occassional TestRestoreErrorPropagates flake r=stevendanna a=adityamaru Very rarely under stress race another automatic job would race with the restore and increment the error count. This would result in the count being greater than our expected value of 1. This disables all the automatic jobs eliminating the chance of this race. Fixes: #98037 Release note: None 99099: kvserver: deflake TestReplicaTombstone r=andrewbaptist a=tbg Like many other tests, this test could flake because we'd sometimes catch a "cannot remove learner while snapshot is in flight" error. I think the root cause is that sometimes there are errant Raft snapshots in the system[^1] and these get mistaken for LEARNERs that are still being caught up by the replicate queue. I tried to address this general class of issues by making the check for in-flight learner snapshots not care about *raft* snapshots. I was able to stress TestReplicaTombstone for 30+ minutes without a failure using that approach, whereas previously it usually failed within a few minutes. ``` ./dev test --stress pkg/kv/kvserver/ --filter TestReplicaTombstone 2>&1 | tee stress.log [...] 2461 runs so far, 0 failures, over 35m45s ``` [^1]: #87553 Fixes #98883. Epic: none Release note: None 99126: kv: return error on locking request in LeafTxn r=nvanbenschoten a=miraradeva Previously, as noted in #94290, it was possible for a LeafTxn to issue locking requests as part of SELECT FOR UPDATE. This behavior was unexpected and the RootTxn wasn't properly cleaning up the locks, resulting in others waiting for those locks to be released. The issue was resolved, in #94399, by ensuring non-default locking strength transactions don't use the streamer API and always run as RootTxn. This patch adds an assertion on the kv side to prevent other existing or future attempts of LeafTxn issuing locking requests. We don't expect that there are such existing cases, so we don't expect this assertion to fail, but will keep an eye on the nightly tests to make sure. Fixes: #97817 Release note: None 99150: backupccl: stop logging unsanitized backup stmt in schedule executor r=stevendanna a=msbutler Informs #99145 Release note: None Co-authored-by: cockroach-teamcity <teamcity@cockroachlabs.com> Co-authored-by: adityamaru <adityamaru@gmail.com> Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com> Co-authored-by: Mira Radeva <mira@cockroachlabs.com> Co-authored-by: Michael Butler <butler@cockroachlabs.com>
Previously, as noted in #94290, it was possible for a LeafTxn to issue locking requests as part of SELECT FOR UPDATE. This behavior was unexpected and the RootTxn wasn't properly cleaning up the locks, resulting in others waiting for those locks to be released. The issue was resolved, in #94399, by ensuring non-default locking strength transactions don't use the streamer API and always run as RootTxn. This patch adds an assertion on the kv side to prevent other existing or future attempts of LeafTxn issuing locking requests. We don't expect that there are such existing cases, so we don't expect this assertion to fail, but will keep an eye on the nightly tests to make sure. Fixes: #97817 Release note: None
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):
We should figure out how to lift this restriction.
Original issue description
Edit from @jordanlewis: See #94290 (comment) for a trivial repro
To work around this problem in 22.2.0 and 22.2.1, run the following
Describe the problem
It looks like
v22.2.1
has some regression withFOR 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
andcumulative 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 tov22.1.8
fixes the issue.To Reproduce
The test on my codebase fails pretty consistently. From what I can tell, the sequence of events:
select * from tbl where id = 1 for update
tbl
is NOT updated inside of the transaction, but other data in unrelated tables is updated/inserted.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
Environment:
Additional context
E2E (including clicking on buttons) Tests are failing due to timeout.
Jira issue: CRDB-22800
The text was updated successfully, but these errors were encountered: