Merge #46285

46285: opt: fix handling of write-only columns during update r=andy-kimball a=andy-kimball

A column in the write-only state has either been recently added or dropped
from a table. If recently added, then it's currently being backfilled, and
therefore may still have a NULL value. If NULL, then it's not valid to use
this value during an UPDATE or UPSERT. However, this is what the previous
code was doing; when a column value was updated, it would mistakenly
incorporate the NULL value into the update.

The fix is to *always* write the default or computed value of a column
during an UPDATE or UPSERT of a write-only column. Here are the rules:

  1. If column is computed, evaluate that expression as its value.
  2. If column has a default value specified for it, use that as its value.
  3. If column is nullable, use NULL as its value.
  4. If column is currently being added or dropped (i.e. a mutation column),
     use a default value (0 for INT column, "" for STRING column, etc).

One side effect of doing this is that NOT NULL columns can now be dropped
without triggering null constraint violations when other transactions try
to insert rows during the dropping process. Also, other transactions can
observe the default values if they SELECT a column that is dropped at just
the right time.

Fixes #42459

Release justification: Fix for high-severity bug in existing functionality.
This bug can result in NULL values being written to NOT NULL columns. It
can also trigger unexpected errors during INSERT/UPDATE/UPSERT statements
when schema changes are taking place.

Release note (sql change): Columns in the process of being added or removed
to a table are now always set to their default or computed value if another
transaction concurrently inserts, updates, or upserts a row. This fixes an
issue where a column being backfilled would not get properly set by
concurrent transactions.

Co-authored-by: Andrew Kimball <[email protected]>

pkg/sql/descriptor_mutation_test.go

@@ -336,15 +336,9 @@ CREATE INDEX allidx ON t.test (k, v);
 						afterDefaultInsert = [][]string{{"a", "z", "q"}, {"default", "NULL", "i"}}
 						// The default value of "i" for column "i" is written.
 						afterInsert = [][]string{{"a", "z", "q"}, {"c", "x", "i"}}
-						if useUpsert {
-							// Update is not a noop for column "i".
-							afterUpdate = [][]string{{"a", "u", "q"}, {"c", "x", "i"}}
-							afterPKUpdate = [][]string{{"a", "u", "q"}, {"d", "x", "i"}}
-						} else {
-							// Update is a noop for column "i".
-							afterUpdate = [][]string{{"a", "u", "q"}, {"c", "x", "i"}}
-							afterPKUpdate = [][]string{{"a", "u", "q"}, {"d", "x", "i"}}
-						}
+						// Upsert/update sets column "i" to default value of "i".
+						afterUpdate = [][]string{{"a", "u", "i"}, {"c", "x", "i"}}
+						afterPKUpdate = [][]string{{"a", "u", "i"}, {"d", "x", "i"}}
 						// Delete also deletes column "i".
 						afterDelete = [][]string{{"d", "x", "i"}}
 						afterDeleteKeys = 4
@@ -768,14 +762,20 @@ CREATE INDEX allidx ON t.test (k, v);
 				// Make column "i" and index "foo" live.
 				mTest.makeMutationsActive()
 
-				// The update to column "v" is seen; there is no effect on column "i".
-				mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "q"}, {"b", "y", "r"}, {"c", "x", "NULL"}})
+				if state == sqlbase.DescriptorMutation_DELETE_ONLY {
+					// Mutation column "i" is not updated.
+					mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "q"}, {"b", "y", "r"}, {"c", "x", "NULL"}})
+				} else {
+					// Mutation column "i" is set to its default value (NULL).
+					mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "NULL"}, {"b", "y", "r"}, {"c", "x", "NULL"}})
+				}
+
 				if idxState == sqlbase.DescriptorMutation_DELETE_ONLY {
 					// Index entry for row "a" is deleted.
 					mTest.CheckQueryResults(t, indexQuery, [][]string{{"r"}})
 				} else {
-					// No change in index "foo"
-					mTest.CheckQueryResults(t, indexQuery, [][]string{{"NULL"}, {"q"}, {"r"}})
+					// Index "foo" has NULL "i" value for row "a".
+					mTest.CheckQueryResults(t, indexQuery, [][]string{{"NULL"}, {"NULL"}, {"r"}})
 				}
 
 				// Add index "foo" as a mutation.
@@ -788,19 +788,34 @@ CREATE INDEX allidx ON t.test (k, v);
 				// Make column "i" and index "foo" live.
 				mTest.makeMutationsActive()
 				// Row "b" is deleted.
-				mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "q"}, {"c", "x", "NULL"}})
+				if state == sqlbase.DescriptorMutation_DELETE_ONLY {
+					mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "q"}, {"c", "x", "NULL"}})
+				} else {
+					mTest.CheckQueryResults(t, starQuery, [][]string{{"a", "u", "NULL"}, {"c", "x", "NULL"}})
+				}
+
 				// numKVs is the number of expected key-values. We start with the number
 				// of non-NULL values above.
-				numKVs := 7
+				numKVs := 6
+				if state == sqlbase.DescriptorMutation_DELETE_ONLY {
+					// In DELETE_ONLY case, the "q" value is not set to NULL above.
+					numKVs++
+				}
+
 				if idxState == sqlbase.DescriptorMutation_DELETE_ONLY {
-					// Index entry for row "b" is deleted.
+					// Index entry for row "a" is deleted.
 					mTest.CheckQueryResults(t, indexQuery, [][]string{})
 				} else {
-					// Index entry for row "b" is deleted.
-					mTest.CheckQueryResults(t, indexQuery, [][]string{{"NULL"}, {"q"}})
+					// Index entry for row "a" is deleted.
+					if state == sqlbase.DescriptorMutation_DELETE_ONLY {
+						mTest.CheckQueryResults(t, indexQuery, [][]string{{"NULL"}, {"q"}})
+					} else {
+						mTest.CheckQueryResults(t, indexQuery, [][]string{{"NULL"}, {"NULL"}})
+					}
 					// We have two index values.
 					numKVs += 2
 				}
+
 				// Check that there are no hidden KV values for row "b", and column
 				// "i" for row "b" was deleted. Also check that the index values are
 				// all accounted for.

pkg/sql/opt/exec/execbuilder/testdata/schema_change_in_txn

@@ -0,0 +1,73 @@
+# LogicTest: local
+
+statement ok
+CREATE TABLE t (
+  a INT PRIMARY KEY,
+  b DECIMAL(10,1) NOT NULL DEFAULT(1000.15),
+  c TEXT COLLATE en_US DEFAULT('empty' COLLATE en_US),
+  d DECIMAL(10,2) NOT NULL,
+  e TIME,
+  f DECIMAL AS (a + b + d) STORED,
+  --UNIQUE INDEX t_secondary (c, d), -- Fails due to #46276
+  FAMILY (a, b, c),
+  FAMILY (d, e, f)
+)
+
+statement ok
+INSERT INTO t VALUES (100, 500.5, 'stuff' COLLATE en_US, 600.6, '12:12:12')
+
+# Drop all columns except "a" and perform interesting operations.
+statement ok
+BEGIN;
+--DROP INDEX t_secondary CASCADE;
+ALTER TABLE t DROP COLUMN b, DROP COLUMN c, DROP COLUMN d, DROP COLUMN e, DROP COLUMN f;
+ALTER TABLE t ADD COLUMN g INT NOT NULL DEFAULT(15)
+
+# Expect default values for b, c, zero value for d, and NULL value for e.
+query T kvtrace(prefix=/Table/53/)
+INSERT INTO t SELECT a + 1 FROM t
+----
+Scan /Table/53/{1-2}
+CPut /Table/53/1/101/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+CPut /Table/53/1/101/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1101.2
+
+# Expect default values for b, c, zero value for d, and NULL value for e.
+query T kvtrace(prefix=/Table/53/)
+UPSERT INTO t SELECT a + 1 FROM t
+----
+Scan /Table/53/{1-2}
+Scan /Table/53/{1-2}
+Put /Table/53/1/101/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+Put /Table/53/1/101/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1101.2
+CPut /Table/53/1/102/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+CPut /Table/53/1/102/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1102.2
+
+# Expect default values for b, c, zero value for d, and NULL value for e.
+query T kvtrace(prefix=/Table/53/)
+UPDATE t SET a = a + 100
+----
+Scan /Table/53/{1-2}
+Del /Table/53/1/100/0
+Del /Table/53/1/100/1/1
+CPut /Table/53/1/200/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+CPut /Table/53/1/200/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1200.2
+Del /Table/53/1/101/0
+Del /Table/53/1/101/1/1
+CPut /Table/53/1/201/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+CPut /Table/53/1/201/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1201.2
+Del /Table/53/1/102/0
+Del /Table/53/1/102/1/1
+CPut /Table/53/1/202/0 -> /TUPLE/2:2:Decimal/1000.2/1:3:Bytes/empty
+CPut /Table/53/1/202/1/1 -> /TUPLE/4:4:Decimal/0.00/2:6:Decimal/1202.2
+
+statement ok
+DELETE FROM t WHERE a=201
+
+statement ok
+COMMIT
+
+query II
+SELECT * FROM t
+----
+200  15
+202  15