pkg/sql/opt: validate correct use of FOR UPDATE locking clauses #43887

nvanbenschoten · 2020-01-10T23:13:21Z

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.

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

cockroach-teamcity · 2020-01-10T23:13:30Z

This change is

rytaft

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.

nvanbenschoten

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>

craig · 2020-01-14T22:22:56Z

Build succeeded

GitHub CI (Cockroach)

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.

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>

nvanbenschoten added 2 commits January 10, 2020 18:10

sql/sem/tree: move locking AST nodes to bottom of file

109107b

This is consistent with the ordering of the rest of the type definitions in the file. Just code movement. Release note: None

nvanbenschoten requested a review from RaduBerinde January 10, 2020 23:13

nvanbenschoten requested a review from a team as a code owner January 10, 2020 23:13

rytaft approved these changes Jan 14, 2020

View reviewed changes

nvanbenschoten force-pushed the nvanbenschoten/forUpdateAST branch from a70f62f to 8693846 Compare January 14, 2020 21:39

nvanbenschoten commented Jan 14, 2020

View reviewed changes

craig bot merged commit 8693846 into cockroachdb:master Jan 14, 2020

nvanbenschoten deleted the nvanbenschoten/forUpdateAST branch January 14, 2020 22:56

nvanbenschoten mentioned this pull request Jan 15, 2020

sql/opt/optbuilder: propagate FOR UPDATE locking in scope to ScanExprs #44015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/sql/opt: validate correct use of FOR UPDATE locking clauses #43887

pkg/sql/opt: validate correct use of FOR UPDATE locking clauses #43887

nvanbenschoten commented Jan 10, 2020

cockroach-teamcity commented Jan 10, 2020

rytaft left a comment

nvanbenschoten left a comment

craig bot commented Jan 14, 2020

pkg/sql/opt: validate correct use of FOR UPDATE locking clauses #43887

pkg/sql/opt: validate correct use of FOR UPDATE locking clauses #43887

Conversation

nvanbenschoten commented Jan 10, 2020

cockroach-teamcity commented Jan 10, 2020

rytaft left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

craig bot commented Jan 14, 2020

Build succeeded