optbuilder: disallow locking tables on null-extended side of outer join

[rytaft]

This commit disallows locking tables on the null-extended side of outer joins. It does so by adding a new isNullExtended field to the lockingContext in the optbuilder.

Fixes #97434

Release note (bug fix): Locking tables (e.g., with SELECT ... FOR UPDATE) on the null-extended side of outer joins (e.g., the right side of a LEFT JOIN) is now disallowed and returns an error. This improves compatibility with Postgres and prevents ambiguity in locking semantics. This bug has existed since locking with FOR UPDATE was introduced.

[cockroach-teamcity]

This change is 

[DrewKimball]

Here's a potentially interesting test case:

postgres=# create table xy (x int primary key, y int);
CREATE TABLE
postgres=# create table ab (a int primary key, b int);
CREATE TABLE
postgres=# select * from xy left join ab on x = a for update;
ERROR:  FOR UPDATE cannot be applied to the nullable side of an outer join
postgres=# select * from xy left join ab on x = a where a is not null for update;
 x | y | a | b
---+---+---+---
(0 rows)

postgres=# explain select * from xy left join ab on x = a for update;
ERROR:  FOR UPDATE cannot be applied to the nullable side of an outer join
postgres=# explain select * from xy left join ab on x = a where a is not null for update;
                               QUERY PLAN
-------------------------------------------------------------------------
 LockRows  (cost=60.71..121.75 rows=2249 width=28)
   ->  Hash Join  (cost=60.71..99.26 rows=2249 width=28)
         Hash Cond: (xy.x = ab.a)
         ->  Seq Scan on xy  (cost=0.00..32.60 rows=2260 width=14)
         ->  Hash  (cost=32.60..32.60 rows=2249 width=14)
               ->  Seq Scan on ab  (cost=0.00..32.60 rows=2249 width=14)
                     Filter: (a IS NOT NULL)
(7 rows)

Does this mean postgres only cares about null extension if the nulls "make it" to the lock?

[mgartner]

Does this mean postgres only cares about null extension the nulls "make it" to the lock?

Isn't that because that left join is converted to an inner join due to the a is not null filter?

[mgartner]

But @michae2 should also take a look.

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

[DrewKimball]

Isn't that because that left join is converted to an inner join due to the a is not null filter?

Probably, but it means they check after optimization has occurred or after not null properties have been calculated, rather than the AST directly.

[rytaft]

TFTRs! @DrewKimball I added the test case -- nice find! I'm inclined to just leave it as a TODO for now, though, since I think matching Postgres' behavior in this case would add a lot more complexity (unless you have an idea about how to do this cleanly...).

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

[DrewKimball]

@DrewKimball I added the test case -- nice find! I'm inclined to just leave it as a TODO for now, though, since I think matching Postgres' behavior in this case would add a lot more complexity (unless you have an idea about how to do this cleanly...).

Maybe we could check if the primary key columns are nullable when building the lock operator? I agree that this is probably fine for now, though.

[mgartner]

I'm inclined to just leave it as a TODO for now, though, since I think matching Postgres' behavior in this case would add a lot more complexity (unless you have an idea about how to do this cleanly...).

I'm fine leaving as TODO, but is there a reason the check cannot be moved from buildDataSource to after constructJoin is called in buildJoin? The join coming out of constructJoin should be normalized to an inner join.

[rytaft]

Maybe we could check if the primary key columns are nullable when building the lock operator?

I tried this and unfortunately it doesn't fix the test case you found (it also breaks a few other cases). Even if it did work, it makes me uneasy since it feels like this approach would violate the don't inspect what you build principle of optbuilder.

I added this code to lockingItem.validate() (after passing in the scope):

notNullCols := outScope.expr.Relational().NotNullCols  
for _, lb := range item.builders {  
  for _, col := range lb.keyCols {  
    if !notNullCols.Contains(col) {  
      panic(pgerror.Newf(  
        pgcode.FeatureNotSupported, "%s cannot be applied to the nullable side of an outer join",  
        li.Strength))  
    }  
  }  
}

Maybe I'm missing something?

is there a reason the check cannot be moved from buildDataSource to after constructJoin is called in buildJoin? The join coming out of constructJoin should be normalized to an inner join.

I don't think this would work, since we don't actually know if there should be an error until we get down to the data source and determine that the lock is set. That's why we need the isNullExtended field in the lockCtx. If we wanted this to work, we'd need some way to propagate the error back up to the original join that set the isNullExtended flag (which might not be the join immediately above the data source with the error).

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

[DrewKimball]

I tried this and unfortunately it doesn't fix the test case you found (it also breaks a few other cases). Even if it did work, it makes me uneasy since it feels like this approach would violate the don't inspect what you build principle of optbuilder.

I see, that's too bad. I agree we should respect the invariant, and it seems ok to be slightly more strict than postgres. Thanks for trying this out!

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @michae2)

[mgartner]

Ahh, thanks for the explanation and sorry for sending you down that rabbit hole!

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

[rytaft]

No worries, thanks to you both for the suggestions! Always good to try to avoid discrepancies with Postgres if possible...

bors r+

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @michae2)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[michae2]

Reviewable status: complete! 2 of 0 LGTMs obtained

---

pkg/sql/opt/optbuilder/select.go line 270 at r2 (raw file):

		lockCtx.filter(source.As.Alias)
		if lockCtx.locking.isSet() {

Oh, I just realized: we also need to check lockCtx.isNullExtended here to disallow statements like:

CREATE TABLE a (a INT);
SELECT 'a'::regclass::int;
SELECT * FROM [104 a1] LEFT JOIN [104 a2] USING (a) FOR UPDATE;

[mgartner]

Nice catch @michae2!

[rytaft]

Thanks for the catch! I'll open a new PR shortly to fix.