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

[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.

[cockroach-teamcity]

This change is 

[rytaft]

Reviewed 1 of 1 files at r1, 13 of 15 files at r2, 3 of 3 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

pkg/sql/logictest/testdata/logic_test/select_for_update, line 48 at r2 (raw file):

# However, we do mirror Postgres in that we require FOR UPDATE targets to be
# unqualified names and reject anything else.

Is this needed for correctness? Seems like kind of a random requirement...

Edit -- saw the comment below. Either way is fine with me.

---

pkg/sql/logictest/testdata/logic_test/select_for_update, line 180 at r2 (raw file):

2

# Use of SELECT FOR UPDATE requires UPDATE privileges, not just SELECT privileges.

How about a test of FOR SHARE?

---

pkg/sql/opt/optbuilder/scope.go, line 1315 at r2 (raw file):

}

// lockingForTable returns the desired row-level locking mode for the specifies

[nit] specifies -> specified

---

pkg/sql/opt/optbuilder/scope.go, line 1324 at r2 (raw file):

	var copied bool
	updateRet := func(li *tree.LockingItem) {
		if ret == nil && len(li.Targets) == 0 {

Why do you need the check if len(li.Targets) == 0 here? Seems like that's already checked below.

---

pkg/sql/opt/optbuilder/select.go, line 59 at r3 (raw file):

		// TODO DURING REVIEW: surprisingly, it looks like all TableName data
		// sources are wrapped by an AliasedTableExpr. Is this true? Can we

As far as I can tell, the parser doesn't directly return tree.TableNames, but it does return tree.TableRefs (numeric table references).

---

pkg/sql/opt/optbuilder/select.go, line 126 at r3 (raw file):

		priv := privilege.SELECT
		if li != nil {
			// SELECT ... FOR [KEY] UPDATE/SHARE requires UPDATE privileges.

Why does SHARE require update privileges?

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 74 at r3 (raw file):

build
SELECT * FROM t FOR UPDATE OF t2

Is it a problem that we are dropping this locking request? How does Postgres handle this?

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 151 at r3 (raw file):

build
SELECT * FROM v FOR UPDATE OF v2

What about SELECT * FROM v FOR UPDATE OF t? How should that behave?

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 261 at r3 (raw file):

                          ├── columns: a:1(int!null) b:2(int)
                          └── locking: for-update

How about adding a test where a stronger locking from outside overrides a weaker locking inside a subquery?

---

pkg/sql/sem/tree/select.go, line 1063 at r3 (raw file):

}

// Max returns the maximum of the two locking weight policies.

weight -> wait

[nvanbenschoten]

TFTR Becca! I addressed all of your comments. I also addressed comments that @andy-kimball provided offline.

This led to me pull the locking clauses out of scope and passing them on the stack instead. Andy's test case which made it most clear to me that passing locking around on the scope was incorrect was the following:

SELECT (SELECT a FROM t) FOR UPDATE

When we were passing locking information on scope, this would naturally lead to the subquery using row-level locking. I assumed that's what was desired and this case was actually what drove me to use scope like this in the first place. It turns out that this isn't desired and that in PG, table t is not locked.

So the new approach is simpler and easier to reason about. We just pass the locking specification around to wherever it's needed. I added a commit to cleanup a few of the functions around SelectStmt construction so that we don't need to plumb this locking information all the way through buildStmt.

PTAL whenever possible.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft)

---

pkg/sql/logictest/testdata/logic_test/select_for_update, line 180 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

How about a test of FOR SHARE?

Done.

---

pkg/sql/opt/optbuilder/scope.go, line 1315 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] specifies -> specified

Removed.

---

pkg/sql/opt/optbuilder/scope.go, line 1324 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Why do you need the check if len(li.Targets) == 0 here? Seems like that's already checked below.

Removed.

---

pkg/sql/opt/optbuilder/select.go, line 59 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

As far as I can tell, the parser doesn't directly return tree.TableNames, but it does return tree.TableRefs (numeric table references).

Huh, I guess that makes sense, but it's certainly not what I was expecting.

---

pkg/sql/opt/optbuilder/select.go, line 126 at r3 (raw file):
That's how it works in Postgres for some reason. In https://www.postgresql.org/docs/12/sql-select.html:

The use of FOR NO KEY UPDATE, FOR UPDATE, FOR SHARE or FOR KEY SHARE requires UPDATE privilege as well

I tested it out as well and that's how it seemed to work.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 74 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is it a problem that we are dropping this locking request? How does Postgres handle this?

Postgres throws an error. It's not a huge problem but I do have a pair of TODOs to try to fix it.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 151 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

What about SELECT * FROM v FOR UPDATE OF t? How should that behave?

Added a test for that.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 261 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

How about adding a test where a stronger locking from outside overrides a weaker locking inside a subquery?

Done.

---

pkg/sql/sem/tree/select.go, line 1063 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

weight -> wait

Done.

[andy-kimball]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @rytaft)

---

pkg/sql/opt/memo/interner.go, line 883 at r6 (raw file):

		return l == r
	}
	return l.Strength == r.Strength && l.WaitPolicy == r.WaitPolicy

Better here would be *l == *r. If other fields got added to the struct, it would prevent a bug from manifesting if this code wasn't updated.

---

pkg/sql/opt/optbuilder/locking.go, line 94 at r6 (raw file):

		return
	}
	*lm = append(*lm, locking...)

I think this could mutate the AST slice in case where the existing lockingSpec comes from the AST. We want to keep the AST immutable. So instead something like:

l := len(*lm)
*lm = append((*lm)[:l:l], locking...)

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 10 at r7 (raw file):

exec-ddl
CREATE VIEW v AS SELECT a FROM t

Should make this a bit trickier by aliasing t as t2 or similar.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 91 at r7 (raw file):

build
SELECT * FROM t AS t2 FOR UPDATE OF t

This test should fail with an error, since t is not in-scope.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 123 at r7 (raw file):

build
SELECT * FROM [53 AS t] FOR UPDATE OF t2

Should be an error case. Perhaps the right way to handle these cases is by doing a FOR UPDATE target scoping check after processing the FROM clause, for sole purpose of triggering errors.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 159 at r7 (raw file):

build
SELECT * FROM v FOR UPDATE OF t

As above, this should be an error case. I'll stop calling out cases that should be errors from here on out...

I'd also add another case like this, where I try to use t2, the alias of t in the view:

SELECT * FROM v FOR UPDATE of t2

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 241 at r7 (raw file):

# TODO(nvanbenschoten): To match Postgres perfectly, this would throw an error.
# It's not clear that it's worth going out of our way to mirror that behavior.

I think it's worth it because it's confusing to silently make FOR UPDATE a no-op. This syntax looks like it should work, but doesn't, and that's not a great user experience.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 493 at r7 (raw file):

# Tests with common-table expressions.
#
# Unlike with subqueries, row-level locking clauses do not apply to WITH queries

Unlike with FROM subqueries, perhaps?

[nvanbenschoten]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @rytaft)

---

pkg/sql/opt/memo/interner.go, line 883 at r6 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Better here would be *l == *r. If other fields got added to the struct, it would prevent a bug from manifesting if this code wasn't updated.

tree.LockingItem isn't comparable because it contains a slice (Targets). The slice is guaranteed to be empty (see documentation on ScanPrivate), but it still prevents us from doing *l == *r. An alternative approach would be to define a new type and to not use tree.LockingItem directly in ScanPrivate. I think that's overkill and also comes with its own set of downsides related to leaving room for bugs if anything changes here, but I'd be interested in your take.

---

pkg/sql/opt/optbuilder/locking.go, line 94 at r6 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I think this could mutate the AST slice in case where the existing lockingSpec comes from the AST. We want to keep the AST immutable. So instead something like:

l := len(*lm)
*lm = append((*lm)[:l:l], locking...)

Good point, done.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 10 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Should make this a bit trickier by aliasing t as t2 or similar.

Done.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 91 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

This test should fail with an error, since t is not in-scope.

See below.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 123 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Should be an error case. Perhaps the right way to handle these cases is by doing a FOR UPDATE target scoping check after processing the FROM clause, for sole purpose of triggering errors.

Discussed on Slack. There was a TODO at the end of validateLockingInFrom to address this. We already had access to fromScope in validateLockingInFrom, but it wasn't immediately clear what the best approach for checking the target scoping would be. I was hoping that scope would carry information about the relations it contains, but that isn't the case. scope does, however, contain information about the columns it contains. Using this information, I was able to address the TODO in a follow-up commit. PTAL at that approach and let me know if you think there's a better way to accomplish what it's trying to accomplish.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 159 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

As above, this should be an error case. I'll stop calling out cases that should be errors from here on out...

I'd also add another case like this, where I try to use t2, the alias of t in the view:

SELECT * FROM v FOR UPDATE of t2

Done.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 241 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

I think it's worth it because it's confusing to silently make FOR UPDATE a no-op. This syntax looks like it should work, but doesn't, and that's not a great user experience.

See the discussion above.

---

pkg/sql/opt/optbuilder/testdata/select_for_update, line 493 at r7 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Unlike with FROM subqueries, perhaps?

Done.

[andy-kimball]

, nice work

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @rytaft)

---

pkg/sql/opt/memo/interner.go, line 883 at r6 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

tree.LockingItem isn't comparable because it contains a slice (Targets). The slice is guaranteed to be empty (see documentation on ScanPrivate), but it still prevents us from doing *l == *r. An alternative approach would be to define a new type and to not use tree.LockingItem directly in ScanPrivate. I think that's overkill and also comes with its own set of downsides related to leaving room for bugs if anything changes here, but I'd be interested in your take.

I see, makes sense.

[nvanbenschoten]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)

[rytaft]

Sorry for the delay,

Just a few typos in comments -- feel free to ignore or address in a future PR

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @rytaft)

---

pkg/sql/opt/optbuilder/locking.go, line 98 at r9 (raw file):

}

// filter returns the desired row-level locking mode for the specifies table as

specifies -> specified

---

pkg/sql/opt/optbuilder/locking.go, line 162 at r9 (raw file):

// current scope to the CTE. This mirrors Postgres' behavior. It also avoids a
// number of awkward questions like how row-level locking would interact with
// mutating commong table expressions.

commong -> common

---

pkg/sql/opt/optbuilder/select.go, line 138 at r9 (raw file):

	case *tree.Subquery:
		// Remove any target relations from the current scope's locking spec, as
		// those only apply to relations in this statements. Interestingly, this

statements -> statement

---

pkg/sql/opt/optbuilder/select.go, line 1232 at r9 (raw file):

			// columns in scope for each of the locking targets. We could use a
			// more efficient data structure (e.g. a hash map of relation names)
			// to improve the complexity here, but we don't expect the number of

Did you mean "we expect" instead of "we don't expect"?

---

pkg/sql/opt/optbuilder/select.go, line 1262 at r9 (raw file):

// raiseLockingContextError raises an error indicating that a row-level locking
// clause is not permitted in the specified context. locking.set must be true.

locking.set -> locking.isSet()

[nvanbenschoten]

There wasn't a delay - I should have waited for your stamp as well. I addressed you comments in #44169. Thanks for the review!

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @rytaft)

---

pkg/sql/opt/optbuilder/locking.go, line 98 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

specifies -> specified

Done.

---

pkg/sql/opt/optbuilder/locking.go, line 162 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

commong -> common

Done.

---

pkg/sql/opt/optbuilder/select.go, line 138 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

statements -> statement

Done.

---

pkg/sql/opt/optbuilder/select.go, line 1232 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Did you mean "we expect" instead of "we don't expect"?

Done.

---

pkg/sql/opt/optbuilder/select.go, line 1262 at r9 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

locking.set -> locking.isSet()

Done.