optbuilder: refactor semantic analysis of FOR UPDATE #111258
Conversation
|
This change is |
|
TLDR: this changes type lockingSpec []*tree.LockingItemto type lockingSpec []*lockingItemAnd then replaces almost all uses of type lockingContext struct {
lockScope []*lockingItem
locking lockingSpec
} |
michae2
force-pushed
the
locking
branch
2 times, most recently
from
4577677 to
b964599
Compare
|
The test failures have been flakes. This is RFAL. |
There was a problem hiding this comment.
Reviewed 10 of 10 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, @msirek, and @nvanbenschoten)
-- commits line 32 at r3:
Do we have tests somewhere for allowed cases like these?
pkg/sql/opt/optbuilder/locking.go line 160 at r4 (raw file):
// blankLockingScope is a sentinel locking item that, when pushed, prevents // lockCtx.filter from matching targets outside it.
Is "it" the current scope?
pkg/sql/opt/optbuilder/select.go line 1148 at r4 (raw file):
} for range lockingClause {
Huh, I didn't know this was possible.
pkg/sql/logictest/testdata/logic_test/select_for_update line 168 at r4 (raw file):
query II SELECT * FROM (SELECT 1 UNION SELECT 1) a, i FOR UPDATE OF i
Could we do another test with FOR UPDATE OF a?
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 2 of 10 files at r2, 10 of 10 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status:
-- commits line 94 at r2:
These release notes are great.
pkg/sql/opt/optbuilder/select.go line 1100 at r1 (raw file):
inScope *scope, ) (outScope *scope) { locking.apply(lockingClause)
lockingSpec.apply modifies the receiver, not the parameter. We're then passing the parameter to rejectIfLocking for VALUES clauses, but still passing the receiver for UNION clauses. That's intentional?
If so, consider a comment explaining why we aren't passing the entire lockingSpec down on the VALUES paths.
EDIT: this is different in the second commit. I believe it no longer applies.
pkg/sql/opt/optbuilder/select.go line 1148 at r4 (raw file):
} for range lockingClause {
Consider giving this a comment, explaining the two different roles that it is playing.
There was a problem hiding this comment.
Very nice work!
Reviewed 10 of 10 files at r4, all commit messages.
Reviewable status:
pkg/sql/opt/optbuilder/locking.go line 122 at r4 (raw file):
// might include locking items that do not currently apply because they have // an unmatched target. lockScope []*lockingItem
Is there a reason lockScope is a slice, while locking is a type with the same slice definition? Is it just to indicate locking is the Spec of actually applicable items? Just wondering if lockingSpec should be used in both cases.
Make a few minor tweaks to semantic analysis of FOR UPDATE locking clauses. 1. Disallow multiple FOR UPDATE clauses on parenthesized queries. We do not currently handle scopes of parenthesized queries correctly. Because of this, we disallow multiple ORDER BY and LIMIT clauses on parenthesized queries. The previous implementation of FOR UPDATE was simple enough that we could work around this, but the upcoming changes make it impossible to support. 2. Allow FOR UPDATE on statements with VALUES in the FROM list (but continue to disallow FOR UPDATE on VALUES directly). This matches Postgres. Release note (sql change): Two minor changes to FOR UPDATE clauses: 1. Multiple FOR UPDATE clauses on fully-parenthesized queries are now disallowed. For example, the following statements are now disallowed: ``` (SELECT 1 FOR UPDATE) FOR UPDATE; SELECT * FROM ((SELECT 1 FOR UPDATE) FOR UPDATE) AS x; ``` whereas statements like the following are still allowed: ``` SELECT * FROM (SELECT 1 FOR UPDATE) AS x FOR UPDATE; SELECT (SELECT 1 FOR UPDATE) FOR UPDATE; ``` This does not match PostgreSQL, which allows all of these, but does match our behavior for ORDER BY and LIMIT. 2. FOR UPDATE is now allowed on statements with VALUES in the FROM list or as a subquery. For example, the following statements are now allowed: ``` SELECT (VALUES (1)) FOR UPDATE; SELECT * FROM (VALUES (1)) AS x FOR UPDATE; ``` Using FOR UPDATE directly on VALUES is still disallowed: ``` VALUES (1) FOR UPDATE; (VALUES (1)) FOR UPDATE; INSERT INTO t VALUES (1) FOR UPDATE; ``` This matches PostgreSQL.
Locking clauses such as FOR UPDATE and FOR SHARE apply to some or all of the data sources in a query's FROM list, depending on whether they have targets (FOR UPDATE OF x). Without targets, they always apply within subqueries in the FROM list. With targets, they apply within subqueries if the subquery alias matches the target. Because of this scope-like nature of FOR UPDATE and FOR SHARE, we implement semantic analysis using a stack of locking items that grow as we build inner subqueries deeper in the recursive optbuilder calls. Prior to this change, we only used the stack of locking items during buildScan, at the very bottom of the recursion. Because of this, calls to `lockingSpec.filter` could afford to compress the stack into a single locking item on our way deeper in the recursion. As part of the upcoming fix for cockroachdb#75457, however, we will need to build a new Lock operator when popping off locking items after returning from the recursion. That Lock operator will need some information gathered from buildScan at the bottom of the recursion. To support this, we refactor the stack of locking items to be two stacks: one that tracks all locking items in scope, and a second that tracks which locking items currently apply. This will allow buildScan to associate table information with the correct locking item(s), which can then be used to build Lock operators when popping the locking items. As a bonus, by using only the applied locking item stack in `validateLockingInFrom` we can make validation of SELECT FOR UPDATE queries a little more precise, which allows some queries we were incorrectly disallowing. Informs: cockroachdb#57031, cockroachdb#75457 Epic: CRDB-25322 Release note (sql change): Allow FOR UPDATE on some queries that were previously disallowed. Queries that use the following operations are now allowed to have FOR UPDATE OF as long as the prohibited operation is in a subquery not locked by the FOR UPDATE OF: - UNION - INTERSECT - EXCEPT - DISTINCT - GROUP BY - HAVING - aggregations - window functions For example, the following query is now allowed because the subquery using the prohibited operations is not affected by the FOR UPDATE OF: ``` SELECT * FROM t, (SELECT DISTINCT 0, 0 UNION SELECT a, count(*) FROM t GROUP BY a HAVING a > 0) AS u FOR UPDATE OF t; ``` This matches PostgreSQL.
There was a problem hiding this comment.
TFTRs! I'll go ahead and bors this and then if there are more comments, I can make changes in the next PR.
bors r=DrewKimball,msirek
Reviewable status:
Previously, DrewKimball (Drew Kimball) wrote…
Do we have tests somewhere for allowed cases like these?
Oh, good call. I added these to pkg/sql/logictest/testdata/logic_test/select_for_update.
pkg/sql/opt/optbuilder/locking.go line 122 at r4 (raw file):
Previously, msirek (Mark Sirek) wrote…
Is there a reason
lockScopeis a slice, whilelockingis a type with the same slice definition? Is it just to indicatelockingis theSpecof actually applicable items? Just wondering iflockingSpecshould be used in both cases.
Good question. Yes, it is to differentiate between the applied items and the items that are in scope but not necessarily applied. For example, it wouldn't make sense to call .isSet() or .get() on the items that are in scope but not necessarily applied.
I also did it to make the diff a little more meaningful (locking lockingSpec has the same purpose both before and after this PR: to describe the currently-applied locking).
pkg/sql/opt/optbuilder/locking.go line 160 at r4 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Is "it" the current scope?
"It" is the blank locking scope that was pushed.
For example, if we have a statement like:
SELECT * FROM t, (SELECT * FROM s, t FOR UPDATE OF s FOR SHARE) AS u FOR UPDATE OF t;Then when we're building the FROM clause of the subquery the lockCtx.lockScope will look like:
[ { strength: FOR UPDATE, targets: [t] }, blankLockingScope, { strength: FOR SHARE, targets: [] }, { strength: FOR UPDATE, targets: [s] } ]
And inside the subquery, only the FOR SHARE locking applies to t.
pkg/sql/opt/optbuilder/select.go line 1100 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
lockingSpec.applymodifies the receiver, not the parameter. We're then passing the parameter torejectIfLockingfor VALUES clauses, but still passing the receiver forUNIONclauses. That's intentional?If so, consider a comment explaining why we aren't passing the entire lockingSpec down on the VALUES paths.
EDIT: this is different in the second commit. I believe it no longer applies.
This was intentional, but yes it is confusing. Added a comment. For VALUES we only check the immediate statement for locking, whereas for UNION we check all outer statements as well. It is to match Postgres.
pkg/sql/opt/optbuilder/select.go line 1148 at r4 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Huh, I didn't know this was possible.
Me neither! crlfmt did this for me!
pkg/sql/opt/optbuilder/select.go line 1148 at r4 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Consider giving this a comment, explaining the two different roles that it is playing.
Done.
pkg/sql/logictest/testdata/logic_test/select_for_update line 168 at r4 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Could we do another test with
FOR UPDATE OF a?
Done.
|
Build succeeded: |
optbuilder: a few minor tweaks to building of FOR UPDATE
Make a few minor tweaks to semantic analysis of FOR UPDATE locking
clauses.
Disallow multiple FOR UPDATE clauses on parenthesized queries. We do
not currently handle scopes of parenthesized queries correctly.
Because of this, we disallow multiple ORDER BY and LIMIT clauses on
parenthesized queries. The previous implementation of FOR UPDATE was
simple enough that we could work around this, but the upcoming
changes make it impossible to support.
Allow FOR UPDATE on statements with VALUES in the FROM list (but
continue to disallow FOR UPDATE on VALUES directly). This matches
Postgres.
Release note (sql change): Two minor changes to FOR UPDATE clauses:
disallowed. For example, the following statements are now disallowed:
whereas statements like the following are still allowed:
This does not match PostgreSQL, which allows all of these, but does
match our behavior for ORDER BY and LIMIT.
or as a subquery. For example, the following statements are now allowed:
Using FOR UPDATE directly on VALUES is still disallowed:
This matches PostgreSQL.
optbuilder: refactor semantic analysis of FOR UPDATE
Locking clauses such as FOR UPDATE and FOR SHARE apply to some or all of
the data sources in a query's FROM list, depending on whether they have
targets (FOR UPDATE OF x). Without targets, they always apply within
subqueries in the FROM list. With targets, they apply within subqueries
if the subquery alias matches the target.
Because of this scope-like nature of FOR UPDATE and FOR SHARE, we
implement semantic analysis using a stack of locking items that grow as
we build inner subqueries deeper in the recursive optbuilder calls.
Prior to this change, we only used the stack of locking items during
buildScan, at the very bottom of the recursion. Because of this, calls
to
lockingSpec.filtercould afford to compress the stack into a singlelocking item on our way deeper in the recursion.
As part of the upcoming fix for #75457, however, we will need to build a
new Lock operator when popping off locking items after returning from
the recursion. That Lock operator will need some information gathered
from buildScan at the bottom of the recursion.
To support this, we refactor the stack of locking items to be two
stacks: one that tracks all locking items in scope, and a second that
tracks which locking items currently apply. This will allow buildScan to
associate table information with the correct locking item(s), which can
then be used to build Lock operators when popping the locking items.
As a bonus, by using only the applied locking item stack in
validateLockingInFromwe can make validation of SELECT FOR UPDATEqueries a little more precise, which allows some queries we were
incorrectly disallowing.
Informs: #57031, #75457
Epic: CRDB-25322
Release note (sql change): Allow FOR UPDATE on some queries that were
previously disallowed. Queries that use the following operations are now
allowed to have FOR UPDATE OF as long as the prohibited operation is in
a subquery not locked by the FOR UPDATE OF:
For example, the following query is now allowed because the subquery
using the prohibited operations is not affected by the FOR UPDATE OF:
This matches PostgreSQL.