release-25.2: execbuilder: fix locked indexes info in an edge case #149301
Conversation
yuzefovich
force-pushed
the
blathers/backport-release-25.2-149093
branch
from
5bb9043 to
77f4ae1
Compare
blathers-crl
bot
added
blathers-backport
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
Thanks for opening a backport. Before merging, please confirm that the change does not break backwards compatibility and otherwise complies with the backport policy. Include a brief release justification in the PR description explaining why the backport is appropriate. All backports must be reviewed by the TL for the owning area. While the stricter LTS policy does not yet apply, please exercise judgment and consider gating non-critical changes behind a disabled-by-default feature flag when appropriate. |
blathers-crl
bot
added
the
backport
|
This change is |
For buffered writes project, in 5e968e6 we started propagating information about which indexes have been locked which allows us to use non-locking Puts and Dels when mutating those indexes. That change had an incorrect assumption that any indexes we lock belong to the same table as the one being mutated. However, we just saw a user report where this wasn't the case - in some cases we might perform the initial scan on a different table (in the example, it was a FK referencing us), so we could incorrectly treat some indexes as having been locked when they haven't. With buffered writes enabled, this could lead to difference in blocking behavior (but no corruption or correctness issues since we still would find the concurrent write on COMMIT); with buffered writes disabled, the locked indexes info doesn't have any impact, so we could only hit an internal error when a given index ordinal doesn't exist in the target table. Furthermore, there is no point in locking an index of a different table since we might not actually modify it (this will depend on whether there exists a CASCADE action and which action it is). Release note (bug fix): CockroachDB previously could hit an internal error when performing a DELETE, an UPDATE, or an UPSERT statement where the initial scan of the mutation is locking and is on a table different from the one being mutated. As a workaround, one could do `SET enable_implicit_select_for_update = false`, but note that it might increase contention, so use with caution. The bug was introduced in 25.2 version and is now fixed.
yuzefovich
force-pushed
the
blathers/backport-release-25.2-149093
branch
from
77f4ae1 to
39501ea
Compare
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)
There was a problem hiding this comment.
IMO this counts as "breaking working and widely used functionality" because it fixes a bug that breaks statements such as DELETE ... WHERE ... (SELECT) and UPDATE ... WHERE ... (SELECT) which worked before v25.2.
I agree that this is already behind the existing enable_implicit_select_for_update session variable.
Tagging @rytaft because I'm not sure if this is the kind of change that needs committee approval.
Reviewable status:
|
Oh, 25.2 is not yet in LTS. So I guess this doesn't apply yet. |
|
Yeah, I think we don't need a more extensive review given 25.2 is not LTS yet. TFTR! |
Backport 1/1 commits from #149093 on behalf of @yuzefovich.
For buffered writes project, in 5e968e6 we started propagating information about which indexes have been locked which allows us to use non-locking Puts and Dels when mutating those indexes. That change had an incorrect assumption that any indexes we lock belong to the same table as the one being mutated. However, we just saw a user report where this wasn't the case - in some cases we might perform the initial scan on a different table (in the example, it was a FK referencing us), so we could incorrectly treat some indexes as having been locked when they haven't. With buffered writes enabled, this could lead to difference in blocking behavior (but no corruption or correctness issues since we still would find the concurrent write on COMMIT); with buffered writes disabled, the locked indexes info doesn't have any impact, so we could only hit an internal error when a given index ordinal doesn't exist in the target table.
Furthermore, there is no point in locking an index of a different table since we might not actually modify it (this will depend on whether there exists a CASCADE action and which action it is).
Fixes: #148650.
Release note (bug fix): CockroachDB previously could hit an internal error when performing a DELETE, an UPDATE, or an UPSERT statement where the initial scan of the mutation is locking and is on a table different from the one being mutated. As a workaround, one could do
SET enable_implicit_select_for_update = false, but note that it might increase contention, so use with caution. The bug was introduced in 25.2 version and is now fixed.Release justification: low-risk bug fix that can be disabled via existing
enable_implicit_select_for_updatesession variable. Disabling the fix by default doesn't make sense.