Merge #159001 #159017

159001: opt: fix placeholder scan NULL semantics r=mgartner a=mgartner

#### logictest: fix "query empty" directive

This commit fixes a bug in logictest's `query empty` directive that
caused a failing test, i.e., one that returns non-empty results, to
result in a index-out-of-bounds panic instead of the clear assertion
failure "expected empty result, found X rows".

Release note: None

#### opt: fix placeholder scan NULL semantics

Fixes #158945

Release note (bug fix): A bug has been fixed which could cause incorrect
results. The bug has existed since v21.2. From v21.2 up to v25.3, the
bug only presented when all of the following were true:
  - The query was run with an explicit or implicit prepared statement.
  - The query had an equality filter on a placeholder and a UNIQUE
    column.
  - The column contained NULL values.
  - The placeholder was assigned to NULL during execution.
In this case, the query could return rows in which the column's value is
NULL, which violates SQL NULL-equality semantics. The correct result set
should always be empty.
Starting in v25.4, the requirements were loosened slightly. It was no
longer necessary for the column to be UNIQUE. The bug could reproduce if
the column was included in any index.

#### opt: refactor placeholder scan execbuilding

This is a purely mechanical change that moves a block of code in
`buildPlaceholderScan`. An early exit now occurs earlier, avoiding
unnecessary computation.

Release note: None


159017: sql/sem/tree: prevent schema_locked bypass with comma syntax r=rafiss a=rafiss

Previously, the legacy schema changer allowed bypassing the schema_locked protection by combining `SET (schema_locked=false)` with other schema-changing commands in the same ALTER TABLE statement. For example:

```sql
ALTER TABLE t SET (schema_locked=false), DROP COLUMN n;
```

This would first unset schema_locked, then execute the DROP COLUMN on the now-unlocked table, effectively circumventing the protection.

The fix ensures `IsSetOrResetSchemaLocked()` returns `false` when the ALTER TABLE statement contains any commands other than storage parameter changes. This prevents schema_locked changes from being combined with other schema changes in the same statement.

Fixes: #150484

Release note (bug fix): Fixed a bug where the `schema_locked` table storage parameter could be bypassed by combining `SET (schema_locked=false)` with other schema changes in the same `ALTER TABLE` statement using comma syntax. Schema-locked tables now correctly reject such combined statements.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

Author: 3 people

pkg/sql/logictest/logic.go

@@ -4010,6 +4010,13 @@ func (t *logicTest) finishExecQuery(query logicQuery, rowses []*gosql.Rows, exec
 					}
 
 					rowCount++
+
+					if query.empty {
+						// Skip column assertions if we are expecting an empty
+						// result.
+						continue
+					}
+
 					for i, v := range vals {
 						colT := query.colTypes[i]
 						// Ignore column - useful for non-deterministic output.

pkg/sql/logictest/testdata/logic_test/generic

@@ -622,3 +622,71 @@ quality of service: regular
 
 statement ok
 DEALLOCATE p
+
+# Regression test for #158945. Placeholder scans should correctly handle NULL
+# placeholder values.
+statement ok
+CREATE TABLE t158945 (
+  k INT PRIMARY KEY,
+  a INT,
+  b INT,
+  UNIQUE (a),
+  INDEX (b)
+)
+
+statement ok
+INSERT INTO t158945 VALUES (1, NULL, NULL)
+
+statement ok
+PREPARE p158945a AS SELECT k FROM t158945 WHERE a = $1
+
+query empty
+EXECUTE p158945a(NULL)
+
+query T
+EXPLAIN ANALYZE EXECUTE p158945a(NULL)
+----
+planning time: 10µs
+execution time: 100µs
+distribution: <hidden>
+vectorized: <hidden>
+plan type: generic, reused
+maximum memory usage: <hidden>
+DistSQL network usage: <hidden>
+regions: <hidden>
+isolation level: serializable
+priority: normal
+quality of service: regular
+·
+• norows
+  sql nodes: <hidden>
+  regions: <hidden>
+  actual row count: 0
+  execution time: 0µs
+
+statement ok
+PREPARE p158945b AS SELECT k FROM t158945 WHERE a = $1
+
+query empty
+EXECUTE p158945b(NULL)
+
+query T
+EXPLAIN ANALYZE EXECUTE p158945b(NULL)
+----
+planning time: 10µs
+execution time: 100µs
+distribution: <hidden>
+vectorized: <hidden>
+plan type: generic, reused
+maximum memory usage: <hidden>
+DistSQL network usage: <hidden>
+regions: <hidden>
+isolation level: serializable
+priority: normal
+quality of service: regular
+·
+• norows
+  sql nodes: <hidden>
+  regions: <hidden>
+  actual row count: 0
+  execution time: 0µs

pkg/sql/logictest/testdata/logic_test/schema_locked

@@ -218,3 +218,45 @@ t_147993  CREATE TABLE public.t_147993 (
           ) WITH (schema_locked = true);
 
 subtest end
+
+# Regression test for #150484: schema_locked cannot be unset with the comma
+# syntax combined with other schema-changing operations. This prevents bypassing
+# the schema_locked protection by unsetting the lock and performing a schema
+# change in the same ALTER TABLE statement.
+subtest regression_150484
+
+statement ok
+CREATE TABLE t_150484 (n INT, m INT, FAMILY (n, m)) WITH (schema_locked=true);
+
+# Combining SET (schema_locked=false) with DROP COLUMN should fail.
+statement error pgcode 57000 pq: this schema change is disallowed because table "t_150484" is locked
+ALTER TABLE t_150484 SET (schema_locked=false), DROP COLUMN n;
+
+# Combining RESET (schema_locked) with ADD COLUMN should fail.
+statement error pgcode 57000 pq: this schema change is disallowed because table "t_150484" is locked
+ALTER TABLE t_150484 RESET (schema_locked), ADD COLUMN k INT;
+
+# Verify the table is still locked and unchanged.
+query TT
+SHOW CREATE TABLE t_150484
+----
+t_150484  CREATE TABLE public.t_150484 (
+            n INT8 NULL,
+            m INT8 NULL,
+            rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+            CONSTRAINT t_150484_pkey PRIMARY KEY (rowid ASC),
+            FAMILY fam_0_n_m_rowid (n, m, rowid)
+          ) WITH (schema_locked = true);
+
+# Confirm that SET (schema_locked=false) alone still works.
+statement ok
+ALTER TABLE t_150484 SET (schema_locked=false);
+
+# Now schema changes should work.
+statement ok
+ALTER TABLE t_150484 DROP COLUMN n;
+
+statement ok
+DROP TABLE t_150484;
+
+subtest end

pkg/sql/opt/exec/execbuilder/relational.go

@@ -972,6 +972,33 @@ func (b *Builder) buildPlaceholderScan(
 		return execPlan{}, colOrdMap{}, errors.AssertionFailedf("PlaceholderScan cannot have constraints")
 	}
 
+	// Evaluate the scalar expressions.
+	values := make([]tree.Datum, len(scan.Span))
+	for i, expr := range scan.Span {
+		// The expression is either a placeholder or a constant.
+		var val tree.Datum
+		if p, ok := expr.(*memo.PlaceholderExpr); ok {
+			val, err = eval.Expr(b.ctx, b.evalCtx, p.Value)
+			if err != nil {
+				return execPlan{}, colOrdMap{}, err
+			}
+		} else {
+			val = memo.ExtractConstDatum(expr)
+		}
+		if val == tree.DNull {
+			// If any value is NULL, then build an empty values operator instead
+			// of a scan. No row can satisfy the equality filter that was used
+			// to build this placeholder scan, because of SQL NULL-equality
+			// semantics.
+			return b.buildValues(&memo.ValuesExpr{
+				ValuesPrivate: memo.ValuesPrivate{
+					Cols: scan.Cols.ToList(),
+				},
+			})
+		}
+		values[i] = val
+	}
+
 	md := b.mem.Metadata()
 	tab := md.Table(scan.Table)
 	idx := tab.Index(scan.Index)
@@ -988,20 +1015,6 @@ func (b *Builder) buildPlaceholderScan(
 	columns.Init(spanColumns)
 	keyCtx := constraint.MakeKeyContext(b.ctx, &columns, b.evalCtx)
 
-	values := make([]tree.Datum, len(scan.Span))
-	for i, expr := range scan.Span {
-		// The expression is either a placeholder or a constant.
-		if p, ok := expr.(*memo.PlaceholderExpr); ok {
-			val, err := eval.Expr(b.ctx, b.evalCtx, p.Value)
-			if err != nil {
-				return execPlan{}, colOrdMap{}, err
-			}
-			values[i] = val
-		} else {
-			values[i] = memo.ExtractConstDatum(expr)
-		}
-	}
-
 	key := constraint.MakeCompositeKey(values...)
 	var span constraint.Span
 	span.Init(key, constraint.IncludeBoundary, key, constraint.IncludeBoundary)

pkg/sql/opt/ops/relational.opt

@@ -107,9 +107,11 @@ define ScanPrivate {
 }
 
 # PlaceholderScan is a special variant of Scan. It scans exactly one span of a
-# non-inverted index, and the span has the same start and end key. The values
-# for the span key are child scalar operators which are either constants or
-# placeholders. This operator evaluates the placeholders at execbuild time.
+# non-inverted index, and the span has the same start and end key. The span key
+# is built at execbuild-time. Each scalar expression in the Span list
+# corresponds to a column in the index's key prefix, assuming SQL-equality
+# semantics (e.g., NULL is not equal to NULL). The scalar expressions are either
+# constants or placeholders. Placeholders are evaluated at execbuild-time.
 #
 # PlaceholderScan cannot have a Constraint or InvertedConstraint.
 [Relational]

pkg/sql/sem/tree/schema_helpers.go

@@ -7,26 +7,34 @@ package tree
 
 import "slices"
 
-// IsSetOrResetSchemaLocked returns true if `n` contains a command to
-// set/reset "schema_locked" storage parameter.
+// IsSetOrResetSchemaLocked returns true if `n` contains only commands to
+// set/reset storage parameters including "schema_locked", with no other
+// schema-changing commands. This prevents bypassing schema_locked protection
+// by combining SET (schema_locked=false) with other operations like DROP
+// COLUMN in the same ALTER TABLE statement.
 func IsSetOrResetSchemaLocked(n Statement) bool {
 	alterStmt, ok := n.(*AlterTable)
 	if !ok {
 		return false
 	}
+	hasSchemaLockedChange := false
 	for _, cmd := range alterStmt.Cmds {
 		switch cmd := cmd.(type) {
 		case *AlterTableSetStorageParams:
 			if cmd.StorageParams.GetVal("schema_locked") != nil {
-				return true
+				hasSchemaLockedChange = true
 			}
 		case *AlterTableResetStorageParams:
 			if slices.Contains(cmd.Params, "schema_locked") {
-				return true
+				hasSchemaLockedChange = true
 			}
+		default:
+			// Any other command type means this is not a pure storage parameter
+			// change, so we should not allow bypassing schema_locked checks.
+			return false
 		}
 	}
-	return false
+	return hasSchemaLockedChange
 }
 
 // IsAllowedLDRSchemaChange returns true if the schema change statement is