Skip to content

Commit b697e85

Browse files
fqazifqazi
committed
sql/schemachanger: clean up triggers before their references
Previously, when cascaded drops occurred in the declarative schema changer it was possible for a function, columns, and types to be cleaned up before triggers that referenced them. This could lead to failed DROP SCHEMA CASCADE operations. To address this, this patch adds a rule for enforcing that function references are cleaned up first in the case of triggers. Fixes: #154248 Release note (bug fix): Addresses a bug where DROP SCHEMA CASCADE could run into an error with complex references from triggers.
1 parent 87deb72 commit b697e85

File tree

14 files changed

+247
-35
lines changed

14 files changed

+247
-35
lines changed

‎pkg/ccl/schemachangerccl/testdata/end_to_end/create_trigger/create_trigger.explain‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ Schema change plan for CREATE TRIGGER tr BEFORE INSERT OR UPDATE OR DELETE ON
2020
│ │ ├── ABSENT → PUBLIC TriggerTiming:{DescID: 104 (t), TriggerID: 1}
2121
│ │ ├── ABSENT → PUBLIC TriggerEvents:{DescID: 104 (t), TriggerID: 1}
2222
│ │ ├── ABSENT → PUBLIC TriggerFunctionCall:{DescID: 104 (t), TriggerID: 1}
23-
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1}
23+
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
2424
│ ├── 1 element transitioning toward TRANSIENT_PUBLIC
2525
│ │ └── PUBLIC → ABSENT TableSchemaLocked:{DescID: 104 (t)}
2626
│ └── 10 Mutation operations
@@ -43,7 +43,7 @@ Schema change plan for CREATE TRIGGER tr BEFORE INSERT OR UPDATE OR DELETE ON
4343
│ │ │ ├── PUBLIC → ABSENT TriggerTiming:{DescID: 104 (t), TriggerID: 1}
4444
│ │ │ ├── PUBLIC → ABSENT TriggerEvents:{DescID: 104 (t), TriggerID: 1}
4545
│ │ │ ├── PUBLIC → ABSENT TriggerFunctionCall:{DescID: 104 (t), TriggerID: 1}
46-
│ │ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1}
46+
│ │ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
4747
│ │ ├── 1 element transitioning toward TRANSIENT_PUBLIC
4848
│ │ │ └── ABSENT → PUBLIC TableSchemaLocked:{DescID: 104 (t)}
4949
│ │ └── 1 Mutation operation
@@ -56,7 +56,7 @@ Schema change plan for CREATE TRIGGER tr BEFORE INSERT OR UPDATE OR DELETE ON
5656
│ │ ├── ABSENT → PUBLIC TriggerTiming:{DescID: 104 (t), TriggerID: 1}
5757
│ │ ├── ABSENT → PUBLIC TriggerEvents:{DescID: 104 (t), TriggerID: 1}
5858
│ │ ├── ABSENT → PUBLIC TriggerFunctionCall:{DescID: 104 (t), TriggerID: 1}
59-
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1}
59+
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
6060
│ ├── 1 element transitioning toward TRANSIENT_PUBLIC
6161
│ │ └── PUBLIC → ABSENT TableSchemaLocked:{DescID: 104 (t)}
6262
│ └── 13 Mutation operations

‎pkg/ccl/schemachangerccl/testdata/end_to_end/create_trigger/create_trigger__rollback_1_of_1.explain‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ Schema change plan for rolling back CREATE TRIGGER tr BEFORE INSERT OR UPDATE OR
2121
│ │ ├── PUBLIC → ABSENT TriggerTiming:{DescID: 104 (t), TriggerID: 1}
2222
│ │ ├── PUBLIC → ABSENT TriggerEvents:{DescID: 104 (t), TriggerID: 1}
2323
│ │ ├── PUBLIC → ABSENT TriggerFunctionCall:{DescID: 104 (t), TriggerID: 1}
24-
│ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1}
24+
│ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
2525
│ └── 11 Mutation operations
2626
│ ├── RemoveTrigger {"Trigger":{"TableID":104,"TriggerID":1}}
2727
│ ├── NotImplementedForPublicObjects {"DescID":104,"ElementType":"scpb.TriggerName","TriggerID":1}

‎pkg/ccl/schemachangerccl/testdata/end_to_end/drop_table_trigger/drop_table_trigger.explain‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ Schema change plan for DROP TABLE ‹defaultdb›.‹public›.‹t›;
4848
│ │ ├── PUBLIC → ABSENT TriggerTiming:{DescID: 104 (t-), TriggerID: 1}
4949
│ │ ├── PUBLIC → ABSENT TriggerEvents:{DescID: 104 (t-), TriggerID: 1}
5050
│ │ ├── PUBLIC → ABSENT TriggerFunctionCall:{DescID: 104 (t-), TriggerID: 1}
51-
│ │ ├── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t-), TriggerID: 1}
51+
│ │ ├── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t-), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
5252
│ │ └── PUBLIC → ABSENT TableSchemaLocked:{DescID: 104 (t-)}
5353
│ └── 53 Mutation operations
5454
│ ├── MarkDescriptorAsDropped {"DescriptorID":104}
@@ -147,7 +147,7 @@ Schema change plan for DROP TABLE ‹defaultdb›.‹public›.‹t›;
147147
│ │ │ ├── ABSENT → PUBLIC TriggerTiming:{DescID: 104 (t-), TriggerID: 1}
148148
│ │ │ ├── ABSENT → PUBLIC TriggerEvents:{DescID: 104 (t-), TriggerID: 1}
149149
│ │ │ ├── ABSENT → PUBLIC TriggerFunctionCall:{DescID: 104 (t-), TriggerID: 1}
150-
│ │ │ ├── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t-), TriggerID: 1}
150+
│ │ │ ├── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t-), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
151151
│ │ │ └── ABSENT → PUBLIC TableSchemaLocked:{DescID: 104 (t-)}
152152
│ │ └── 1 Mutation operation
153153
│ │ └── UndoAllInTxnImmediateMutationOpSideEffects
@@ -193,7 +193,7 @@ Schema change plan for DROP TABLE ‹defaultdb›.‹public›.‹t›;
193193
│ │ ├── PUBLIC → ABSENT TriggerTiming:{DescID: 104 (t-), TriggerID: 1}
194194
│ │ ├── PUBLIC → ABSENT TriggerEvents:{DescID: 104 (t-), TriggerID: 1}
195195
│ │ ├── PUBLIC → ABSENT TriggerFunctionCall:{DescID: 104 (t-), TriggerID: 1}
196-
│ │ ├── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t-), TriggerID: 1}
196+
│ │ ├── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t-), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
197197
│ │ └── PUBLIC → ABSENT TableSchemaLocked:{DescID: 104 (t-)}
198198
│ └── 56 Mutation operations
199199
│ ├── MarkDescriptorAsDropped {"DescriptorID":104}

‎pkg/ccl/schemachangerccl/testdata/end_to_end/drop_trigger/drop_trigger.explain‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ Schema change plan for DROP TRIGGER ‹tr› ON ‹defaultdb›.‹t›;
99
│ └── Stage 1 of 1 in StatementPhase
1010
│ ├── 2 elements transitioning toward ABSENT
1111
│ │ ├── PUBLIC → ABSENT Trigger:{DescID: 104 (t), TriggerID: 1}
12-
│ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1}
12+
│ │ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
1313
│ └── 3 Mutation operations
1414
│ ├── RemoveTrigger {"Trigger":{"TableID":104,"TriggerID":1}}
1515
│ ├── UpdateTableBackReferencesInRelations {"TableID":104}
@@ -18,13 +18,13 @@ Schema change plan for DROP TRIGGER ‹tr› ON ‹defaultdb›.‹t›;
1818
├── Stage 1 of 2 in PreCommitPhase
1919
│ ├── 2 elements transitioning toward ABSENT
2020
│ │ ├── ABSENT → PUBLIC Trigger:{DescID: 104 (t), TriggerID: 1}
21-
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1}
21+
│ │ └── ABSENT → PUBLIC TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
2222
│ └── 1 Mutation operation
2323
│ └── UndoAllInTxnImmediateMutationOpSideEffects
2424
└── Stage 2 of 2 in PreCommitPhase
2525
├── 2 elements transitioning toward ABSENT
2626
│ ├── PUBLIC → ABSENT Trigger:{DescID: 104 (t), TriggerID: 1}
27-
│ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1}
27+
│ └── PUBLIC → ABSENT TriggerDeps:{DescID: 104 (t), TriggerID: 1, ReferencedFunctionIDs: [105 (f)]}
2828
└── 3 Mutation operations
2929
├── RemoveTrigger {"Trigger":{"TableID":104,"TriggerID":1}}
3030
├── UpdateTableBackReferencesInRelations {"TableID":104}

‎pkg/sql/logictest/testdata/logic_test/schema‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1517,3 +1517,48 @@ statement error pgcode 42601 pq: .*empty schema name
15171517
CREATE SCHEMA "";
15181518

15191519
subtest end
1520+
1521+
1522+
# Regression test for #154248, which ensures that during cascade drops of schemas,
1523+
# we will clean up triggers before functions that reference them.
1524+
subtest drop_schema_with_triggers
1525+
1526+
statement ok
1527+
CREATE SCHEMA drop_schema_with_triggers;
1528+
1529+
statement ok
1530+
CREATE TYPE drop_schema_with_triggers.typ_1 AS ENUM ('foo');
1531+
1532+
statement ok
1533+
CREATE TYPE typ_2_drop_schema_with_triggers AS ENUM ('foo');
1534+
1535+
statement ok
1536+
CREATE TABLE drop_schema_with_triggers.t1 (
1537+
n INT8 PRIMARY KEY, j INT8 UNIQUE
1538+
);
1539+
1540+
statement ok
1541+
CREATE TABLE t2_drop_schema_with_triggers (
1542+
n
1543+
INT8 PRIMARY KEY,
1544+
j
1545+
INT8 REFERENCES drop_schema_with_triggers.t1 (j)
1546+
);
1547+
1548+
statement ok
1549+
CREATE FUNCTION drop_schema_with_triggers.trigger_function_w4_520()
1550+
RETURNS TRIGGER
1551+
LANGUAGE plpgsql
1552+
AS $$ BEGIN SELECT 'foo'::drop_schema_with_triggers.typ_1, 'foo'::public.typ_2_drop_schema_with_triggers FROM drop_schema_with_triggers.t1;RETURN NEW;END; $$;
1553+
1554+
skipif config local-legacy-schema-changer
1555+
statement ok
1556+
CREATE TRIGGER trigger_w4_521 BEFORE UPDATE OR DELETE ON drop_schema_with_triggers.t1 FOR EACH ROW EXECUTE FUNCTION drop_schema_with_triggers.trigger_function_w4_520();
1557+
1558+
skipif config local-mixed-25.2
1559+
skipif config local-mixed-25.3
1560+
skipif config local-legacy-schema-changer
1561+
statement ok
1562+
DROP SCHEMA drop_schema_with_triggers CASCADE;
1563+
1564+
subtest end

‎pkg/sql/schemachanger/scbuild/testdata/create_trigger‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ CREATE TRIGGER tr BEFORE INSERT OR UPDATE ON xy FOR EACH ROW EXECUTE FUNCTION f(
3434
{events: [{columnNames: [], type: INSERT}, {columnNames: [], type: UPDATE}], tableId: 104, triggerId: 1}
3535
- [[TriggerFunctionCall:{DescID: 104, TriggerID: 1}, PUBLIC], ABSENT]
3636
{funcArgs: [], funcBody: "DECLARE\nfoo @100106 := 'a';\nBEGIN\nINSERT INTO defaultdb.public.ab VALUES ((new).x, (new).y);\nRAISE NOTICE '% %', public.g(), nextval(108:::REGCLASS);\nRETURN new;\nEND;\n", funcId: 110, tableId: 104, triggerId: 1}
37-
- [[TriggerDeps:{DescID: 104, TriggerID: 1}, PUBLIC], ABSENT]
37+
- [[TriggerDeps:{DescID: 104, ReferencedTypeIDs: [106 107], TriggerID: 1, ReferencedFunctionIDs: [109 110]}, PUBLIC], ABSENT]
3838
{tableId: 104, triggerId: 1, usesRelations: [{columnIds: [1, 2], id: 105}, {id: 108}], usesRoutineIds: [109, 110], usesTypeIds: [106, 107]}
3939

4040
# TODO(#126362, #135655): uncomment this test case.

‎pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_object.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func init() {
129129
from.TypeFilter(rulesVersionKey, isTypeDescriptor),
130130
from.DescIDEq(fromDescID),
131131
to.ReferencedTypeDescIDsContain(fromDescID),
132-
to.TypeFilter(rulesVersionKey, isSimpleDependent, isWithTypeT),
132+
to.TypeFilter(rulesVersionKey, isSimpleDependent, isWithTypeTOrHasReferences),
133133
StatusesToAbsent(from, scpb.Status_DROPPED, to, scpb.Status_ABSENT),
134134
}
135135
},
@@ -168,7 +168,7 @@ func init() {
168168
)
169169

170170
registerDepRule(
171-
"descriptor drop right before removing dependent with function refs in columns",
171+
"descriptor drop right before removing dependent with function refs in columns / triggers",
172172
scgraph.SameStagePrecedence,
173173
"referenced-descriptor", "referencing-via-function",
174174
func(from, to NodeVars) rel.Clauses {
@@ -177,12 +177,11 @@ func init() {
177177
from.Type((*scpb.Function)(nil)),
178178
from.DescIDEq(fromDescID),
179179
to.ReferencedFunctionIDsContains(fromDescID),
180-
to.TypeFilter(rulesVersionKey, isSimpleDependent, isWithExpression),
180+
to.TypeFilter(rulesVersionKey, isSimpleDependent, isWithExpressionOrHasReferences),
181181
StatusesToAbsent(from, scpb.Status_DROPPED, to, scpb.Status_ABSENT),
182182
}
183183
},
184184
)
185-
186185
}
187186

188187
// These rules ensure that descriptor, back-reference in parent descriptor,

‎pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_trigger.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,36 @@ func init() {
2828
}
2929
},
3030
)
31+
32+
// Ensure that trigger dependencies are cleared before any external table
33+
// references.
34+
registerDepRuleForDrop("trigger references cleaned before columns",
35+
scgraph.Precedence,
36+
"trigger dependency", "column",
37+
scpb.Status_ABSENT, scpb.Status_WRITE_ONLY,
38+
func(from, to NodeVars) rel.Clauses {
39+
return rel.Clauses{
40+
from.Type((*scpb.TriggerDeps)(nil)),
41+
to.Type((*scpb.Column)(nil)),
42+
FilterElements("trigger refers to column", from, to, func(trigger *scpb.TriggerDeps, column *scpb.Column) bool {
43+
// Note: This rule is fairly expensive since it needs to join on
44+
// all columns in a plan. Sadly, the current embedding makes this
45+
// suboptimal so we can only use a filter here.
46+
for _, colRef := range trigger.UsesRelations {
47+
// Check if it refers to this table.
48+
if colRef.ID != column.TableID {
49+
continue
50+
}
51+
// Check if it refers to this column.
52+
for _, columnID := range colRef.ColumnIDs {
53+
if columnID == column.ColumnID {
54+
return true
55+
}
56+
}
57+
}
58+
// Otherwise, no references exist.
59+
return false
60+
}),
61+
}
62+
})
3163
}

‎pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,19 @@ func isWithTypeT(element scpb.Element) bool {
146146
return err == nil
147147
}
148148

149+
// isWithExpressionOrHasReferences returns true if `element` has an embedded
150+
// type or has references to types inside.
151+
func isWithTypeTOrHasReferences(element scpb.Element) bool {
152+
if isWithTypeT(element) {
153+
return true
154+
}
155+
switch element.(type) {
156+
case *scpb.TriggerDeps:
157+
return true
158+
}
159+
return false
160+
}
161+
149162
func getExpression(element scpb.Element) (*scpb.Expression, error) {
150163
switch e := element.(type) {
151164
case *scpb.ColumnType:
@@ -212,6 +225,19 @@ func isWithExpression(element scpb.Element) bool {
212225
return err == nil
213226
}
214227

228+
// isWithExpressionOrHasReferences returns true if `element` has an embedded
229+
// expression or has references to either types, functions or relations.
230+
func isWithExpressionOrHasReferences(element scpb.Element) bool {
231+
if isWithExpression(element) {
232+
return true
233+
}
234+
switch element.(type) {
235+
case *scpb.TriggerDeps:
236+
return true
237+
}
238+
return false
239+
}
240+
215241
func isTypeDescriptor(element scpb.Element) bool {
216242
switch element.(type) {
217243
case *scpb.EnumType, *scpb.AliasType, *scpb.CompositeType:

0 commit comments

Comments
 (0)