Merge #143827

143827: sql: disallow creating triggers which use TG_ARGV r=yuzefovich a=DrewKimball

The `TG_ARGV` trigger function argument is 0-indexed in Postgres. Since we currently don't have a way to replicate this behvaior, this commit disallows usage of `TG_ARGV` for now. The session setting `allow_create_trigger_function_with_argv_references` can be set to true to allow usage (with 1-based indexing).

Informs #135311

Release note (sql change): Usage of `TG_ARGV` in trigger functions is now disallowed by default.

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

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/triggers

@@ -4086,6 +4086,10 @@ DROP FUNCTION update_listing_balance ;
 statement ok
 drop table listings_balance cascade;
 
+# ==============================================================================
+# Regression tests.
+# ==============================================================================
+
 # Test that CREATE and DROP TRIGGER work when sent in a statement batch.
 
 subtest create_drop_in_batch_statement
@@ -4150,3 +4154,53 @@ SELECT id, a, b FROM tab_141810 ORDER BY id
 4  NULL  NULL
 
 subtest end
+
+# Until #135311 can be fixed, disallow TG_ARGV references by default. There is
+# a setting to allow them if necessary.
+subtest regression_135311
+
+statement ok
+DROP FUNCTION IF EXISTS g;
+
+statement ok
+CREATE FUNCTION g() RETURNS TRIGGER LANGUAGE PLpgSQL AS $$
+  BEGIN
+    RAISE NOTICE 'g()';
+    RAISE NOTICE 'TG_ARGV[1]: %', TG_ARGV[1];
+    RETURN NEW;
+  END
+$$;
+
+statement error pgcode 0A000 pq: unimplemented: referencing the TG_ARGV trigger function parameter is not yet supported
+CREATE TRIGGER foo BEFORE INSERT ON xy FOR EACH ROW EXECUTE FUNCTION g('foo', 'bar');
+
+statement ok
+SET allow_create_trigger_function_with_argv_references = true;
+
+statement ok
+CREATE TRIGGER foo BEFORE INSERT ON xy FOR EACH ROW EXECUTE FUNCTION g('foo', 'bar');
+
+query T noticetrace
+INSERT INTO xy VALUES (1, 2);
+----
+NOTICE: g()
+NOTICE: TG_ARGV[1]: foo
+
+statement ok
+RESET allow_create_trigger_function_with_argv_references;
+
+# The setting only disallows creating the trigger - not executing it if it was
+# already created.
+query T noticetrace
+INSERT INTO xy VALUES (3, 4);
+----
+NOTICE: g()
+NOTICE: TG_ARGV[1]: foo
+
+statement ok
+DROP TRIGGER foo ON xy;
+
+statement ok
+DROP FUNCTION g;
+
+subtest end

pkg/sql/exec_util.go

@@ -4119,6 +4119,10 @@ func (m *sessionDataMutator) SetOptimizerUseDeleteRangeFastPath(val bool) {
 	m.data.OptimizerUseDeleteRangeFastPath = val
 }
 
+func (m *sessionDataMutator) SetAllowCreateTriggerFunctionWithArgvReferences(val bool) {
+	m.data.AllowCreateTriggerFunctionWithArgvReferences = val
+}
+
 // Utility functions related to scrubbing sensitive information on SQL Stats.
 
 // quantizeCounts ensures that the Count field in the

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -3902,6 +3902,7 @@ WHERE
 ORDER BY variable
 ----
 variable                                                   value
+allow_create_trigger_function_with_argv_references         off
 allow_ordinal_column_references                            off
 allow_role_memberships_to_change_during_transaction        off
 alter_primary_region_super_region_override                 off

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -2911,6 +2911,7 @@ WHERE
 ORDER BY name
 ----
 name                                                       setting             category  short_desc  extra_desc  vartype
+allow_create_trigger_function_with_argv_references         off                 NULL      NULL        NULL        string
 allow_ordinal_column_references                            off                 NULL      NULL        NULL        string
 allow_role_memberships_to_change_during_transaction        off                 NULL      NULL        NULL        string
 alter_primary_region_super_region_override                 off                 NULL      NULL        NULL        string
@@ -3127,6 +3128,7 @@ WHERE
 ORDER BY name
 ----
 name                                                       setting             unit  context  enumvals  boot_val            reset_val
+allow_create_trigger_function_with_argv_references         off                 NULL  user     NULL      off                 off
 allow_ordinal_column_references                            off                 NULL  user     NULL      off                 off
 allow_role_memberships_to_change_during_transaction        off                 NULL  user     NULL      off                 off
 alter_primary_region_super_region_override                 off                 NULL  user     NULL      off                 off
@@ -3336,6 +3338,7 @@ query TTTTTT colnames,rowsort
 SELECT name, source, min_val, max_val, sourcefile, sourceline FROM pg_catalog.pg_settings
 ----
 name                                                       source  min_val  max_val  sourcefile  sourceline
+allow_create_trigger_function_with_argv_references         NULL    NULL     NULL     NULL        NULL
 allow_ordinal_column_references                            NULL    NULL     NULL     NULL        NULL
 allow_role_memberships_to_change_during_transaction        NULL    NULL     NULL     NULL        NULL
 alter_primary_region_super_region_override                 NULL    NULL     NULL     NULL        NULL

pkg/sql/logictest/testdata/logic_test/show_source

@@ -27,6 +27,7 @@ WHERE variable NOT IN ('optimizer', 'crdb_version', 'session_id', 'distsql_workm
 ORDER BY variable
 ----
 variable                                                   value
+allow_create_trigger_function_with_argv_references         off
 allow_ordinal_column_references                            off
 allow_role_memberships_to_change_during_transaction        off
 alter_primary_region_super_region_override                 off

pkg/sql/opt/optbuilder/create_function.go

@@ -428,6 +428,7 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateRoutine, inScope *scope) (o
 
 		// Special handling for trigger functions.
 		buildSQL := true
+		isTriggerFn := false
 		if funcReturnType.Identical(types.Trigger) {
 			// Trigger functions cannot have user-defined parameters. However, they do
 			// have a set of implicitly defined parameters.
@@ -449,15 +450,16 @@ func (b *Builder) buildCreateFunction(cf *tree.CreateRoutine, inScope *scope) (o
 			// Analysis of SQL expressions for trigger functions must be deferred
 			// until the function is bound to a trigger.
 			buildSQL = false
+			isTriggerFn = true
 		}
 
 		// We need to disable stable function folding because we want to catch the
 		// volatility of stable functions. If folded, we only get a scalar and lose
 		// the volatility.
 		b.factory.FoldingControl().TemporarilyDisallowStableFolds(func() {
 			plBuilder := newPLpgSQLBuilder(
-				b, cf.Name.Object(), stmt.AST.Label, nil /* colRefs */, routineParams,
-				funcReturnType, cf.IsProcedure, false /* isDoBlock */, buildSQL, nil, /* outScope */
+				b, cf.Name.Object(), stmt.AST.Label, nil /* colRefs */, routineParams, funcReturnType,
+				cf.IsProcedure, false /* isDoBlock */, isTriggerFn, buildSQL, nil, /* outScope */
 			)
 			stmtScope = plBuilder.buildRootBlock(stmt.AST, bodyScope, routineParams)
 		})

pkg/sql/opt/optbuilder/create_trigger.go

@@ -216,15 +216,20 @@ func (b *Builder) buildFunctionForTrigger(
 	// The trigger function takes a set of implicitly-defined parameters, two of
 	// which are determined by the table's record type. Add them to the trigger
 	// function scope.
-	numStaticParams := len(triggerFuncStaticParams)
-	triggerFuncParams := make([]routineParam, numStaticParams, numStaticParams+2)
-	copy(triggerFuncParams, triggerFuncStaticParams)
+	triggerFuncParams := make([]routineParam, 0, len(triggerFuncStaticParams)+2)
 	triggerFuncParams = append(triggerFuncParams, routineParam{name: triggerColNew, typ: tableTyp})
 	triggerFuncParams = append(triggerFuncParams, routineParam{name: triggerColOld, typ: tableTyp})
+	triggerFuncParams = append(triggerFuncParams, triggerFuncStaticParams...)
 	for i, param := range triggerFuncParams {
 		paramColName := funcParamColName(param.name, i)
 		col := b.synthesizeColumn(funcScope, paramColName, param.typ, nil /* expr */, nil /* scalar */)
 		col.setParamOrd(i)
+		if i == triggerArgvColIdx {
+			// Due to #135311, we disallow references to the TG_ARGV param for now.
+			if !b.evalCtx.SessionData().AllowCreateTriggerFunctionWithArgvReferences {
+				col.resolveErr = unimplementedArgvErr
+			}
+		}
 	}
 
 	// Now that the transition relations and table type are known, fully build and
@@ -240,7 +245,8 @@ func (b *Builder) buildFunctionForTrigger(
 	b.factory.FoldingControl().TemporarilyDisallowStableFolds(func() {
 		plBuilder := newPLpgSQLBuilder(
 			b, ct.FuncName.String(), stmt.AST.Label, nil /* colRefs */, triggerFuncParams, tableTyp,
-			false /* isProcedure */, false /* isDoBlock */, true /* buildSQL */, nil, /* outScope */
+			false /* isProcedure */, false, /* isDoBlock */
+			true /* isTriggerFn */, true /* buildSQL */, nil, /* outScope */
 		)
 		funcScope = plBuilder.buildRootBlock(stmt.AST, funcScope, triggerFuncParams)
 	})
@@ -330,6 +336,10 @@ var triggerFuncStaticParams = []routineParam{
 	{name: "tg_argv", typ: types.StringArray, class: tree.RoutineParamIn},
 }
 
+// The trigger function parameters consist of OLD and NEW, followed by the
+// static parameters. The TG_ARGV parameter is last.
+var triggerArgvColIdx = len(triggerFuncStaticParams) + 1
+
 const triggerColNew = "new"
 const triggerColOld = "old"
 
@@ -369,4 +379,6 @@ var (
 		"column lists are not yet supported for triggers")
 	unimplementedViewTriggerErr = unimplemented.NewWithIssue(135658,
 		"triggers on views are not yet supported")
+	unimplementedArgvErr = unimplemented.NewWithIssue(135311,
+		"referencing the TG_ARGV trigger function parameter is not yet supported")
 )

pkg/sql/opt/optbuilder/plpgsql.go

@@ -192,6 +192,7 @@ type plpgsqlBuilder struct {
 	routineName  string
 	isProcedure  bool
 	isDoBlock    bool
+	isTriggerFn  bool
 	buildSQL     bool
 	identCounter int
 }
@@ -209,7 +210,7 @@ func newPLpgSQLBuilder(
 	colRefs *opt.ColSet,
 	routineParams []routineParam,
 	returnType *types.T,
-	isProcedure, isDoBlock, buildSQL bool,
+	isProcedure, isDoBlock, isTriggerFn, buildSQL bool,
 	outScope *scope,
 ) *plpgsqlBuilder {
 	const initialBlocksCap = 2
@@ -221,6 +222,7 @@ func newPLpgSQLBuilder(
 		routineName: routineName,
 		isProcedure: isProcedure,
 		isDoBlock:   isDoBlock,
+		isTriggerFn: isTriggerFn,
 		buildSQL:    buildSQL,
 		outScope:    outScope,
 	}
@@ -2017,7 +2019,14 @@ func (b *plpgsqlBuilder) makeContinuation(conName string) continuation {
 		col := b.ob.synthesizeColumn(s, name, typ, nil /* expr */, nil /* scalar */)
 		// TODO(mgartner): Lift the 100 parameter restriction for synthesized
 		// continuation UDFs.
+		paramOrd := len(params)
 		col.setParamOrd(len(params))
+		if b.ob.insideFuncDef && b.isTriggerFn && paramOrd == triggerArgvColIdx {
+			// Due to #135311, we disallow references to the TG_ARGV param for now.
+			if !b.ob.evalCtx.SessionData().AllowCreateTriggerFunctionWithArgvReferences {
+				col.resolveErr = unimplementedArgvErr
+			}
+		}
 		params = append(params, col.id)
 	}
 	// Invariant: the variables of a child block always follow those of a parent
@@ -2166,7 +2175,9 @@ func (b *plpgsqlBuilder) makeContinuationArgs(con *continuation, s *scope) memo.
 		block := &b.blocks[i]
 		for _, name := range block.vars {
 			_, source, _, err := s.FindSourceProvidingColumn(b.ob.ctx, name)
-			if err != nil {
+			if err != nil && !errors.Is(err, unimplementedArgvErr) {
+				// Swallow unimplementedArgvErr, since it's ok to reference the TG_ARGV
+				// parameter when calling a continuation.
 				panic(err)
 			}
 			args = append(args, b.ob.factory.ConstructVariable(source.(*scopeColumn).id))

pkg/sql/opt/optbuilder/routine.go

@@ -447,7 +447,7 @@ func (b *Builder) buildRoutine(
 		var physProps *physical.Required
 		plBuilder := newPLpgSQLBuilder(
 			b, def.Name, stmt.AST.Label, colRefs, routineParams, f.ResolvedType(),
-			isProc, false /* isDoBlock */, true /* buildSQL */, outScope,
+			isProc, false /* isDoBlock */, false /* isTriggerFn */, true /* buildSQL */, outScope,
 		)
 		stmtScope := plBuilder.buildRootBlock(stmt.AST, bodyScope, routineParams)
 		rTyp := b.finalizeRoutineReturnType(f, stmtScope, inScope, oldInsideDataSource)
@@ -859,7 +859,8 @@ func (b *Builder) buildPLpgSQLDoBody(
 	// Build an expression for each statement in the function body.
 	plBuilder := newPLpgSQLBuilder(
 		b, doBlockRoutineName, do.Block.Label, nil /* colRefs */, nil /* routineParams */, types.Void,
-		true /* isProc */, true /* isDoBlock */, true /* buildSQL */, nil, /* outScope */
+		true /* isProc */, true, /* isDoBlock */
+		false /* isTriggerFn */, true /* buildSQL */, nil, /* outScope */
 	)
 	// Allocate a fresh scope, since DO blocks do not take parameters or reference
 	// variables or columns from the calling context.

pkg/sql/opt/optbuilder/scope.go

@@ -872,7 +872,7 @@ func (s *scope) FindSourceProvidingColumn(
 			if candidate.ambiguous {
 				return nil, nil, -1, s.newAmbiguousColumnError(colName, candidate.matchClass)
 			}
-			return &col.table, col, int(col.id), nil
+			return &col.table, col, int(col.id), col.resolveErr
 		}
 		// No matches in this scope; proceed to the parent scope.
 	}
@@ -1015,7 +1015,7 @@ func (s *scope) Resolve(
 		if col.visibility != inaccessible &&
 			col.name.MatchesReferenceName(colName) &&
 			sourceNameMatches(*prefix, col.table) {
-			return col, nil
+			return col, col.resolveErr
 		}
 	}