Skip to content

Commit 8f13f74

Browse files
mgartnermgartner
committed
opt: fix trigger+computed column internal error
This commit fixes a bug that was caused by prematurely re-synthesizing 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.
1 parent 2a1771e commit 8f13f74

File tree

6 files changed

+122
-15
lines changed

6 files changed

+122
-15
lines changed

‎pkg/sql/opt/optbuilder/delete.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func (b *Builder) buildDelete(del *tree.Delete, inScope *scope) (outScope *scope
8989
mb.buildInputForDelete(inScope, del.Table, del.Where, del.Using, del.Limit, del.OrderBy)
9090

9191
// Project row-level BEFORE triggers for DELETE.
92-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, false /* cascade */)
92+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, false /* cascade */, true /* recomputeCols */)
9393

9494
// Build the final delete statement, including any returned expressions.
9595
if resultsNeeded(del.Returning) {

‎pkg/sql/opt/optbuilder/fk_cascade.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ func (cb *onDeleteCascadeBuilder) Build(
124124
b.checkMultipleMutations(mb.tab, generalMutation)
125125

126126
// Cascades can fire triggers on the child table.
127-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, true /* cascade */)
127+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, true /* cascade */, true /* recomputeCols */)
128128

129129
mb.buildDelete(nil /* returning */)
130130
return mb.outScope.expr
@@ -426,7 +426,7 @@ func (cb *onDeleteFastCascadeBuilder) Build(
426426
b.checkMultipleMutations(mb.tab, generalMutation)
427427

428428
// Cascades can fire triggers on the child table.
429-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, true /* cascade */)
429+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventDelete, true /* cascade */, true /* recomputeCols */)
430430

431431
mb.buildDelete(nil /* returning */)
432432
return mb.outScope.expr
@@ -567,7 +567,7 @@ func (cb *onDeleteSetBuilder) Build(
567567
b.checkMultipleMutations(mb.tab, generalMutation)
568568

569569
// Cascades can fire triggers on the child table.
570-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, true /* cascade */)
570+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, true /* cascade */, true /* recomputeCols */)
571571

572572
// TODO(radu): consider plumbing a flag to prevent building the FK check
573573
// against the parent we are cascading from. Need to investigate in which
@@ -840,7 +840,7 @@ func (cb *onUpdateCascadeBuilder) Build(
840840
b.checkMultipleMutations(mb.tab, generalMutation)
841841

842842
// Cascades can fire triggers on the child table.
843-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, true /* cascade */)
843+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, true /* cascade */, true /* recomputeCols */)
844844

845845
// The exempt policy is used for RLS to maintain data integrity.
846846
mb.buildUpdate(nil /* returning */, cat.PolicyScopeExempt, nil /* colRefs */)

‎pkg/sql/opt/optbuilder/insert.go‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
307307
// Case 1: Simple INSERT statement.
308308
case ins.OnConflict == nil:
309309
// Project row-level BEFORE triggers for INSERT.
310-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */)
310+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */, true /* recomputeCols */)
311311

312312
// Build the final insert statement, including any returned expressions.
313313
mb.buildInsert(returning, ins.VectorInsert(), false /* hasOnConflict */)
@@ -316,7 +316,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
316316
case ins.OnConflict.DoNothing:
317317

318318
// Project row-level BEFORE triggers for INSERT.
319-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */)
319+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */, true /* recomputeCols */)
320320

321321
// Wrap the input in one ANTI JOIN per UNIQUE index, and filter out rows
322322
// that have conflicts. See the buildInputForDoNothing comment for more
@@ -345,7 +345,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
345345
// NOTE: we avoid building INSERT triggers until after buildInputForUpsert
346346
// so that there are no buffering operators between INSERT and UPDATE
347347
// triggers. This helps preserve Postgres compatibility
348-
if mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */) {
348+
if mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */, true /* recomputeCols */) {
349349
// INSERT triggers are able to modify the row being inserted, so we need
350350
// to recompute the upsert columns.
351351
mb.setUpsertCols(nil /* insertCols */)
@@ -356,10 +356,10 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
356356
mb.addSynthesizedColsForUpdate()
357357

358358
// Project row-level BEFORE triggers for UPDATE.
359-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */)
359+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */, true /* recomputeCols */)
360360
} else {
361361
// Project row-level BEFORE triggers for INSERT.
362-
if mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */) {
362+
if mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */, true /* recomputeCols */) {
363363
// INSERT triggers are able to modify the row being inserted, so we need
364364
// to recompute the upsert columns.
365365
mb.setUpsertCols(nil /* insertCols */)
@@ -376,7 +376,9 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
376376
canaryCol := mb.buildInputForUpsert(inScope, ins.Table, ins.OnConflict)
377377

378378
// Project row-level BEFORE triggers for INSERT.
379-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */)
379+
// NOTE: Do not recompute computed columns, this will happen in
380+
// addUpdateCols.
381+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventInsert, false /* cascade */, false /* recomputeCols */)
380382

381383
// Add a filter from the WHERE clause if one exists. This must happen after
382384
// the INSERT triggers are added, since BEFORE INSERT triggers are called
@@ -392,7 +394,7 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
392394
mb.addUpdateCols(ins.OnConflict.Exprs, nil /* colRefs */)
393395

394396
// Project row-level BEFORE triggers for UPDATE.
395-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */)
397+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */, true /* recomputeCols */)
396398

397399
// Build the final upsert statement, including any returned expressions.
398400
mb.buildUpsert(returning)

‎pkg/sql/opt/optbuilder/testdata/trigger‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,3 +1253,108 @@ insert t133329
12531253
└── filters
12541254
├── a = t133329.a
12551255
└── k != t133329.k
1256+
1257+
# Regression test for #154672. Do not set target columns when re-projecting
1258+
# computed columns after a BEFORE trigger.
1259+
exec-ddl
1260+
CREATE TABLE t154672 (
1261+
k INT PRIMARY KEY,
1262+
a INT,
1263+
b INT AS (a + 1) STORED
1264+
)
1265+
----
1266+
1267+
exec-ddl
1268+
CREATE FUNCTION f154672() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
1269+
BEGIN
1270+
RETURN COALESCE(NEW, OLD);
1271+
END
1272+
$$
1273+
----
1274+
1275+
exec-ddl
1276+
CREATE TRIGGER tr154672 BEFORE INSERT OR UPDATE OR DELETE ON t154672 FOR EACH ROW EXECUTE FUNCTION f154672()
1277+
----
1278+
1279+
norm
1280+
INSERT INTO t154672 (k, a) VALUES (1, 2) ON CONFLICT (k) DO UPDATE SET a = 3
1281+
----
1282+
upsert t154672
1283+
├── arbiter indexes: t154672_pkey
1284+
├── columns: <none>
1285+
├── canary column: k:9
1286+
├── fetch columns: k:9 a:10 b:11
1287+
├── insert-mapping:
1288+
│ ├── k_new:29 => k:1
1289+
│ ├── a_new:30 => a:2
1290+
│ └── b_comp:8 => b:3
1291+
├── update-mapping:
1292+
│ ├── upsert_k:39 => k:1
1293+
│ ├── upsert_a:40 => a:2
1294+
│ └── upsert_b:41 => b:3
1295+
├── before-triggers
1296+
│ └── tr154672
1297+
└── project
1298+
├── columns: upsert_k:39 upsert_a:40 upsert_b:41 b_comp:8!null k:9 a:10 b:11 k_new:29 a_new:30
1299+
├── project
1300+
│ ├── columns: k_new:36 a_new:37 b_comp:8!null k:9 a:10 b:11 k_new:29 a_new:30
1301+
│ ├── barrier
1302+
│ │ ├── 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 a_new:31!null b_comp:32!null old:33 new:34 f154672:35!null
1303+
│ │ └── select
1304+
│ │ ├── 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 a_new:31!null b_comp:32!null old:33 new:34 f154672:35!null
1305+
│ │ ├── project
1306+
│ │ │ ├── columns: f154672:35 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 a_new:31!null b_comp:32!null old:33 new:34
1307+
│ │ │ ├── barrier
1308+
│ │ │ │ ├── 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 a_new:31!null b_comp:32!null old:33 new:34
1309+
│ │ │ │ └── project
1310+
│ │ │ │ ├── columns: new:34 old:33 b_comp:32!null a_new:31!null 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
1311+
│ │ │ │ ├── barrier
1312+
│ │ │ │ │ ├── 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
1313+
│ │ │ │ │ └── select
1314+
│ │ │ │ │ ├── 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
1315+
│ │ │ │ │ ├── project
1316+
│ │ │ │ │ │ ├── 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
1317+
│ │ │ │ │ │ ├── barrier
1318+
│ │ │ │ │ │ │ ├── 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
1319+
│ │ │ │ │ │ │ └── project
1320+
│ │ │ │ │ │ │ ├── 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
1321+
│ │ │ │ │ │ │ ├── left-join (cross)
1322+
│ │ │ │ │ │ │ │ ├── columns: column1:6!null column2:7!null b_comp:8!null k:9 a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
1323+
│ │ │ │ │ │ │ │ ├── values
1324+
│ │ │ │ │ │ │ │ │ ├── columns: column1:6!null column2:7!null b_comp:8!null
1325+
│ │ │ │ │ │ │ │ │ └── (1, 2, 3)
1326+
│ │ │ │ │ │ │ │ ├── select
1327+
│ │ │ │ │ │ │ │ │ ├── columns: k:9!null a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
1328+
│ │ │ │ │ │ │ │ │ ├── scan t154672
1329+
│ │ │ │ │ │ │ │ │ │ ├── columns: k:9!null a:10 b:11 crdb_internal_mvcc_timestamp:12 tableoid:13
1330+
│ │ │ │ │ │ │ │ │ │ ├── computed column expressions
1331+
│ │ │ │ │ │ │ │ │ │ │ └── b:11
1332+
│ │ │ │ │ │ │ │ │ │ │ └── a:10 + 1
1333+
│ │ │ │ │ │ │ │ │ │ └── flags: avoid-full-scan disabled not visible index feature
1334+
│ │ │ │ │ │ │ │ │ └── filters
1335+
│ │ │ │ │ │ │ │ │ └── k:9 = 1
1336+
│ │ │ │ │ │ │ │ └── filters (true)
1337+
│ │ │ │ │ │ │ └── projections
1338+
│ │ │ │ │ │ │ └── ((column1:6, column2:7, CAST(NULL AS INT8)) AS k, a, b) [as=new:14]
1339+
│ │ │ │ │ │ └── projections
1340+
│ │ │ │ │ │ └── f154672(new:14, NULL, 'tr154672', 'BEFORE', 'ROW', 'INSERT', 61, 't154672', 't154672', 'public', 0, ARRAY[]) [as=f154672:28]
1341+
│ │ │ │ │ └── filters
1342+
│ │ │ │ │ └── f154672:28 IS DISTINCT FROM NULL
1343+
│ │ │ │ └── projections
1344+
│ │ │ │ ├── ((k:9, 3, CAST(NULL AS INT8)) AS k, a, b) [as=new:34]
1345+
│ │ │ │ ├── ((k:9, a:10, CAST(NULL AS INT8)) AS k, a, b) [as=old:33]
1346+
│ │ │ │ ├── 4 [as=b_comp:32]
1347+
│ │ │ │ ├── 3 [as=a_new:31]
1348+
│ │ │ │ ├── (f154672:28).k [as=k_new:29]
1349+
│ │ │ │ └── (f154672:28).a [as=a_new:30]
1350+
│ │ │ └── projections
1351+
│ │ │ └── CASE WHEN k:9 IS NOT NULL THEN f154672(new:34, old:33, 'tr154672', 'BEFORE', 'ROW', 'UPDATE', 61, 't154672', 't154672', 'public', 0, ARRAY[]) ELSE new:34 END [as=f154672:35]
1352+
│ │ └── filters
1353+
│ │ └── f154672:35 IS DISTINCT FROM NULL
1354+
│ └── projections
1355+
│ ├── (f154672:35).k [as=k_new:36]
1356+
│ └── (f154672:35).a [as=a_new:37]
1357+
└── projections
1358+
├── CASE WHEN k:9 IS NULL THEN k_new:29 ELSE k_new:36 END [as=upsert_k:39]
1359+
├── CASE WHEN k:9 IS NULL THEN a_new:30 ELSE a_new:37 END [as=upsert_a:40]
1360+
└── CASE WHEN k:9 IS NULL THEN b_comp:8 ELSE a_new:37 + 1 END [as=upsert_b:41]

‎pkg/sql/opt/optbuilder/trigger.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import (
3636
// based on the event type. It returns true if triggers were built, and false
3737
// otherwise.
3838
func (mb *mutationBuilder) buildRowLevelBeforeTriggers(
39-
eventType tree.TriggerEventType, cascade bool,
39+
eventType tree.TriggerEventType, cascade bool, recomputeCols bool,
4040
) bool {
4141
var eventsToMatch tree.TriggerEventTypeSet
4242
eventsToMatch.Add(eventType)
@@ -155,7 +155,7 @@ func (mb *mutationBuilder) buildRowLevelBeforeTriggers(
155155

156156
// Since INSERT and UPDATE triggers can modify the row, we need to recompute
157157
// the computed columns.
158-
if eventType == tree.TriggerEventInsert || eventType == tree.TriggerEventUpdate {
158+
if recomputeCols && (eventType == tree.TriggerEventInsert || eventType == tree.TriggerEventUpdate) {
159159
mb.recomputeComputedColsForTrigger(eventType)
160160
}
161161
return true

‎pkg/sql/opt/optbuilder/update.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func (b *Builder) buildUpdate(upd *tree.Update, inScope *scope) (outScope *scope
115115
mb.addUpdateCols(upd.Exprs, &exprColRefs)
116116

117117
// Project row-level BEFORE triggers for UPDATE.
118-
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */)
118+
mb.buildRowLevelBeforeTriggers(tree.TriggerEventUpdate, false /* cascade */, true /* recomputeCols */)
119119

120120
// Build the final update statement, including any returned expressions.
121121
var returningExpr *tree.ReturningExprs

0 commit comments

Comments
 (0)