Merge pull request #115878 from cockroachdb/blathers/backport-release…

…-23.2-115795 release-23.2: optbuilder: disallow locking tables on null-extended side of outer join (#115878) Co-Authored-By: Rebecca Taft <becca@cockroachlabs.com>
cockroachdb · Dec 13, 2023 · b879209 · b879209
2 parents 22dc596 + 9c35e4a
commit b879209
Show file tree

Hide file tree

Showing 4 changed files with 253 additions and 5 deletions.
diff --git a/pkg/sql/opt/optbuilder/join.go b/pkg/sql/opt/optbuilder/join.go
@@ -33,9 +33,13 @@ import (
 func (b *Builder) buildJoin(
 	join *tree.JoinTableExpr, lockCtx lockingContext, inScope *scope,
 ) (outScope *scope) {
-	// TODO(michae2, 97434): Poison the lockCtx for the null-extended side(s) if
-	// this is an outer join.
-	leftScope := b.buildDataSource(join.Left, nil /* indexFlags */, lockCtx, inScope)
+	joinType := descpb.JoinTypeFromAstString(join.JoinType)
+	leftLockCtx := lockCtx
+	// Poison the lockCtx for the null-extended side(s) if this is an outer join.
+	if joinType == descpb.RightOuterJoin || joinType == descpb.FullOuterJoin {
+		leftLockCtx.isNullExtended = true
+	}
+	leftScope := b.buildDataSource(join.Left, nil /* indexFlags */, leftLockCtx, inScope)
 
 	inScopeRight := inScope
 	isLateral := b.exprIsLateral(join.Right)
@@ -47,12 +51,16 @@ func (b *Builder) buildJoin(
 		inScopeRight.context = exprKindLateralJoin
 	}
 
-	rightScope := b.buildDataSource(join.Right, nil /* indexFlags */, lockCtx, inScopeRight)
+	rightLockCtx := lockCtx
+	// Poison the lockCtx for the null-extended side(s) if this is an outer join.
+	if joinType == descpb.LeftOuterJoin || joinType == descpb.FullOuterJoin {
+		rightLockCtx.isNullExtended = true
+	}
+	rightScope := b.buildDataSource(join.Right, nil /* indexFlags */, rightLockCtx, inScopeRight)
 
 	// Check that the same table name is not used on both sides.
 	b.validateJoinTableNames(leftScope, rightScope)
 
-	joinType := descpb.JoinTypeFromAstString(join.JoinType)
 	var flags memo.JoinFlags
 	switch join.Hint {
 	case "":

diff --git a/pkg/sql/opt/optbuilder/locking.go b/pkg/sql/opt/optbuilder/locking.go
@@ -154,6 +154,12 @@ type lockingContext struct {
 	// either because they did not have a target or because one of their targets
 	// matched an ancestor of this scope.
 	locking lockingSpec
+
+	// isNullExtended is set to true if this lockingContext is being passed down
+	// to the null-extended side of an outer join. This is needed so that we can
+	// return an error if the locking is set when we are building a table scan and
+	// isNullExtended is true.
+	isNullExtended bool
 }
 
 // noLocking indicates that no row-level locking has been specified.
@@ -216,6 +222,11 @@ func (lockCtx *lockingContext) filter(alias tree.Name) {
 // cannot be applied. Already applied locking items remain applied.
 func (lockCtx *lockingContext) withoutTargets() {
 	lockCtx.lockScope = append(lockCtx.lockScope, &blankLockingScope)
+	// Reset isNullExtended if no lock applies at this point, since we shouldn't
+	// throw an error if locking is introduced lower in the plan tree.
+	if !lockCtx.locking.isSet() {
+		lockCtx.isNullExtended = false
+	}
 }
 
 // ignoreLockingForCTE is a placeholder for the following comment:

diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go
@@ -139,6 +139,13 @@ func (b *Builder) buildDataSource(
 		ds, depName, resName := b.resolveDataSource(tn, privilege.SELECT)
 		lockCtx.filter(tn.ObjectName)
 		if lockCtx.locking.isSet() {
+			// If this table was on the null-extended side of an outer join, we are not
+			// allowed to lock it.
+			if lockCtx.isNullExtended {
+				panic(pgerror.Newf(
+					pgcode.FeatureNotSupported, "%s cannot be applied to the nullable side of an outer join",
+					lockCtx.locking.get().Strength))
+			}
 			// SELECT ... FOR [KEY] UPDATE/SHARE also requires UPDATE privileges.
 			b.checkPrivilege(depName, ds, privilege.UPDATE)
 		}

diff --git a/pkg/sql/opt/optbuilder/testdata/select_for_update b/pkg/sql/opt/optbuilder/testdata/select_for_update
@@ -1670,6 +1670,228 @@ lock u
                           └── filters
                                └── t.a:1 = u.a:5
 
+# ------------------------------------------------------------------------------
+# Tests with outer joins.
+# ------------------------------------------------------------------------------
+
+build
+SELECT * FROM t LEFT JOIN u ON b = c FOR UPDATE
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t LEFT JOIN u ON b = c FOR UPDATE of t
+----
+project
+ ├── columns: a:1!null b:2 a:5 c:6
+ └── left-join (hash)
+      ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 u.a:5 c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      ├── scan t
+      │    ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      │    └── locking: for-update
+      ├── scan u
+      │    └── columns: u.a:5!null c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      └── filters
+           └── b:2 = c:6
+
+build
+SELECT * FROM t LEFT JOIN u ON b = c FOR UPDATE of u
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t RIGHT JOIN u ON b = c FOR UPDATE
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t RIGHT JOIN u ON b = c FOR UPDATE of t
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t RIGHT JOIN u ON b = c FOR UPDATE of u
+----
+project
+ ├── columns: a:1 b:2 a:5!null c:6
+ └── right-join (hash)
+      ├── columns: t.a:1 b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 u.a:5!null c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      ├── scan t
+      │    └── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      ├── scan u
+      │    ├── columns: u.a:5!null c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      │    └── locking: for-update
+      └── filters
+           └── b:2 = c:6
+
+build
+SELECT * FROM t FULL JOIN u ON b = c FOR UPDATE
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t FULL JOIN u ON b = c FOR UPDATE of t
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t FULL JOIN u ON b = c FOR UPDATE of u
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t LEFT JOIN u ON b = c FOR KEY SHARE
+----
+error (0A000): FOR KEY SHARE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t RIGHT JOIN u ON b = c FOR SHARE
+----
+error (0A000): FOR SHARE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t FULL JOIN u ON b = c FOR NO KEY UPDATE
+----
+error (0A000): FOR NO KEY UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t LEFT JOIN (SELECT t.a FROM t JOIN u ON b = c) j ON j.a = t.a FOR UPDATE
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+# Since the locking is happening below the join, this query should not error.
+build
+SELECT * FROM t LEFT JOIN (SELECT t.a FROM t JOIN u ON b = c FOR UPDATE) j ON j.a = t.a
+----
+project
+ ├── columns: a:1!null b:2 a:5
+ └── left-join (hash)
+      ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 t.a:5
+      ├── scan t
+      │    └── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      ├── project
+      │    ├── columns: t.a:5!null
+      │    └── inner-join (hash)
+      │         ├── columns: t.a:5!null b:6!null t.crdb_internal_mvcc_timestamp:7 t.tableoid:8 u.a:9!null c:10!null u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         ├── scan t
+      │         │    ├── columns: t.a:5!null b:6 t.crdb_internal_mvcc_timestamp:7 t.tableoid:8
+      │         │    └── locking: for-update
+      │         ├── scan u
+      │         │    ├── columns: u.a:9!null c:10 u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         │    └── locking: for-update
+      │         └── filters
+      │              └── b:6 = c:10
+      └── filters
+           └── t.a:5 = t.a:1
+
+# Since the locking is happening below the join, this query should not error.
+build
+SELECT * FROM t RIGHT JOIN (SELECT t.a FROM t JOIN u ON b = c FOR UPDATE) j ON j.a = t.a
+----
+project
+ ├── columns: a:1 b:2 a:5!null
+ └── right-join (hash)
+      ├── columns: t.a:1 b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 t.a:5!null
+      ├── scan t
+      │    └── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      ├── project
+      │    ├── columns: t.a:5!null
+      │    └── inner-join (hash)
+      │         ├── columns: t.a:5!null b:6!null t.crdb_internal_mvcc_timestamp:7 t.tableoid:8 u.a:9!null c:10!null u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         ├── scan t
+      │         │    ├── columns: t.a:5!null b:6 t.crdb_internal_mvcc_timestamp:7 t.tableoid:8
+      │         │    └── locking: for-update
+      │         ├── scan u
+      │         │    ├── columns: u.a:9!null c:10 u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         │    └── locking: for-update
+      │         └── filters
+      │              └── b:6 = c:10
+      └── filters
+           └── t.a:5 = t.a:1
+
+build
+SELECT * FROM t LEFT JOIN (SELECT t.a FROM t JOIN u ON b = c) j ON j.a = t.a FOR UPDATE of t
+----
+project
+ ├── columns: a:1!null b:2 a:5
+ └── left-join (hash)
+      ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 t.a:5
+      ├── scan t
+      │    ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      │    └── locking: for-update
+      ├── project
+      │    ├── columns: t.a:5!null
+      │    └── inner-join (hash)
+      │         ├── columns: t.a:5!null b:6!null t.crdb_internal_mvcc_timestamp:7 t.tableoid:8 u.a:9!null c:10!null u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         ├── scan t
+      │         │    └── columns: t.a:5!null b:6 t.crdb_internal_mvcc_timestamp:7 t.tableoid:8
+      │         ├── scan u
+      │         │    └── columns: u.a:9!null c:10 u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         └── filters
+      │              └── b:6 = c:10
+      └── filters
+           └── t.a:5 = t.a:1
+
+build
+SELECT * FROM t LEFT JOIN (SELECT t.a FROM t JOIN u ON b = c) j ON j.a = t.a FOR UPDATE of j
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
+build
+SELECT * FROM t LEFT JOIN (SELECT t.a FROM t LEFT JOIN u ON b = c FOR UPDATE of t) j ON j.a = t.a FOR UPDATE of t
+----
+project
+ ├── columns: a:1!null b:2 a:5
+ └── left-join (hash)
+      ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 t.a:5
+      ├── scan t
+      │    ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      │    └── locking: for-update
+      ├── project
+      │    ├── columns: t.a:5!null
+      │    └── left-join (hash)
+      │         ├── columns: t.a:5!null b:6 t.crdb_internal_mvcc_timestamp:7 t.tableoid:8 u.a:9 c:10 u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         ├── scan t
+      │         │    ├── columns: t.a:5!null b:6 t.crdb_internal_mvcc_timestamp:7 t.tableoid:8
+      │         │    └── locking: for-update
+      │         ├── scan u
+      │         │    └── columns: u.a:9!null c:10 u.crdb_internal_mvcc_timestamp:11 u.tableoid:12
+      │         └── filters
+      │              └── b:6 = c:10
+      └── filters
+           └── t.a:5 = t.a:1
+
+build
+SELECT * FROM (SELECT t.a FROM t LEFT JOIN u ON b = c FOR UPDATE of t) j RIGHT JOIN t ON j.a = t.a FOR UPDATE of t
+----
+project
+ ├── columns: a:1 a:9!null b:10
+ └── right-join (hash)
+      ├── columns: t.a:1 t.a:9!null b:10 t.crdb_internal_mvcc_timestamp:11 t.tableoid:12
+      ├── project
+      │    ├── columns: t.a:1!null
+      │    └── left-join (hash)
+      │         ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4 u.a:5 c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      │         ├── scan t
+      │         │    ├── columns: t.a:1!null b:2 t.crdb_internal_mvcc_timestamp:3 t.tableoid:4
+      │         │    └── locking: for-update
+      │         ├── scan u
+      │         │    └── columns: u.a:5!null c:6 u.crdb_internal_mvcc_timestamp:7 u.tableoid:8
+      │         └── filters
+      │              └── b:2 = c:6
+      ├── scan t
+      │    ├── columns: t.a:9!null b:10 t.crdb_internal_mvcc_timestamp:11 t.tableoid:12
+      │    └── locking: for-update
+      └── filters
+           └── t.a:1 = t.a:9
+
+# TODO(rytaft): Postgres does not error in this case because it determines that
+# the null-extended rows are filtered out.
+build
+SELECT * FROM t LEFT JOIN u ON b = c WHERE c IS NOT NULL FOR UPDATE
+----
+error (0A000): FOR UPDATE cannot be applied to the nullable side of an outer join
+
 # ------------------------------------------------------------------------------
 # Tests with joins of aliased tables and aliased joins.
 # ------------------------------------------------------------------------------