sql: avoid unnecessary column dependencies in routines

DrewKimball · DrewKimball · commit a5e9fbe2d292 · 2025-12-08T11:17:26.000-06:00
Previously, there were several cases where routines would add unnecessary column dependencies: * Columns referenced by a computed column expression would be added to the list of columns referenced by the routine. * BEFORE triggers would confuse the logic used to distinguish explicit from implicit INSERT columns, because triggers change the column IDs to be inserted. This could cause the routine to add implicitly referenced columns to its dependencies. * Dependencies from statements within a trigger would transitively apply to a routine that invoked trigger on a mutation. This commit prevents newly-created routines from adding unnecessary column dependencies in those cases, gated behind the session setting `use_improved_routine_deps_triggers_and_computed_cols`. Fixes #158154 Release note (bug fix): Fixed a bug that caused routines to prevent dropping more columns than necessary, most notably columns referenced by computed column expressions. The fix is gated behind the session setting `use_improved_routine_deps_triggers_and_computed_cols`, which is off by default prior to v26.1.
diff --git a/pkg/ccl/logictestccl/testdata/logic_test/triggers b/pkg/ccl/logictestccl/testdata/logic_test/triggers
@@ -5121,6 +5121,67 @@ DROP TRIGGER tr1 ON sc_tr_tbl;
 
 subtest end
 
+# Regression test for unnecessary column dependencies added to a routine with a
+# mutation that involves a trigger.
+subtest regression_158154
+
+statement ok
+SET use_improved_routine_deps_triggers_and_computed_cols = true;
+
+statement ok
+CREATE TABLE xy_158154 (x INT, y INT);
+
+statement ok
+CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
+  BEGIN
+    SELECT x, y FROM xy_158154;
+    RAISE NOTICE '%: old: %, new: %', TG_OP, OLD, NEW;
+    RETURN COALESCE(NEW, OLD);
+  END
+$$;
+
+statement ok
+CREATE TRIGGER foo BEFORE INSERT ON xy_158154 FOR EACH ROW EXECUTE FUNCTION g();
+
+statement ok
+CREATE PROCEDURE p(blah INT) LANGUAGE SQL AS $$
+  INSERT INTO xy_158154 (x) VALUES (blah);
+$$;
+
+statement ok
+CALL p(100)
+
+statement error pgcode 2BP01 pq: cannot drop column "y" because trigger "foo" on table "xy_158154" depends on it
+ALTER TABLE xy_158154 DROP COLUMN y;
+
+statement ok
+DROP TRIGGER foo ON xy_158154;
+DROP FUNCTION g();
+
+# The procedure should not prevent the column drop.
+statement ok
+ALTER TABLE xy_158154 DROP COLUMN y;
+
+statement ok
+CALL p(200)
+
+query I rowsort
+SELECT * FROM xy_158154;
+----
+100
+200
+
+statement ok
+DROP PROCEDURE p
+
+statement ok
+DROP TABLE xy_158154
+
+statement ok
+RESET use_improved_routine_deps_triggers_and_computed_cols;
+
+subtest end
+
 # ==============================================================================
 # End of regression tests. Add new features above the regression test section.
 # ==============================================================================
diff --git a/pkg/sql/logictest/testdata/logic_test/information_schema b/pkg/sql/logictest/testdata/logic_test/information_schema
@@ -4285,6 +4285,7 @@ unconstrained_non_covering_index_scan_enabled                    off
 unsafe_allow_triggers_modifying_cascades                         off
 use_cputs_on_non_unique_indexes                                  off
 use_improved_routine_dependency_tracking                         on
+use_improved_routine_deps_triggers_and_computed_cols             off
 use_pre_25_2_variadic_builtins                                   off
 use_proc_txn_control_extended_protocol_fix                       on
 use_soft_limit_for_distribute_scan                               on
diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog
@@ -3241,6 +3241,7 @@ unsafe_allow_triggers_modifying_cascades                         off
 use_cputs_on_non_unique_indexes                                  off                 NULL      NULL        NULL        string
 use_declarative_schema_changer                                   on                  NULL      NULL        NULL        string
 use_improved_routine_dependency_tracking                         on                  NULL      NULL        NULL        string
+use_improved_routine_deps_triggers_and_computed_cols             off                 NULL      NULL        NULL        string
 use_pre_25_2_variadic_builtins                                   off                 NULL      NULL        NULL        string
 use_proc_txn_control_extended_protocol_fix                       on                  NULL      NULL        NULL        string
 use_soft_limit_for_distribute_scan                               on                  NULL      NULL        NULL        string
@@ -3492,6 +3493,7 @@ unsafe_allow_triggers_modifying_cascades                         off
 use_cputs_on_non_unique_indexes                                  off                 NULL  user     NULL      off                 off
 use_declarative_schema_changer                                   on                  NULL  user     NULL      on                  on
 use_improved_routine_dependency_tracking                         on                  NULL  user     NULL      on                  on
+use_improved_routine_deps_triggers_and_computed_cols             off                 NULL  user     NULL      off                 off
 use_pre_25_2_variadic_builtins                                   off                 NULL  user     NULL      off                 off
 use_proc_txn_control_extended_protocol_fix                       on                  NULL  user     NULL      on                  on
 use_soft_limit_for_distribute_scan                               on                  NULL  user     NULL      on                  on
@@ -3735,6 +3737,7 @@ unsafe_allow_triggers_modifying_cascades                         NULL    NULL
 use_cputs_on_non_unique_indexes                                  NULL    NULL     NULL     NULL        NULL
 use_declarative_schema_changer                                   NULL    NULL     NULL     NULL        NULL
 use_improved_routine_dependency_tracking                         NULL    NULL     NULL     NULL        NULL
+use_improved_routine_deps_triggers_and_computed_cols             NULL    NULL     NULL     NULL        NULL
 use_pre_25_2_variadic_builtins                                   NULL    NULL     NULL     NULL        NULL
 use_proc_txn_control_extended_protocol_fix                       NULL    NULL     NULL     NULL        NULL
 use_soft_limit_for_distribute_scan                               NULL    NULL     NULL     NULL        NULL
diff --git a/pkg/sql/logictest/testdata/logic_test/show_source b/pkg/sql/logictest/testdata/logic_test/show_source
@@ -258,6 +258,7 @@ unsafe_allow_triggers_modifying_cascades                         off
 use_cputs_on_non_unique_indexes                                  off
 use_declarative_schema_changer                                   on
 use_improved_routine_dependency_tracking                         on
+use_improved_routine_deps_triggers_and_computed_cols             off
 use_pre_25_2_variadic_builtins                                   off
 use_proc_txn_control_extended_protocol_fix                       on
 use_soft_limit_for_distribute_scan                               on
diff --git a/pkg/sql/logictest/testdata/logic_test/udf_insert b/pkg/sql/logictest/testdata/logic_test/udf_insert
@@ -235,9 +235,20 @@ SELECT f_drop(5,100,101);
 ----
 (5,100,101)
 
-statement error pgcode 2BP01 cannot drop column \"c\" because function \"f_drop\" depends on it
+statement error pgcode 2BP01 pq: cannot drop column "c" because function "f_drop" depends on it
 ALTER TABLE t_alter DROP COLUMN c;
 
+statement ok
+DROP FUNCTION f_drop;
+
+# Columns that aren't explicitly referenced by a routine can be dropped.
+statement ok
+ALTER TABLE t_alter DROP COLUMN c;
+ALTER TABLE t_alter DROP COLUMN b;
+
+statement error pgcode 2BP01 pq: cannot drop column "a" because function "f_int" depends on it
+ALTER TABLE t_alter DROP COLUMN a;
+
 query T
 SELECT f_record(6);
 ----
diff --git a/pkg/sql/logictest/testdata/logic_test/udf_update b/pkg/sql/logictest/testdata/logic_test/udf_update
@@ -412,3 +412,46 @@ statement ok
 RESET prevent_update_set_column_drop;
 
 subtest end
+
+# Allow drop of columns referenced only in computed column expressions.
+subtest regression_158154
+
+statement ok
+SET use_improved_routine_deps_triggers_and_computed_cols = true;
+
+statement ok
+CREATE TABLE t158154 (col1 INT8 NOT NULL, col2 TIMESTAMP NOT NULL, computed TIMESTAMP NOT NULL AS (col2) STORED);
+INSERT INTO t158154 (col1, col2) VALUES (1, '2024-01-01');
+
+statement ok
+CREATE OR REPLACE PROCEDURE p158154() LANGUAGE PLpgSQL AS $proc$
+  BEGIN UPDATE t158154 SET col1 = col1 + 1; END;
+$proc$;
+
+statement ok
+CALL p158154()
+
+query ITT
+SELECT * FROM t158154
+----
+2  2024-01-01 00:00:00 +0000 +0000  2024-01-01 00:00:00 +0000 +0000
+
+statement ok
+ALTER TABLE t158154 DROP COLUMN computed, DROP COLUMN col2;
+
+statement ok
+CALL p158154()
+
+query I
+SELECT * FROM t158154
+----
+3
+
+statement ok
+DROP PROCEDURE p158154;
+DROP TABLE t158154;
+
+statement ok
+RESET use_improved_routine_deps_triggers_and_computed_cols;
+
+subtest end
diff --git a/pkg/sql/opt/memo/memo.go b/pkg/sql/opt/memo/memo.go
@@ -218,6 +218,7 @@ type Memo struct {
 	usingHintInjection                         bool
 	useSwapMutations                           bool
 	preventUpdateSetColumnDrop                 bool
+	useImprovedRoutineDepsTriggersComputedCols bool
 
 	// txnIsoLevel is the isolation level under which the plan was created. This
 	// affects the planning of some locking operations, so it must be included in
@@ -336,6 +337,7 @@ func (m *Memo) Init(ctx context.Context, evalCtx *eval.Context) {
 		usingHintInjection:                         evalCtx.Planner != nil && evalCtx.Planner.UsingHintInjection(),
 		useSwapMutations:                           evalCtx.SessionData().UseSwapMutations,
 		preventUpdateSetColumnDrop:                 evalCtx.SessionData().PreventUpdateSetColumnDrop,
+		useImprovedRoutineDepsTriggersComputedCols: evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols,
 		txnIsoLevel:                                evalCtx.TxnIsoLevel,
 	}
 	m.metadata.Init()
@@ -518,6 +520,7 @@ func (m *Memo) IsStale(
 		m.usingHintInjection != (evalCtx.Planner != nil && evalCtx.Planner.UsingHintInjection()) ||
 		m.useSwapMutations != evalCtx.SessionData().UseSwapMutations ||
 		m.preventUpdateSetColumnDrop != evalCtx.SessionData().PreventUpdateSetColumnDrop ||
+		m.useImprovedRoutineDepsTriggersComputedCols != evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols ||
 		m.txnIsoLevel != evalCtx.TxnIsoLevel {
 		return true, nil
 	}
diff --git a/pkg/sql/opt/memo/memo_test.go b/pkg/sql/opt/memo/memo_test.go
@@ -617,6 +617,12 @@ func TestMemoIsStale(t *testing.T) {
 	evalCtx.SessionData().PreventUpdateSetColumnDrop = false
 	notStale()
 
+	// Stale use_improved_routine_deps_triggers_and_computed_cols.
+	evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols = true
+	stale()
+	evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols = false
+	notStale()
+
 	// User no longer has access to view.
 	catalog.View(tree.NewTableNameWithSchema("t", catconstants.PublicSchemaName, "abcview")).Revoked = true
 	_, err = o.Memo().IsStale(ctx, &evalCtx, catalog)
diff --git a/pkg/sql/opt/optbuilder/builder.go b/pkg/sql/opt/optbuilder/builder.go
@@ -601,6 +601,27 @@ func (b *Builder) DisableUnsafeInternalCheck() func() {
 	}
 }
 
+// DisableSchemaDepTracking is used to disable dependency tracking for views and
+// routines, so that users don't have to face unnecessary restrictions during
+// schema changes.
+//
+// For example, we must prevent dropping a column that is referenced in the
+// WHERE clause or SET clause of an UPDATE statement. However, adding or
+// dropping columns that are synthesized with default or computed values is
+// perfectly safe, because those columns are determined from the table schema
+// rather than being user specified. If such a column is dropped, its value will
+// simply not be synthesized in mutation statements going forward.
+func (b *Builder) DisableSchemaDepTracking() func() {
+	if !b.trackSchemaDeps {
+		return func() {}
+	}
+	originalTrackSchemaDeps := b.trackSchemaDeps
+	b.trackSchemaDeps = false
+	return func() {
+		b.trackSchemaDeps = originalTrackSchemaDeps
+	}
+}
+
 // optTrackingTypeResolver is a wrapper around a TypeReferenceResolver that
 // remembers all of the resolved types in the provided Metadata.
 type optTrackingTypeResolver struct {
diff --git a/pkg/sql/opt/optbuilder/insert.go b/pkg/sql/opt/optbuilder/insert.go
@@ -398,7 +398,10 @@ func (b *Builder) buildInsert(ins *tree.Insert, inScope *scope) (outScope *scope
 		mb.buildUpsert(returning)
 	}
 
-	if b.trackSchemaDeps {
+	mb.trackTargetColDeps()
+
+	// Legacy path for resolving insert target column dependencies.
+	if b.trackSchemaDeps && !b.evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols {
 		dep := opt.SchemaDep{DataSource: tab}
 		// Track dependencies on insert columns.
 		for i := range mb.insertColIDs {
diff --git a/pkg/sql/opt/optbuilder/mutation_builder.go b/pkg/sql/opt/optbuilder/mutation_builder.go
@@ -93,6 +93,8 @@ type mutationBuilder struct {
 	// explicit values in the insert statement, if b.trackSchemaDeps is true.
 	// It does not include columns that were explicitly given the value of
 	// DEFAULT, e.g., INSERT INTO t VALUES (1, DEFAULT).
+	// TODO(drewk): Remove this once we make the improved dependency tracking
+	// unconditional and use_improved_routine_dependency_tracking becomes a no-op.
 	implicitInsertCols opt.ColSet
 
 	// fetchColIDs lists the input column IDs storing values which are fetched
@@ -827,6 +829,14 @@ func (mb *mutationBuilder) addSynthesizedDefaultCols(
 func (mb *mutationBuilder) addSynthesizedComputedCols(
 	colIDs opt.OptionalColList, targetColList *opt.ColList, targetColSet *opt.ColSet, restrict bool,
 ) {
+	if mb.b.evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols {
+		// Avoid adding unnecessary dependencies on columns that are referenced by
+		// computed column expressions. We only need to track columns that were
+		// explicitly specified by the user, e.g. those in SET, WHERE or RETURNING
+		// clause, or the target columns of an INSERT.
+		defer mb.b.DisableSchemaDepTracking()()
+	}
+
 	// We will construct a new Project operator that will contain the newly
 	// synthesized column(s).
 	pb := makeProjectionBuilder(mb.b, mb.outScope)
@@ -945,6 +955,13 @@ func (mb *mutationBuilder) maybeAddRegionColLookup(op opt.Operator) {
 			return
 		}
 	}
+	if mb.b.evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols {
+		// It is not necessary to add transitive dependencies on the objects
+		// resolved below, since the schema changer already ensures that they are
+		// not dropped if "infer_rbr_region_col_using_constraint" is set.
+		defer mb.b.DisableSchemaDepTracking()()
+	}
+
 	// Resolve the referenced table.
 	refTabDescID := int64(lookupFK.ReferencedTableID())
 	refTab := mb.b.resolveTableRef(&tree.TableRef{TableID: refTabDescID}, privilege.SELECT)
diff --git a/pkg/sql/opt/optbuilder/partial_index.go b/pkg/sql/opt/optbuilder/partial_index.go
@@ -31,12 +31,7 @@ func (b *Builder) addPartialIndexPredicatesForTable(tabMeta *opt.TableMeta, scan
 	// depending on a table with a partial index predicate using an UDT will
 	// result in a type dependency being added between the view/function and the
 	// UDT.
-	if b.trackSchemaDeps {
-		b.trackSchemaDeps = false
-		defer func() {
-			b.trackSchemaDeps = true
-		}()
-	}
+	defer b.DisableSchemaDepTracking()()
 	tab := tabMeta.Table
 	numIndexes := tab.DeletableIndexCount()
 
diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go
@@ -353,13 +353,10 @@ func (b *Builder) buildView(
 		b.skipSelectPrivilegeChecks = true
 		defer func() { b.skipSelectPrivilegeChecks = false }()
 	}
-	trackDeps := b.trackSchemaDeps
-	if trackDeps {
-		// We are only interested in the direct dependency on this view descriptor.
-		// Any further dependency by the view's query should not be tracked.
-		b.trackSchemaDeps = false
-		defer func() { b.trackSchemaDeps = true }()
-	}
+	// We are only interested in the direct dependency on this view descriptor.
+	// Any further dependency by the view's query should not be tracked.
+	trackViewDep := b.trackSchemaDeps
+	defer b.DisableSchemaDepTracking()()
 
 	// We don't want the view to be able to refer to any outer scopes in the
 	// query. This shouldn't happen if the view is valid but there may be
@@ -382,7 +379,7 @@ func (b *Builder) buildView(
 		}
 	}
 
-	if trackDeps && !view.IsSystemView() {
+	if trackViewDep && !view.IsSystemView() {
 		dep := opt.SchemaDep{DataSource: view}
 		for i := range outScope.cols {
 			dep.ColumnOrdinals.Add(i)
@@ -807,12 +804,7 @@ func (b *Builder) addCheckConstraintsForTable(tabMeta *opt.TableMeta) {
 	// track view deps here, or else a view depending on a table with a
 	// column that is a UDT will result in a type dependency being added
 	// between the view and the UDT, even if the view does not use that column.
-	if b.trackSchemaDeps {
-		b.trackSchemaDeps = false
-		defer func() {
-			b.trackSchemaDeps = true
-		}()
-	}
+	defer b.DisableSchemaDepTracking()()
 	tab := tabMeta.Table
 
 	// Check if we have any validated check constraints. Only validated
@@ -898,16 +890,11 @@ func (b *Builder) addCheckConstraintsForTable(tabMeta *opt.TableMeta) {
 func (b *Builder) addComputedColsForTable(
 	tabMeta *opt.TableMeta, includeVirtualMutationColOrds intsets.Fast,
 ) {
-	// We do not want to track view deps here, otherwise a view depending
-	// on a table with a computed column of a UDT will result in a
-	// type dependency being added between the view and the UDT,
-	// even if the view does not use that column.
-	if b.trackSchemaDeps {
-		b.trackSchemaDeps = false
-		defer func() {
-			b.trackSchemaDeps = true
-		}()
-	}
+	// We do not want to track view/routine deps here, otherwise a view/routine
+	// depending on a table with a computed column of a UDT will result in a
+	// type dependency being added between the view/routine and the UDT,
+	// even if the view/routine does not use that column.
+	defer b.DisableSchemaDepTracking()()
 	var tableScope *scope
 	tab := tabMeta.Table
 	for i, n := 0, tab.ColumnCount(); i < n; i++ {
diff --git a/pkg/sql/opt/optbuilder/trigger.go b/pkg/sql/opt/optbuilder/trigger.go
@@ -38,6 +38,11 @@ import (
 func (mb *mutationBuilder) buildRowLevelBeforeTriggers(
 	eventType tree.TriggerEventType, cascade bool,
 ) bool {
+	if mb.b.evalCtx.SessionData().UseImprovedRoutineDepsTriggersAndComputedCols {
+		// Avoid adding transitive dependencies that are already tracked in the
+		// trigger object.
+		defer mb.b.DisableSchemaDepTracking()()
+	}
 	var eventsToMatch tree.TriggerEventTypeSet
 	eventsToMatch.Add(eventType)
 	triggers := cat.GetRowLevelTriggers(mb.tab, tree.TriggerActionTimeBefore, eventsToMatch)
diff --git a/pkg/sql/sessiondatapb/local_only_session_data.proto b/pkg/sql/sessiondatapb/local_only_session_data.proto
@@ -766,6 +766,11 @@ message LocalOnlySessionData {
   // referenced only as target columns in an UPDATE statement within a routine
   // are correctly tracked, preventing a drop that breaks the routine.
   bool prevent_update_set_column_drop = 195;
+  // UseImprovedRoutineDepsTriggersAndComputedCols, when true, enables improved
+  // dependency tracking for routines that perform mutations involving BEFORE
+  // triggers or computed columns. The fix applies at routine creation time and
+  // prevents unnecessary column dependencies from being recorded.
+  bool use_improved_routine_deps_triggers_and_computed_cols = 196;
 
   ///////////////////////////////////////////////////////////////////////////
   // WARNING: consider whether a session parameter you're adding needs to  //
diff --git a/pkg/sql/sessionmutator/mutator.go b/pkg/sql/sessionmutator/mutator.go
diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go
