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

sql/opt: row-level locking and wait policy modes not propagated to index or lookup joins #56941

Closed
Cynthia-Lee97 opened this issue Nov 20, 2020 · 5 comments · Fixed by #60719
Closed
Labels
A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-community Originated from the community T-sql-queries SQL Queries Team

Comments

@Cynthia-Lee97
Copy link

Cynthia-Lee97 commented Nov 20, 2020

Describe the problem
I tried to use select for update nowait, but this statement got stuck. There was no result returned and no error was reported.

To Reproduce

  1. Set up CockroachDB Node
  2. CREATE TABLE t(a INT, b INT, c INT, INDEX id (c) STORING (b)); INSERT INTO t VALUES(1, 1, 1), (2, 2, 2),(3, 3, 3);

image
4. See error

Expected behavior
You should report an error instead of being stuck

Environment:

  • CockroachDB version 20.2
  • Server OS Linux
  • Client app cockroach sql

Jira issue: CRDB-2890

@blathers-crl
Copy link

blathers-crl bot commented Nov 20, 2020

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I was unable to automatically find someone to ping.

If we have not gotten back to your issue within a few business days, you can try the following:

  • Join our community slack channel and ask on #cockroachdb.
  • Try find someone from here if you know they worked closely on the area and CC them.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Nov 20, 2020
@rafiss
Copy link
Collaborator

rafiss commented Feb 18, 2021

cc @nvanbenschoten i think you might be familiar with the SFU locking behavior

@nvanbenschoten
Copy link
Member

Thanks @rafiss. I'm taking a look.

For me and others:

CREATE TABLE t(a INT, b INT, c INT, INDEX id (c) STORING (b)); 
INSERT INTO t VALUES(1, 1, 1), (2, 2, 2),(3, 3, 3);

# terminal 1
BEGIN;
SELECT * FROM t WHERE c % 2 != 0 FOR UPDATE NOWAIT;

# terminal 2
SELECT * FROM t@id WHERE c = 2 FOR UPDATE NOWAIT;

@nvanbenschoten
Copy link
Member

I'm able to reproduce. The issue appears to be that the index join present in the second query is not using the correct wait policy (or locking mode). So we're scanning the secondary index correctly, but not the primary index.

> EXPLAIN SELECT * FROM t@id WHERE c = 2 FOR UPDATE NOWAIT;

                 info
--------------------------------------
  distribution: local
  vectorized: true

  • index join
  │ table: t@primary
  │
  └── • scan
        estimated row count: 1
        table: t@id
        spans: [/2 - /2]
        locking strength: for update
        locking wait policy: nowait

nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Feb 18, 2021
… inverted joins

Fixes cockroachdb#56941.

This commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.

This is a WIP and needs some additional testing. I won't be able to get
to that before the upcoming stability period starts, so this likely
won't land in v21.1 unless someone else wants to pick this up.
@nvanbenschoten
Copy link
Member

I pushed a WIP fix for this to #60719, which likely won't land for the next release. This issue reveals an interesting change in expectations for SELECT FOR UPDATE. Before the addition of NOWAIT, it was mostly fine if locking policy propagation was best-effort because locking modes were just a performance optimization, so we didn't bother supporting it in all cases. Now that users have access to NOWAIT, they'll expect the wait policy to be propagated correctly in all cases.

@nvanbenschoten nvanbenschoten changed the title BUG in nowait statement of select for update sql/opt: row-level locking and wait policy modes not propagated to index or lookup joins Feb 18, 2021
@nvanbenschoten nvanbenschoten added A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed X-blathers-untriaged blathers was unable to find an owner labels Feb 18, 2021
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Apr 20, 2022
…ted joins

Fixes cockroachdb#56941.

This commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.

Release note (sql change): table scans performed as a part of index
joins, lookup joins, and inverted joins now respect the row-level
locking strength and wait policy specified by the optional
FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Apr 21, 2022
…ted joins

Fixes cockroachdb#56941.

This commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.

Release note (sql change): table scans performed as a part of index
joins, lookup joins, and inverted joins now respect the row-level
locking strength and wait policy specified by the optional
FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue Apr 22, 2022
…ted joins

Fixes cockroachdb#56941.

This commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.

Release note (sql change): table scans performed as a part of index
joins, lookup joins, and inverted joins now respect the row-level
locking strength and wait policy specified by the optional
FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
@craig craig bot closed this as completed in 8944fd0 Apr 26, 2022
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this issue May 17, 2022
…ted joins

Fixes cockroachdb#56941.

This commit updates the execbuilder to propagate row-level locking modes
through transformations from standard Scan and Join operations to
specialized IndexJoin, LookupJoin, and InvertedJoin operations.

Release note (sql change): table scans performed as a part of index
joins, lookup joins, and inverted joins now respect the row-level
locking strength and wait policy specified by the optional
FOR SHARE/UPDATE [NOWAIT] clause on SELECT statements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) O-community Originated from the community T-sql-queries SQL Queries Team
Projects
None yet
4 participants