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

release-23.2: opt: add lock operator #112138

Merged
merged 2 commits into from Oct 13, 2023

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Oct 10, 2023

Backport 2/2 commits from #111662 on behalf of @michae2.

/cc @cockroachdb/release


execbuilder: change error message to match PostgreSQL

Release note (sql change): Change the following error message:

statement error cannot execute FOR UPDATE in a read-only transaction

to this to match PostgreSQL:

statement error cannot execute SELECT FOR UPDATE in a read-only transaction

opt: add lock operator

Add a new implementation of SELECT FOR UPDATE and SELECT FOR SHARE
statements. Instead of locking during the initial row fetch, this new
implementation constructs a Lock operator on the top of the query plan
which performs the locking phase using a locking semi-join lookup.

During optbuilder we build plans with both Lock operators and
initial-row-fetch locking. During execbuilder we decide which
implementation to use based on the isolation level and whether
optimizer_use_lock_op_for_serializable is set. If the new
implementation is chosen, Lock operators become locking
semi-LookupJoins.

In some cases these new plans will have superfluous lookup joins. A
future PR will optimize away some of these superfluous lookup joins.

Fixes: #57031, #75457

Epic: CRDB-25322

Release note (sql change): Add a new session variable,
optimizer_use_lock_op_for_serializable, which when set enables a new
implementation of SELECT FOR UPDATE. This new implementation of
SELECT FOR UPDATE acquires row locks after any joins and filtering,
and always acquires row locks on the primary index of the table being
locked. This more closely matches SELECT FOR UPDATE behavior in
PostgreSQL, but at the cost of more round trips from gateway node to
replica leaseholder.

Under read committed isolation (and other isolation levels weaker than
serializable) we will always use this new implementation of SELECT FOR UPDATE regardless of the value of
optimizer_use_lock_op_for_serializable to ensure correctness.


Release justification: part of the read committed commitment for 23.2.

Release note (sql change): Change the following error message:

```
statement error cannot execute FOR UPDATE in a read-only transaction
```

to this to match PostgreSQL:

```
statement error cannot execute SELECT FOR UPDATE in a read-only transaction
```
Add a new implementation of `SELECT FOR UPDATE` and `SELECT FOR SHARE`
statements. Instead of locking during the initial row fetch, this new
implementation constructs a `Lock` operator on the top of the query plan
which performs the locking phase using a locking semi-join lookup.

During optbuilder we build plans with both `Lock` operators and
initial-row-fetch locking. During execbuilder we decide which
implementation to use based on the isolation level and whether
`optimizer_use_lock_op_for_serializable` is set. If the new
implementation is chosen, `Lock` operators become locking
semi-LookupJoins.

In some cases these new plans will have superfluous lookup joins. A
future PR will optimize away some of these superfluous lookup joins.

Fixes: #57031, #75457

Epic: CRDB-25322

Release note (sql change): Add a new session variable,
`optimizer_use_lock_op_for_serializable`, which when set enables a new
implementation of `SELECT FOR UPDATE`. This new implementation of
`SELECT FOR UPDATE` acquires row locks *after* any joins and filtering,
and always acquires row locks on the primary index of the table being
locked. This more closely matches `SELECT FOR UPDATE` behavior in
PostgreSQL, but at the cost of more round trips from gateway node to
replica leaseholder.

Under read committed isolation (and other isolation levels weaker than
serializable) we will always use this new implementation of `SELECT FOR
UPDATE` regardless of the value of
`optimizer_use_lock_op_for_serializable` to ensure correctness.
@blathers-crl blathers-crl bot requested a review from a team as a code owner October 10, 2023 22:18
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-111662 branch from 4cef6e7 to e129694 Compare October 10, 2023 22:18
@blathers-crl blathers-crl bot requested review from DrewKimball and removed request for a team October 10, 2023 22:18
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-111662 branch from 2ec05b3 to aae01f9 Compare October 10, 2023 22:18
@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Oct 10, 2023
@blathers-crl
Copy link
Author

blathers-crl bot commented Oct 10, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Oct 10, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @msirek)

@michae2
Copy link
Collaborator

michae2 commented Oct 12, 2023

@mgartner this is the last of the read committed functionality. It just missed the branch cut by about an hour, so I had to backport. PTAL when you get the chance.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In some cases these new plans will have superfluous lookup joins. A
future PR will optimize away some of these superfluous lookup joins.

Is this planned for 23.2 or later?

@michae2
Copy link
Collaborator

michae2 commented Oct 13, 2023

Is this planned for 23.2 or later?

The work hasn't been done yet, but maybe 23.2 if I get to it and the backport is small enough and you think it's a good idea.

@michae2 michae2 merged commit 6bf8eb2 into release-23.2 Oct 13, 2023
6 checks passed
@michae2 michae2 deleted the blathers/backport-release-23.2-111662 branch October 13, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants