sql: parser and optimizer support FOR {UPDATE,SHARE} SKIP LOCKED #85720

rytaft · 2022-08-07T19:37:59Z

This PR adds support for SKIP LOCKED by building on commits from #83627 and #82188. See below for details.

sql: enable the skip-locked wait policy

Now that KV support this, we can pass the wait policy through.

Release note (sql change): SELECT ... FOR {UPDATE,SHARE} SKIP LOCKED
is now supported. The option can be used to skip rows that cannot be
immediately locked instead of blocking on contended row-level lock
acquisition.

opt: optimizer updates for support of SKIP LOCKED

For queries using SELECT FOR {SHARE,UPDATE} SKIP LOCKED, we need to disable
optimizations that depend on preserved-multiplicity consistency of
tables. When SKIP LOCKED is used, we will no longer use optimizations that
assume:

a PK row exists for every secondary index row
a PK row exists for every referencing FK (if the PK table uses SKIP LOCKED)

One result of this change is that we will no longer push limits into index
joins if the primary index uses locking wait policy SKIP LOCKED.

This commit also disallows use of multiple column families in tables scanned
with SKIP LOCKED, since it could result in returning partial rows.

Release note: None

Co-authored-by: Michael Erickson michae2@cockroachlabs.com

cockroach-teamcity · 2022-08-07T19:38:10Z

This change is

nvanbenschoten

, though I only skimmed the second and third commit.

Reviewed 6 of 6 files at r1, 13 of 13 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

michae2

Thank you for picking this up! This looks great!

Reviewed 6 of 6 files at r1, 13 of 13 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @rytaft)

mgartner

Don't forget to update the commit messages when you squash them - if I understand correctly, SKIP LOCKED only affects:

Pushing limits below index joins
Optimizations that rely on multiplicity and pushing limits

Reviewed 6 of 6 files at r1, 13 of 13 files at r2, 21 of 21 files at r3, all commit messages.
Reviewable status: complete! 3 of 0 LGTMs obtained (waiting on @rytaft)

pkg/sql/logictest/testdata/logic_test/select_for_update line 432 at r3 (raw file):


# All columns are available from the secondary index on u, so no index join is
# needed.

nit: It might be helpful to spell out that the secondary index (and lack of index join) can produce the row where k=2 instead of skipping it.

pkg/sql/opt/table_meta.go line 148 at r3 (raw file):

	// of this table (i.e. rules that assume there is always a PK row for every
	// secondary index row or a FK row for every FK reference).
	IsSkipLocked bool

It's regrettable that we need this bool in addition to the locking struct in ScanPrivate. But I see how it allows checkIsSkipLocked to avoid traversing the entire right subexpression. I don't see any way around it though, and I can't think of a better place to track this.

I initially thought that it could be better to include the set of skip-locked columns in relational properties - but I'm not so sure anymore. For one thing, that'd require more computation and space.

pkg/sql/opt/memo/multiplicity_builder.go line 92 at r3 (raw file):

		// If no rows are being removed from the table, add all its columns. No rows
		// are removed from a table if this is an un-limited, unconstrained scan
		// over a non-partial index.

nit: update this comment

pkg/sql/opt/memo/multiplicity_builder.go line 263 at r3 (raw file):

func filtersMatchAllLeftRows(left, right RelExpr, filters FiltersExpr) bool {
	if checkIsSkipLocked(right.Memo().Metadata(), right) {
		return false

nit: You could add unit tests for this in multiplicity_builder_test.go.

pkg/sql/opt/xform/groupby_funcs.go line 416 at r3 (raw file):

		// Otherwise, try to construct an IndexJoin operator that provides the
		// columns missing from the index.
		if sp.Flags.NoIndexJoin {

We should probably have tests for all these rules where we were forgetting to check NoIndexJoin, what do you think? You can do it in a follow-up PR if that's easier.

pkg/sql/opt/xform/index_scan_builder.go line 167 at r3 (raw file):

func (b *indexScanBuilder) AddIndexJoin(cols opt.ColSet) {
	if b.scanPrivate.Flags.NoIndexJoin {
		panic(errors.AssertionFailedf("attempt to create an index join with NoIndexJoin flag"))

nit: you could also add this in check_expr since there is at least one rule where an index join is created without using this function.

nvanbenschoten · 2022-08-09T14:39:25Z

Should we mark this as fixing #40476?

Now that KV support this, we can pass the wait policy through. Fixes cockroachdb#40476 Release note (sql change): SELECT ... FOR {UPDATE,SHARE} SKIP LOCKED is now supported. The option can be used to skip rows that cannot be immediately locked instead of blocking on contended row-level lock acquisition.

rytaft

TFTRs!

Don't forget to update the commit messages when you squash them

Done.

Should we mark this as fixing #40476?

Done 🥳

Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner, @michae2, and @nvanbenschoten)

pkg/sql/logictest/testdata/logic_test/select_for_update line 432 at r3 (raw file):