pkg/sql/opt: validate correct use of FOR UPDATE locking clauses#43887
pkg/sql/opt: validate correct use of FOR UPDATE locking clauses#43887craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
This is consistent with the ordering of the rest of the type definitions in the file. Just code movement. Release note: None
This commit updates our SQL grammar to be identical to Postgres regarding select statement locking clauses. Concretely, this results in the following changes in behavior: - We can now parse multiple locking clause items - We can now parse the `FOR READ ONLY` syntax, which is a no-op - We can now parse the `SELECT ... FOR UPDATE ... LIMIT` syntax Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/gram.y#L11834 Release note: none
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 8 of 8 files at r2, 3 of 3 files at r3.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @RaduBerinde)
pkg/sql/opt/optbuilder/select.go, line 778 at r2 (raw file):
limit = stmt.Limit } if stmt.Locking != nil {
Do you have any tests of a locking clause inside a paren select?
pkg/sql/opt/optbuilder/select.go, line 1106 at r3 (raw file):
case sel.GroupBy != nil: b.raiseLockingError(first, "GROUP BY clause")
I didn't see a test for this one
This commit introduces a number of validation checks into the optbuilder to ensure that FOR UPDATE locking clauses are only being used in places where they're allowed. Most of this was based off of Postgres, which disallows the same set of operations to be used with FOR [KEY] UPDATE/SHARE locking. Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/analyze.c#L2691 Release note (sql change): Invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
a70f62f to
8693846
Compare
nvb
left a comment
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)
pkg/sql/opt/optbuilder/select.go, line 778 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Do you have any tests of a locking clause inside a paren select?
Done.
pkg/sql/opt/optbuilder/select.go, line 1106 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I didn't see a test for this one
Done.
42845: coldata: expose a modifier for batchSize variable r=yuzefovich a=yuzefovich This commit exposes a setter for batchSize private variable to be modified in tests and runs all tests in sql/colexec with a random batch size in [3, 4096] range. The tests in several places needed to be adjusted because their assumptions might no longer hold when batch size is modified. Fixes: #40791. Release note: None 43409: execinfrapb: print input types one per line in EXPLAIN (DISTSQL, TYPES) r=yuzefovich a=yuzefovich Previously, input types were printed in a single line within the input synchronizer box on the flow diagram. However, when we have a processor with two inputs and when each input has several columns, the boxes would collide and make the types unreadable. This commit changes it to print each type one per line, so there is no collision between different boxes of input synchronizer. Separately, cockroachdb.github.io has been adjusted so that the boxes for the input synchronizers could be displayed well in such scenario. Release note: None 43887: pkg/sql/opt: validate correct use of FOR UPDATE locking clauses r=nvanbenschoten a=nvanbenschoten The second commit updates our SQL grammar to be identical to Postgres regarding select statement locking clauses. Concretely, this results in the following changes in behavior: - We can now parse multiple locking clause items - We can now parse the `FOR READ ONLY` syntax, which is a no-op - We can now parse the `SELECT ... FOR UPDATE ... LIMIT` syntax Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/gram.y#L11834 The third commit introduces a number of validation checks into the optbuilder to ensure that FOR UPDATE locking clauses are only being used in places where they're allowed. Most of this was based off of Postgres, which disallows the same set of operations to be used with `FOR [KEY] UPDATE/SHARE` locking. Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/analyze.c#L2691 Release note (sql change): Invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer. 43980: storage: rename SSTSnapshotStorage{,Scratch,File} vars r=ajwerner a=irfansharif Around snapshot receiving code, we have variables for SSTSnapshotStorage, SSTSnapshotStorageScratch, and SSTSnapshotStorageFile types all abbreviated to some variation of sss{,s,f}. We also have "ssts" to talk about SSTs. This is all terribly confusing, we could do with some much needed clarity here. Release note: None. Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Build succeeded |
Relates to cockroachdb#40205. This change updates optbuilder to propagate row-level locking modes through the *scope object to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the ScanExpr level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in cockroachdb#43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Release note (sql change): More invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
Relates to cockroachdb#40205. This change updates optbuilder to propagate row-level locking modes through the *scope object to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the ScanExpr level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in cockroachdb#43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Release note (sql change): More invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
Relates to cockroachdb#40205. This change updates optbuilder to propagate row-level locking modes on the stack to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the ScanExpr level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in cockroachdb#43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Release note (sql change): More invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
Relates to cockroachdb#40205. This change updates optbuilder to propagate row-level locking modes on the stack to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the ScanExpr level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in cockroachdb#43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Release note (sql change): More invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
Relates to cockroachdb#40205. This change updates optbuilder to propagate row-level locking modes on the stack to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the ScanExpr level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in cockroachdb#43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Release note (sql change): More invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.
44015: sql/opt/optbuilder: propagate FOR UPDATE locking in scope to ScanExprs r=nvanbenschoten a=nvanbenschoten Relates to #40205. This change updates optbuilder to propagate row-level locking modes through the `*scope` object to achieve the same locking clause scoping that Postgres contains. The change propagates the locking mode all the way down to the `ScanExpr` level, where a single locking mode becomes an optional property of a Scan. With proper scoping rules, the change also improves upon the locking validation introduced in #43887. This completes the SELECT FOR [KEY] SHARE/UPDATE optbuilder interactions. The next step will be to hook the ScanExpr locking property into the execbuilder. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
The second commit updates our SQL grammar to be identical to Postgres regarding select statement locking clauses. Concretely, this results in the following changes in behavior:
FOR READ ONLYsyntax, which is a no-opSELECT ... FOR UPDATE ... LIMITsyntaxRef: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/gram.y#L11834
The third commit introduces a number of validation checks into the optbuilder to ensure that FOR UPDATE locking clauses are only being used in places where they're allowed. Most of this was based off of Postgres, which disallows the same set of operations to be used with
FOR [KEY] UPDATE/SHARElocking.Ref: https://github.com/postgres/postgres/blob/1a4a0329650b0545a54afb3c317aa289fd817f8a/src/backend/parser/analyze.c#L2691
Release note (sql change): Invalid usages of FOR UPDATE locking clauses are now rejected by the SQL optimizer.