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

[WIP] opt: add hint to ignore preserved-multiplicity consistency #82188

Closed

Conversation

michae2
Copy link
Collaborator

@michae2 michae2 commented Jun 1, 2022

For some queries (e.g. SELECT FOR UPDATE SKIP LOCKED) we need to disable
optimizations that depend on preserved-multiplicity consistency of
tables. Add an IGNORE_PRESERVED_CONSISTENCY hint that turns off these
optimizations. With this hint, we will no longer use optimizations that
assume:

  • a PK row exists for every secondary index row
  • a FK row exists for every referenced FK

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@michae2 michae2 force-pushed the ignore_preserved_consistency branch 2 times, most recently from 733de8e to 6e3908c Compare June 1, 2022 05:49
For some queries (e.g. SELECT FOR UPDATE SKIP LOCKED) we need to disable
optimizations that depend on preserved-multiplicity consistency of
tables. Add an IGNORE_PRESERVED_CONSISTENCY hint that turns off these
optimizations. With this hint, we will no longer use optimizations that
assume:
- a PK row exists for every secondary index row
- a FK row exists for every referenced FK

Release note: None
@michae2 michae2 force-pushed the ignore_preserved_consistency branch from 6e3908c to 4b350da Compare June 2, 2022 00:12
@michae2 michae2 changed the title opt: add hint to ignore preserved-multiplicity consistency [WIP] opt: add hint to ignore preserved-multiplicity consistency Jun 7, 2022
@michae2 michae2 added the do-not-merge bors won't merge a PR with this label. label Jun 7, 2022
@michae2
Copy link
Collaborator Author

michae2 commented Jun 21, 2022

Note to self: this is a little bit broken. Some of the rules that I modified would work fine and probably don't need to be modified. Also this needs tests.

@michae2
Copy link
Collaborator Author

michae2 commented Jun 21, 2022

Tracking issue is: #83156

@michae2
Copy link
Collaborator Author

michae2 commented Jul 19, 2022

Notes from talking to @rytaft:

  • do we need to do something similar to IgnoreUniqueWithoutIndexKeys in logical_props_builder.go for functional dependencies?
  • we probably also need to adjust cardinality of index joins
  • are we handling column families correctly? ah, no, we need to handle this

@michae2 michae2 closed this Aug 8, 2022
@michae2 michae2 deleted the ignore_preserved_consistency branch August 8, 2022 19:23
craig bot pushed a commit that referenced this pull request Aug 10, 2022
85720: sql: parser and optimizer support FOR {UPDATE,SHARE} SKIP LOCKED r=rytaft a=rytaft

This PR adds support for `SKIP LOCKED` by building on commits from #83627 and #82188. See below for details.

**sql: enable the skip-locked wait policy**

Now that KV support this, we can pass the wait policy through.

Fixes #40476

Release note (sql change): `SELECT ... FOR {UPDATE,SHARE} SKIP LOCKED`
is now supported. The option can be used to skip rows that cannot be
immediately locked instead of blocking on contended row-level lock
acquisition.

**opt: optimizer updates for support of SKIP LOCKED**

For queries using `SELECT FOR {SHARE,UPDATE} SKIP LOCKED`, we need to disable
optimizations that depend on preserved-multiplicity consistency of
tables. When `SKIP LOCKED` is used, we will no longer use optimizations that
assume:
- a PK row exists for every secondary index row
- a PK row exists for every referencing FK (if the PK table uses `SKIP LOCKED`)

One result of this change is that we will no longer push limits into index
joins if the primary index uses locking wait policy `SKIP LOCKED`.

This commit also disallows use of multiple column families in tables scanned
with `SKIP LOCKED`, since it could result in returning partial rows.

Release note: None

Co-authored-by: Michael Erickson <[email protected]>


Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge bors won't merge a PR with this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants