Skip to content
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

kv: skip locked does not work correctly with shared locks #110743

Closed
arulajmani opened this issue Sep 15, 2023 · 3 comments
Closed

kv: skip locked does not work correctly with shared locks #110743

arulajmani opened this issue Sep 15, 2023 · 3 comments
Assignees
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-read-committed Related to the introduction of Read Committed branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-23.2.0-rc (deleted) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-known-limitation T-kv KV Team

Comments

@arulajmani
Copy link
Collaborator

arulajmani commented Sep 15, 2023

Describe the problem

To reproduce:

-- Session 1
demo@localhost:26257/demoapp/defaultdb> CREATE TABLE t(a INT PRIMARY KEY);
CREATE TABLE

Time: 6ms total (execution 6ms / network 0ms)

demo@localhost:26257/demoapp/defaultdb> INSERT INTO t VALUES(1);
INSERT 0 1

Time: 8ms total (execution 8ms / network 0ms)

demo@localhost:26257/demoapp/defaultdb> BEGIN;
BEGIN

Time: 0ms total (execution 0ms / network 0ms)

demo@localhost:26257/demoapp/defaultdb  OPEN> SELECT * FROM t FOR SHARE;
  a
-----
  1
(1 row)

Time: 3ms total (execution 3ms / network 0ms)

demo@localhost:26257/demoapp/defaultdb  OPEN>

--- Session 2

demo@127.0.0.1:26257/demoapp/defaultdb> BEGIN;
BEGIN

Time: 0ms total (execution 1ms / network 0ms)

demo@127.0.0.1:26257/demoapp/defaultdb  OPEN> SELECT * FROM t FOR SHARE SKIP LOCKED;
  a
-----
(0 rows)

Time: 1ms total (execution 1ms / network 0ms)

demo@127.0.0.1:26257/demoapp/defaultdb  OPEN>

The SKIP LOCKED select should return (and lock) the only row in the table; it does not.

This is because we're not plumbing lock strengths in pebbleMVCCScanner -- we're instead deriving the lock strength from the failOnMoreRecent option; we infer it to be lock.Exclusive.

Jira issue: CRDB-31588

Epic CRDB-34172

@arulajmani arulajmani added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-transactions Relating to MVCC and the transactional model. T-kv KV Team A-read-committed Related to the introduction of Read Committed labels Sep 15, 2023
@arulajmani arulajmani self-assigned this Sep 15, 2023
@arulajmani arulajmani changed the title kv: skip locked does't work correctly with shared locks kv: skip locked does not work correctly with shared locks Sep 15, 2023
arulajmani added a commit to arulajmani/cockroach that referenced this issue Sep 18, 2023
Previously, we would infer the lock strength from `failOnMoreRecent`
when determining whether a key was locked by a conflicting transaction
when evaluating a SKIP LOCKED request -- we'd infer it to either be
lock.Exclusive or lock.None. The introduction of shared locks broke
this mapping, as now both lock.Shared and lock.Exclusive locking
requests map to failOnMoreRecent. This patch correctly plumbs down the
correct locking strength for Get/Scan/ReverseScan requests. It then uses
it when constructing a pebbleMVCCScanner, which in-turn uses it to
determine skip locked conflicts (or lack thereof).

Fixes cockroachdb#110743

Release note: None
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Nov 9, 2023
Informs cockroachdb#110743.

This commit adds a new test file for TestConcurrencyManagerBasic which
tests the skip-locked wait policy with shared locks.

Release note: None
@nvanbenschoten
Copy link
Member

https://github.com/nvanbenschoten/cockroach/commits/nvanbenschoten/skipLockedSharedRepl contains a prototype of the fix for this issue.

@michae2
Copy link
Collaborator

michae2 commented Dec 13, 2023

After talking with @arulajmani and @nvanbenschoten, removing the GA-blocker label.

@michae2 michae2 self-assigned this Dec 13, 2023
arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 14, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
@michae2
Copy link
Collaborator

michae2 commented Dec 14, 2023

The limitation to document here is that FOR SHARE SKIP LOCKED is not yet supported (because it will incorrectly skip over shared locks that could be acquired).

arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 15, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
craig bot pushed a commit that referenced this issue Dec 15, 2023
116446: kv: disable use of shared locks in conjunction with skip locked r=nvanbenschoten a=arulajmani

We currently don't support the use of shared locks and the skip locked wait policy together. Until this limitation is lifted, we should return unimplemented errors instead of the wrong result.

Informs #110743

Release note: None

Co-authored-by: Arul Ajmani <[email protected]>
arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 19, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 19, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 19, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Dec 19, 2023
We currently don't support the use of shared locks and the skip locked
wait policy together. Until this limitation is lifted, we should return
unimplemented errors instead of the wrong result.

Informs cockroachdb#110743

Release note: None
arulajmani pushed a commit to arulajmani/cockroach that referenced this issue Jan 10, 2024
Informs cockroachdb#110743.

This commit adds a new test file for TestConcurrencyManagerBasic which
tests the skip-locked wait policy with shared locks.

Release note: None
arulajmani added a commit to arulajmani/cockroach that referenced this issue Jan 10, 2024
Previously, when calling into the in-memory lock table to determine
whether a key needs to be skipped because of the SKIP LOCKED wait
policy, we would infer the request's strength using the
`FailOnMoreRecent` option. The introduction of shared locks broke this
inference. For this reason, SELECT FOR SHARE SKIP LOCKED was disallowed.

This patch polishes and adds testing for 7bea05d, thereby enabling
SELECT FOR SHARE SKIP LOCKED.

Resolves cockroachdb#110743

Release note (sql change): SELECT FOR SHARE SKIP LOCKED, which was
previously disallowed, no works.
arulajmani pushed a commit to arulajmani/cockroach that referenced this issue Jan 16, 2024
Informs cockroachdb#110743.

This commit adds a new test file for TestConcurrencyManagerBasic which
tests the skip-locked wait policy with shared locks.

Release note: None
@craig craig bot closed this as completed in ed0ccb4 Jan 17, 2024
@github-project-automation github-project-automation bot moved this to Closed in KV Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-transactions Relating to MVCC and the transactional model. A-read-committed Related to the introduction of Read Committed branch-release-23.2 Used to mark GA and release blockers, technical advisories, and bugs for 23.2 branch-release-23.2.0-rc (deleted) C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-known-limitation T-kv KV Team
Projects
No open projects
Status: Closed
Development

Successfully merging a pull request may close this issue.

3 participants