Merge #37791 #37914

37791: sql: use a CockroachDB-specific error code for schg errs upon COMMIT r=knz a=knz

This patch changes the error reported upon schema change failures
raised at COMMIT (multi-stmt txns) to a new CockroachDB-specific code
called "transaction committed with schema change failure", XXA00.

Previously, this situation used the extant PostgreSQL error code
"40003", "statement completion unknown", however that was misleading:
the situation is actually pretty clear and the statement is actually
known to have completed. The other situation
where CockroachDB produces "completion unknown" is when KV fails to
receive an acknowledgement for the txn commit.

Compare:

| Actual transaction status                      | Does the gateway know? | Error code reported to client                      |
|------------------------------------------------|------------------------|----------------------------------------------------|
| EITHER fully committed OR fully aborted        | no                     | "completion unknown"                               |
| BOTH partially committed AND partially aborted | yes                    | "transaction committed with schema change failure" |

Using a separate error code ensures the situation is more noticeable
and can be documented separately.


After this patch:

```
root@127.0.0.1:19343/defaultdb> create table t(x int); insert into t(x) values (0);
INSERT 1

root@127.0.0.1:19343/defaultdb> begin;
> alter table t add column z int default 123;
> insert into t(x) values (1);
> alter table t add column y float as (1::float / x::float) stored;
> commit;
pq: transaction committed but schema change aborted with error: division by zero
HINT: Some of the non-DDL statements may have committed successfully,
but some of the DDL statement(s) failed.
Manual inspection may be required to determine the actual state of the database.
```

(in this specific example, the ALTER failed but the INSERT was
committed. There are two rows in the table.)

Some context for the reader not familiar with this situation (I had to
reverse engineer the code to understand this, so I may as well write
this down here for posterity): in CockroachDB, a processing error
raised during the post-commit processing of schema changes is
"interesting" in that it really violates the "A" in ACID.

At that point:

- all the non-DDL effects of the txns are successfully committed (and
  cannot be rolled back any more);
- some the DDL up to the one that triggered the error may be committed
  (author note: I am unsure -- not investigating that);
- the last DDL that caused the error was aborted.

In effect, the transaction is both committed and aborted at the same
time.

Release note (sql change): CockroachDB now reports a dedicated SQL
error code in the particular situation where a multi-statement
transaction is COMMITTed but one of the schema change (DDL) operations
failed. As in previous versions, CockroachDB leaves the transaction
partly committed and partly aborted in that case; the new error code
aims to facilitate the recognition of this case in client code.

37914: opt: add PushSelectIntoWindow r=justinj a=justinj

This commit adds a rule to push select filters into a Window expression.
This is valid to do whenever a filter is bound by the partition columns
of the Window expression.

Release note: None

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>

pkg/sql/conn_executor.go

@@ -1959,17 +1959,27 @@ func (ex *connExecutor) txnStateTransitionsApplyWrapper(
 			if schemaChangeErr := scc.execSchemaChanges(
 				ex.Ctx(), ex.server.cfg, &ex.sessionTracing, ieFactory,
 			); schemaChangeErr != nil {
-				// We got a schema change error. We'll return it to the client as the
-				// result of the current statement - which is either the DDL statement or
-				// a COMMIT statement if the DDL was part of an explicit transaction. In
-				// the explicit transaction case, we return a funky error code to the
-				// client to seed fear about what happened to the transaction. The reality
-				// is that the transaction committed, but at least some of the staged
-				// schema changes failed. We don't have a good way to indicate this.
 				if implicitTxn {
+					// The schema change failed but it was also the only
+					// operation in the transaction. In this case, the
+					// transaction's error is the schema change error.
 					res.SetError(schemaChangeErr)
 				} else {
-					res.SetError(sqlbase.NewStatementCompletionUnknownError(schemaChangeErr))
+					// The schema change failed but everything else in the transaction
+					// was actually committed successfully already. At this point,
+					// it is too late to cancel the transaction. In effect, we have
+					// violated the "A" of ACID.
+					//
+					// This situation is sufficiently serious that we cannot let
+					// the error that caused the schema change to fail flow back
+					// to the client as-is. We replace it by a custom code
+					// dedicated to this situation.
+					newErr := pgerror.Newf(
+						pgerror.CodeTransactionCommittedWithSchemaChangeFailure,
+						"%v", schemaChangeErr).SetHintf(
+						"Some of the non-DDL statements may have committed successfully, but some of the DDL statement(s) failed.\n" +
+							"Manual inspection may be required to determine the actual state of the database.")
+					res.SetError(newErr)
 				}
 			}
 		}

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -596,7 +596,7 @@ ALTER TABLE add_default ADD fee FLOAT NOT NULL DEFAULT 2.99
 statement ok
 ALTER TABLE add_default ALTER COLUMN fee DROP DEFAULT
 
-statement error null value in column "fee" violates not-null constraint
+statement error pgcode XXA00 null value in column "fee" violates not-null constraint
 COMMIT
 
 statement error pgcode 42703 column "fee" does not exist

pkg/sql/logictest/testdata/logic_test/partial_txn_commit

@@ -0,0 +1,38 @@
+# LogicTest: local local-opt fakedist fakedist-metadata fakedist-opt
+
+# This test exercises the presence of an explanatory hint when a transaction
+# ends up partially committed and partially aborted.
+
+statement ok
+CREATE TABLE t (x INT); INSERT INTO t (x) VALUES (0);
+
+statement ok
+BEGIN
+
+statement ok
+ALTER TABLE t ADD COLUMN z INT DEFAULT 123
+
+statement ok
+INSERT INTO t (x) VALUES (1)
+
+statement ok
+ALTER TABLE t ADD COLUMN y FLOAT AS (1::FLOAT / x::FLOAT) STORED
+
+statement error pgcode XXA00 division by zero.*\nHINT:.*\nManual inspection may be required
+COMMIT
+
+# Verify that the txn was indeed partially committed: the INSERT succeeded.
+query I rowsort
+SELECT * FROM t
+----
+0
+1
+
+# Verify that the txn was indeed partially aborted: the first ALTER failed.
+query TT
+SHOW CREATE t
+----
+t  CREATE TABLE t (
+   x INT8 NULL,
+   FAMILY "primary" (x, rowid)
+)

pkg/sql/logictest/testdata/logic_test/schema_change_in_txn

@@ -732,7 +732,7 @@ CREATE UNIQUE INDEX i_idx ON customers (i)
 statement ok
 CREATE UNIQUE INDEX n_idx ON customers (n)
 
-statement error violates unique constraint
+statement error pgcode XXA00 violates unique constraint
 COMMIT
 
 query TTBTTTB
@@ -931,7 +931,7 @@ ALTER TABLE check_table ADD e INT DEFAULT 0
 statement ok
 ALTER TABLE check_table ADD CONSTRAINT e_0 CHECK (e > 0)
 
-statement error validation of CHECK "e > 0" failed on row: k=1, c=NULL, d=1, e=0
+statement error pgcode XXA00 validation of CHECK "e > 0" failed on row: k=1, c=NULL, d=1, e=0
 COMMIT
 
 # Test rollbacks after error in expression evaluation
@@ -944,7 +944,7 @@ ALTER TABLE check_table ADD e STRING DEFAULT 'a'
 statement ok
 ALTER TABLE check_table ADD CONSTRAINT e_0 CHECK (e::INT > 0)
 
-statement error validate check constraint: could not parse "a" as type int
+statement error pgcode XXA00 validate check constraint: could not parse "a" as type int
 COMMIT
 
 # Constraint e_0 was not added
@@ -984,7 +984,7 @@ CREATE UNIQUE INDEX idx ON check_table (a)
 statement ok
 ALTER TABLE check_table ADD CHECK (a >= 0)
 
-statement error violates unique constraint "idx"
+statement error pgcode XXA00 violates unique constraint "idx"
 COMMIT
 
 query TTTTB
@@ -1001,7 +1001,7 @@ ALTER TABLE check_table ADD CHECK (a >= 0)
 statement ok
 ALTER TABLE check_table ADD CHECK (a < 0)
 
-statement error validation of CHECK \"a < 0\" failed on row: k=0, a=0
+statement error pgcode XXA00 validation of CHECK \"a < 0\" failed on row: k=0, a=0
 COMMIT
 
 query TTTTB
@@ -1252,7 +1252,7 @@ ALTER TABLE t ADD CHECK (a < c)
 statement ok
 INSERT INTO t (a) VALUES (11)
 
-statement error validation of CHECK \"a < c\" failed on row: a=11, .* c=10
+statement error pgcode XXA00 validation of CHECK \"a < c\" failed on row: a=11, .* c=10
 COMMIT
 
 # Insert a pre-existing row to test updates
@@ -1271,7 +1271,7 @@ ALTER TABLE t ADD CHECK (a < c)
 statement ok
 UPDATE t SET a = 12 WHERE a < 12
 
-statement error validation of CHECK \"a < c\" failed on row: a=12, .*, c=10
+statement error pgcode XXA00 validation of CHECK \"a < c\" failed on row: a=12, .*, c=10
 COMMIT
 
 statement ok
@@ -1353,7 +1353,7 @@ ALTER TABLE t ADD CHECK (a < c)
 statement ok
 INSERT INTO t (a) VALUES (11)
 
-statement error validation of CHECK \"a < c\" failed on row: a=11, .* c=10
+statement error pgcode XXA00 validation of CHECK \"a < c\" failed on row: a=11, .* c=10
 COMMIT
 
 # Insert a pre-existing row to test updates
@@ -1372,7 +1372,7 @@ ALTER TABLE t ADD CHECK (a < c)
 statement ok
 UPDATE t SET a = 12 WHERE a < 12
 
-statement error validation of CHECK \"a < c\" failed on row: a=12, .*, c=11
+statement error pgcode XXA00 validation of CHECK \"a < c\" failed on row: a=12, .*, c=11
 COMMIT
 
 statement ok

pkg/sql/opt/norm/custom_funcs.go

@@ -235,6 +235,13 @@ func (c *CustomFuncs) IsBoundBy(src opt.Expr, cols opt.ColSet) bool {
 	return c.OuterCols(src).SubsetOf(cols)
 }
 
+// IsDeterminedBy returns true if all outer references in the source expression
+// are bound by the closure of the given columns according to the functional
+// dependencies of the input expression.
+func (c *CustomFuncs) IsDeterminedBy(src opt.Expr, cols opt.ColSet, input memo.RelExpr) bool {
+	return input.Relational().FuncDeps.InClosureOf(c.OuterCols(src), cols)
+}
+
 // IsCorrelated returns true if any variable in the source expression references
 // a column from the destination expression. For example:
 //   (InnerJoin
@@ -531,6 +538,35 @@ func (c *CustomFuncs) ExtractUnboundConditions(
 	return newFilters
 }
 
+// ExtractDeterminedConditions returns a new list of filters containing only
+// those expressions from the given list which are bound by columns which
+// are functionally determined by the given columns.
+func (c *CustomFuncs) ExtractDeterminedConditions(
+	filters memo.FiltersExpr, cols opt.ColSet, input memo.RelExpr,
+) memo.FiltersExpr {
+	newFilters := make(memo.FiltersExpr, 0, len(filters))
+	for i := range filters {
+		if c.IsDeterminedBy(&filters[i], cols, input) {
+			newFilters = append(newFilters, filters[i])
+		}
+	}
+	return newFilters
+}
+
+// ExtractUndeterminedConditions is the opposite of
+// ExtractDeterminedConditions.
+func (c *CustomFuncs) ExtractUndeterminedConditions(
+	filters memo.FiltersExpr, cols opt.ColSet, input memo.RelExpr,
+) memo.FiltersExpr {
+	newFilters := make(memo.FiltersExpr, 0, len(filters))
+	for i := range filters {
+		if !c.IsDeterminedBy(&filters[i], cols, input) {
+			newFilters = append(newFilters, filters[i])
+		}
+	}
+	return newFilters
+}
+
 // CanConsolidateFilters returns true if there are at least two different
 // filter conditions that contain the same variable, where the conditions
 // have tight constraints and contain a single variable. For example,
@@ -1122,6 +1158,12 @@ func (c *CustomFuncs) ReduceWindowPartitionCols(
 	return &p
 }
 
+// WindowPartition returns the set of columns that the window function uses to
+// partition.
+func (c *CustomFuncs) WindowPartition(priv *memo.WindowPrivate) opt.ColSet {
+	return priv.Partition
+}
+
 // ----------------------------------------------------------------------
 //
 // Boolean Rules

pkg/sql/opt/norm/rules/window.opt

@@ -44,3 +44,33 @@ $input
     $fn
     (SimplifyWindowOrdering $input $private)
 )
+
+# PushSelectIntoWindow pushes down a Select which can be satisfied by only the
+# functional closure of the columns being partitioned over. This is valid
+# because it's "all-or-nothing" - we only entirely eliminate a partition or
+# don't eliminate it at all.
+[PushSelectIntoWindow, Normalize]
+(Select
+    (Window
+      $input:*
+      $fn:*
+      $private:*
+    )
+    $filters:[
+        ...
+        $item:* & (IsDeterminedBy $item $partitionCols:(WindowPartition $private) $input)
+        ...
+    ]
+)
+=>
+(Select
+    (Window
+        (Select
+            $input
+            (ExtractDeterminedConditions $filters $partitionCols $input)
+        )
+        $fn
+        $private
+    )
+    (ExtractUndeterminedConditions $filters $partitionCols $input)
+)

pkg/sql/opt/norm/testdata/rules/decorrelate

@@ -296,34 +296,34 @@ WHERE i = 3
 ----
 project
  ├── columns: u:1(int!null) v:2(int) rank:8(int) i:4(int!null)
- ├── key: (1)
- ├── fd: ()-->(4), (1)-->(2,8)
- └── select
+ ├── key: (1,8)
+ ├── fd: ()-->(4), (1)-->(2)
+ └── window partition=(1)
       ├── columns: u:1(int!null) v:2(int) k:3(int!null) i:4(int!null) rank:8(int)
       ├── key: (3)
-      ├── fd: ()-->(4), (1)-->(2), (1)==(3), (3)==(1), (3)-->(8)
-      ├── window partition=(1)
-      │    ├── columns: u:1(int!null) v:2(int) k:3(int!null) i:4(int) rank:8(int)
+      ├── fd: ()-->(4), (1)-->(2), (1)==(3), (3)==(1)
+      ├── inner-join
+      │    ├── columns: u:1(int!null) v:2(int) k:3(int!null) i:4(int!null)
       │    ├── key: (3)
-      │    ├── fd: (1)-->(2), (3)-->(4), (1)==(3), (3)==(1)
-      │    ├── inner-join
-      │    │    ├── columns: u:1(int!null) v:2(int) k:3(int!null) i:4(int)
+      │    ├── fd: ()-->(4), (1)-->(2), (1)==(3), (3)==(1)
+      │    ├── scan uv
+      │    │    ├── columns: u:1(int!null) v:2(int)
+      │    │    ├── key: (1)
+      │    │    └── fd: (1)-->(2)
+      │    ├── select
+      │    │    ├── columns: k:3(int!null) i:4(int!null)
       │    │    ├── key: (3)
-      │    │    ├── fd: (1)-->(2), (3)-->(4), (1)==(3), (3)==(1)
-      │    │    ├── scan uv
-      │    │    │    ├── columns: u:1(int!null) v:2(int)
-      │    │    │    ├── key: (1)
-      │    │    │    └── fd: (1)-->(2)
+      │    │    ├── fd: ()-->(4)
       │    │    ├── scan a
       │    │    │    ├── columns: k:3(int!null) i:4(int)
       │    │    │    ├── key: (3)
       │    │    │    └── fd: (3)-->(4)
       │    │    └── filters
-      │    │         └── k = u [type=bool, outer=(1,3), constraints=(/1: (/NULL - ]; /3: (/NULL - ]), fd=(1)==(3), (3)==(1)]
-      │    └── windows
-      │         └── rank [type=undefined]
-      └── filters
-           └── i = 3 [type=bool, outer=(4), constraints=(/4: [/3 - /3]; tight), fd=()-->(4)]
+      │    │         └── i = 3 [type=bool, outer=(4), constraints=(/4: [/3 - /3]; tight), fd=()-->(4)]
+      │    └── filters
+      │         └── k = u [type=bool, outer=(1,3), constraints=(/1: (/NULL - ]; /3: (/NULL - ]), fd=(1)==(3), (3)==(1)]
+      └── windows
+           └── rank [type=undefined]
 
 # TryDecorrelateWindow will trigger twice here: first to pull the window above
 # the non-apply join, and then again the pull it above the apply join.