Merge #154789 #155029 #155030

154789: opt: fix trigger+computed column internal error r=mgartner a=mgartner

This commit fixes a bug that was caused by incorrectly modifying the set
and list of target columns when re-projecting computed columns after
building a `BEFORE` trigger.

Fixes #154672

Release note (bug fix): A bug has been fixed that caused internal errors
for `INSERT .. ON CONFLICT .. DO UPDATE` statements when the target
table had both a computed column and a `BEFORE` trigger. This bug has
been present since triggers were introduced in v24.3.0.


155029: log/eventpb: remove unused proto imports r=yuzefovich a=yuzefovich

There were left over in just merged 1ae7b24.

Epic: None
Release note: None

155030: logictest: de-flake recently added geo test r=yuzefovich a=yuzefovich

In recently merged change we added a test that assumes the inverted join is picked for a query. However, if we happen to not use the freshly collected table stats, we default to using a cross join. Fix this by clearing the stats cache before running the query.

Fixes: #155016.
Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: 3 people

pkg/sql/logictest/testdata/logic_test/inverted_index

@@ -2137,6 +2137,11 @@ INSERT INTO t151995 (c0, c1) VALUES (
 statement ok
 ANALYZE t151995;
 
+# Clear the stat cache so that the new statistic is guaranteed to be used (which
+# are needed for the inverted join to be picked).
+statement ok
+SELECT crdb_internal.clear_table_stats_cache();
+
 statement error geos error: IllegalArgumentException: BufferOp::getResultGeometry distance must be a finite value
 SELECT *
   FROM t151995 AS t1

pkg/sql/opt/optbuilder/insert.go

@@ -749,7 +749,8 @@ func (mb *mutationBuilder) addSynthesizedColsForInsert() {
 	mb.addAssignmentCasts(mb.insertColIDs)
 
 	// Now add all computed columns.
-	mb.addSynthesizedComputedCols(mb.insertColIDs, false /* restrict */)
+	mb.addSynthesizedComputedCols(mb.insertColIDs,
+		&mb.targetColList, &mb.targetColSet, false /* restrict */)
 
 	// Add assignment casts for computed column values.
 	mb.addAssignmentCasts(mb.insertColIDs)

pkg/sql/opt/optbuilder/mutation_builder.go

@@ -800,11 +800,16 @@ func (mb *mutationBuilder) addSynthesizedDefaultCols(
 // expression. New columns are synthesized for any missing columns using the
 // computed column expression.
 //
+// The target table columns corresponding to each projected computed column are
+// added to targetColList and targetColSet, if they are not nil.
+//
 // NOTE: colIDs is updated with the column IDs of any synthesized columns which
 // are added to mb.outScope. If restrict is true, only columns that depend on
 // columns that were already in the list (plus all write-only columns) are
 // updated.
-func (mb *mutationBuilder) addSynthesizedComputedCols(colIDs opt.OptionalColList, restrict bool) {
+func (mb *mutationBuilder) addSynthesizedComputedCols(
+	colIDs opt.OptionalColList, targetColList *opt.ColList, targetColSet *opt.ColSet, restrict bool,
+) {
 	// We will construct a new Project operator that will contain the newly
 	// synthesized column(s).
 	pb := makeProjectionBuilder(mb.b, mb.outScope)
@@ -876,8 +881,12 @@ func (mb *mutationBuilder) addSynthesizedComputedCols(colIDs opt.OptionalColList
 		}
 
 		// Add corresponding target column.
-		mb.targetColList = append(mb.targetColList, tabColID)
-		mb.targetColSet.Add(tabColID)
+		if targetColList != nil {
+			*targetColList = append(*targetColList, tabColID)
+		}
+		if targetColSet != nil {
+			targetColSet.Add(tabColID)
+		}
 	}
 
 	mb.outScope = pb.Finish()

pkg/sql/opt/optbuilder/testdata/trigger

@@ -1253,3 +1253,109 @@ insert t133329
                      └── filters
                           ├── a = t133329.a
                           └── k != t133329.k
+
+# Regression test for #154672. Do not set target columns when re-projecting
+# computed columns after a BEFORE trigger.
+exec-ddl
+CREATE TABLE t154672 (
+  k INT PRIMARY KEY,
+  a INT,
+  b INT AS (a + 1) STORED
+)
+----
+
+exec-ddl
+CREATE FUNCTION f154672() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
+  BEGIN
+    RETURN COALESCE(NEW, OLD);
+  END
+$$
+----
+
+exec-ddl
+CREATE TRIGGER tr154672 BEFORE INSERT OR UPDATE OR DELETE ON t154672 FOR EACH ROW EXECUTE FUNCTION f154672()
+----
+
+norm
+INSERT INTO t154672 (k, a) VALUES (1, 2) ON CONFLICT (k) DO UPDATE SET a = 3
+----
+upsert t154672
+ ├── arbiter indexes: t154672_pkey
+ ├── columns: <none>
+ ├── canary column: k:9
+ ├── fetch columns: k:9 a:10 b:11
+ ├── insert-mapping:
+ │    ├── k_new:29 => k:1
+ │    ├── a_new:30 => a:2
+ │    └── b_comp:31 => b:3
+ ├── update-mapping:
+ │    ├── upsert_k:40 => k:1
+ │    ├── upsert_a:41 => a:2
+ │    └── upsert_b:42 => b:3
+ ├── before-triggers
+ │    └── tr154672
+ └── project
+      ├── columns: upsert_k:40 upsert_a:41 upsert_b:42 k:9 a:10 b:11 k_new:29 a_new:30 b_comp:31
+      ├── project
+      │    ├── columns: k_new:37 a_new:38 k:9 a:10 b:11 k_new:29 a_new:30 b_comp:31
+      │    ├── barrier
+      │    │    ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null k_new:29 a_new:30 b_comp:31 a_new:32!null b_comp:33!null old:34 new:35 f154672:36!null
+      │    │    └── select
+      │    │         ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null k_new:29 a_new:30 b_comp:31 a_new:32!null b_comp:33!null old:34 new:35 f154672:36!null
+      │    │         ├── project
+      │    │         │    ├── columns: f154672:36 column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null k_new:29 a_new:30 b_comp:31 a_new:32!null b_comp:33!null old:34 new:35
+      │    │         │    ├── barrier
+      │    │         │    │    ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null k_new:29 a_new:30 b_comp:31 a_new:32!null b_comp:33!null old:34 new:35
+      │    │         │    │    └── project
+      │    │         │    │         ├── columns: new:35 old:34 b_comp:33!null a_new:32!null b_comp:31 k_new:29 a_new:30 column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null
+      │    │         │    │         ├── barrier
+      │    │         │    │         │    ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null
+      │    │         │    │         │    └── select
+      │    │         │    │         │         ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14 f154672:28!null
+      │    │         │    │         │         ├── project
+      │    │         │    │         │         │    ├── columns: f154672:28 column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14
+      │    │         │    │         │         │    ├── barrier
+      │    │         │    │         │         │    │    ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13 new:14
+      │    │         │    │         │         │    │    └── project
+      │    │         │    │         │         │    │         ├── columns: new:14 column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
+      │    │         │    │         │         │    │         ├── left-join (cross)
+      │    │         │    │         │         │    │         │    ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
+      │    │         │    │         │         │    │         │    ├── values
+      │    │         │    │         │         │    │         │    │    ├── columns: column1:6!null column2:7!null b_comp:8!null
+      │    │         │    │         │         │    │         │    │    └── (1, 2, 3)
+      │    │         │    │         │         │    │         │    ├── select
+      │    │         │    │         │         │    │         │    │    ├── columns: k:9!null a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
+      │    │         │    │         │         │    │         │    │    ├── scan t154672
+      │    │         │    │         │         │    │         │    │    │    ├── columns: k:9!null a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
+      │    │         │    │         │         │    │         │    │    │    ├── computed column expressions
+      │    │         │    │         │         │    │         │    │    │    │    └── b:11
+      │    │         │    │         │         │    │         │    │    │    │         └── a:10 + 1
+      │    │         │    │         │         │    │         │    │    │    └── flags: avoid-full-scan disabled not visible index feature
+      │    │         │    │         │         │    │         │    │    └── filters
+      │    │         │    │         │         │    │         │    │         └── k:9 = 1
+      │    │         │    │         │         │    │         │    └── filters (true)
+      │    │         │    │         │         │    │         └── projections
+      │    │         │    │         │         │    │              └── ((column1:6, column2:7, CAST(NULL AS INT8)) AS k, a, b) [as=new:14]
+      │    │         │    │         │         │    └── projections
+      │    │         │    │         │         │         └── f154672(new:14, NULL, 'tr154672', 'BEFORE', 'ROW', 'INSERT', 61, 't154672', 't154672', 'public', 0, ARRAY[]) [as=f154672:28]
+      │    │         │    │         │         └── filters
+      │    │         │    │         │              └── f154672:28 IS DISTINCT FROM NULL
+      │    │         │    │         └── projections
+      │    │         │    │              ├── ((k:9, 3, CAST(NULL AS INT8)) AS k, a, b) [as=new:35]
+      │    │         │    │              ├── ((k:9, a:10, CAST(NULL AS INT8)) AS k, a, b) [as=old:34]
+      │    │         │    │              ├── 4 [as=b_comp:33]
+      │    │         │    │              ├── 3 [as=a_new:32]
+      │    │         │    │              ├── a:10 + 1 [as=b_comp:31]
+      │    │         │    │              ├── (f154672:28).k [as=k_new:29]
+      │    │         │    │              └── (f154672:28).a [as=a_new:30]
+      │    │         │    └── projections
+      │    │         │         └── CASE WHEN k:9 IS NOT NULL THEN f154672(new:35, old:34, 'tr154672', 'BEFORE', 'ROW', 'UPDATE', 61, 't154672', 't154672', 'public', 0, ARRAY[]) ELSE new:35 END [as=f154672:36]
+      │    │         └── filters
+      │    │              └── f154672:36 IS DISTINCT FROM NULL
+      │    └── projections
+      │         ├── (f154672:36).k [as=k_new:37]
+      │         └── (f154672:36).a [as=a_new:38]
+      └── projections
+           ├── CASE WHEN k:9 IS NULL THEN k_new:29 ELSE k_new:37 END [as=upsert_k:40]
+           ├── CASE WHEN k:9 IS NULL THEN a_new:30 ELSE a_new:38 END [as=upsert_a:41]
+           └── CASE WHEN k:9 IS NULL THEN b_comp:31 ELSE a_new:38 + 1 END [as=upsert_b:42]

pkg/sql/opt/optbuilder/trigger.go

@@ -373,7 +373,8 @@ func (mb *mutationBuilder) recomputeComputedColsForTrigger(eventType tree.Trigge
 			colIDs[i] = 0
 		}
 	}
-	mb.addSynthesizedComputedCols(colIDs, false /* restrict */)
+	mb.addSynthesizedComputedCols(colIDs,
+		nil /* targetColList */, nil /* targetColSet */, false /* restrict */)
 }
 
 // ============================================================================

pkg/sql/opt/optbuilder/update.go

@@ -337,7 +337,8 @@ func (mb *mutationBuilder) addSynthesizedColsForUpdate() {
 	mb.disambiguateColumns()
 
 	// Add all computed columns in case their values have changed.
-	mb.addSynthesizedComputedCols(mb.updateColIDs, true /* restrict */)
+	mb.addSynthesizedComputedCols(mb.updateColIDs,
+		&mb.targetColList, &mb.targetColSet, true /* restrict */)
 
 	// Add assignment casts for computed column values.
 	mb.addAssignmentCasts(mb.updateColIDs)

pkg/util/log/eventpb/telemetry.proto

@@ -9,8 +9,6 @@ option go_package = "github.com/cockroachdb/cockroach/pkg/util/log/eventpb";
 
 import "gogoproto/gogo.proto";
 import "sql/catalog/descpb/structured.proto";
-import "util/log/eventpb/events.proto";
-import "util/log/eventpb/sql_audit_events.proto";
 import "util/log/logpb/event.proto";
 
 // Category: Telemetry events