From e4d03f685f662c5a8a10e33499b39536eabf3a53 Mon Sep 17 00:00:00 2001 From: virajchogle Date: Mon, 8 Jun 2026 12:41:39 -0400 Subject: [PATCH 1/2] sql: reject volatility weakening when IMMUTABLE-required dependent exists CREATE OR REPLACE FUNCTION silently accepted weakening a function's volatility while computed columns, expression indexes, or CHECK constraints still required IMMUTABLE, crashing subsequent reads in the optimizer. Reject the replacement in both schema changers and add a nil-check at the virtual-column projection for already-corrupt clusters. Fixes #171486 Epic: none Release note (bug fix): CREATE OR REPLACE FUNCTION now rejects weakening a function's volatility when it is referenced by a computed column,expression index, or CHECK constraint. --- pkg/sql/create_function.go | 29 ++++++++ .../testdata/logic_test/udf_in_table | 67 +++++++++++++++++++ pkg/sql/opt/optbuilder/select.go | 9 ++- .../internal/scbuildstmt/create_function.go | 67 +++++++++++++++++++ 4 files changed, 171 insertions(+), 1 deletion(-) diff --git a/pkg/sql/create_function.go b/pkg/sql/create_function.go index 63b9c80a311f..e77e4a490f89 100644 --- a/pkg/sql/create_function.go +++ b/pkg/sql/create_function.go @@ -269,6 +269,9 @@ func (n *createFunctionNode) replaceFunction( if err := setFuncOptions(params, udfDesc, n.cf.Options); err != nil { return err } + if err := validateVolatilityForDependents(params.ctx, params.p, udfDesc); err != nil { + return err + } // Removing all existing references before adding new references. for _, id := range udfDesc.DependsOn { @@ -673,6 +676,32 @@ func validateVolatilityInOptions( return nil } +// validateVolatilityForDependents rejects a volatility weakening when the +// function is referenced by an IMMUTABLE-requiring dependent (computed +// column, expression index, or CHECK constraint). +func validateVolatilityForDependents( + ctx context.Context, p *planner, udfDesc catalog.FunctionDescriptor, +) error { + if udfDesc.GetVolatility() == catpb.Function_IMMUTABLE { + return nil + } + for _, ref := range udfDesc.GetDependedOnBy() { + if len(ref.ColumnIDs) == 0 && len(ref.IndexIDs) == 0 && len(ref.ConstraintIDs) == 0 { + continue + } + depDesc, err := p.Descriptors().ByIDWithoutLeased(p.txn).WithoutNonPublic().Get().Table(ctx, ref.ID) + if err != nil { + return err + } + return pgerror.Newf( + pgcode.DependentObjectsStillExist, + "cannot change volatility of function %q because it is referenced by table %q in a context requiring IMMUTABLE", + udfDesc.GetName(), depDesc.GetName(), + ) + } + return nil +} + // validateParameters checks that changes to the parameters, if any, are // allowed. This method expects that the return type equality has already been // checked. diff --git a/pkg/sql/logictest/testdata/logic_test/udf_in_table b/pkg/sql/logictest/testdata/logic_test/udf_in_table index 253af10f2de9..e4031d4010cd 100644 --- a/pkg/sql/logictest/testdata/logic_test/udf_in_table +++ b/pkg/sql/logictest/testdata/logic_test/udf_in_table @@ -882,3 +882,70 @@ CREATE FUNCTION f_circle() RETURNS INT LANGUAGE SQL AS $$ SELECT a FROM t_circle statement error pgcode 42P13 pq: .*cannot add dependency from descriptor \d+ to function f_circle \(\d+\) because there will be a dependency cycle.* ALTER TABLE t_circle ALTER COLUMN b SET DEFAULT f_circle(); + +# Verify that CREATE OR REPLACE FUNCTION refuses to weaken volatility when an +# existing dependent uses the function in a context requiring IMMUTABLE. + +statement ok +CREATE FUNCTION f_vol() RETURNS INT LANGUAGE SQL IMMUTABLE AS $$ SELECT 1 $$ + +# Virtual computed column dependent. +statement ok +CREATE TABLE t_replace_vol_virt (a INT PRIMARY KEY, b INT AS (f_vol()) VIRTUAL) + +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_virt" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL VOLATILE AS $$ SELECT 2 $$ + +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_virt" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL STABLE AS $$ SELECT 2 $$ + +# Omitting the volatility option implicitly defaults to VOLATILE. +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_virt" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL AS $$ SELECT 2 $$ + +# Re-specifying IMMUTABLE preserves the invariant and is allowed. +statement ok +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL IMMUTABLE AS $$ SELECT 3 $$ + +statement ok +DROP TABLE t_replace_vol_virt + +# Stored computed column dependent. +statement ok +CREATE TABLE t_replace_vol_stored (a INT PRIMARY KEY, b INT AS (f_vol()) STORED) + +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_stored" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL VOLATILE AS $$ SELECT 2 $$ + +statement ok +DROP TABLE t_replace_vol_stored + +# CHECK constraint dependent. +statement ok +CREATE TABLE t_replace_vol_check (a INT PRIMARY KEY, b INT, CHECK (b > f_vol())) + +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_check" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL VOLATILE AS $$ SELECT 2 $$ + +statement ok +DROP TABLE t_replace_vol_check + +# Expression index dependent. +statement ok +CREATE TABLE t_replace_vol_idx (a INT PRIMARY KEY, b INT) + +statement ok +CREATE INDEX idx_replace_vol ON t_replace_vol_idx ((f_vol() + b)) + +statement error pgcode 2BP01 cannot change volatility of function "f_vol" because it is referenced by table "t_replace_vol_idx" +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL VOLATILE AS $$ SELECT 2 $$ + +statement ok +DROP TABLE t_replace_vol_idx + +# Without IMMUTABLE-requiring dependents, the volatility can be freely changed. +statement ok +CREATE OR REPLACE FUNCTION f_vol() RETURNS INT LANGUAGE SQL VOLATILE AS $$ SELECT 2 $$ + +statement ok +DROP FUNCTION f_vol diff --git a/pkg/sql/opt/optbuilder/select.go b/pkg/sql/opt/optbuilder/select.go index 7b1125de6523..8e75cc993175 100644 --- a/pkg/sql/opt/optbuilder/select.go +++ b/pkg/sql/opt/optbuilder/select.go @@ -826,7 +826,14 @@ func (b *Builder) buildScan( // virtual columns. proj := make(memo.ProjectionsExpr, 0, virtualColIDs.Len()) virtualColIDs.ForEach(func(col opt.ColumnID) { - item := b.factory.ConstructProjectionsItem(tabMeta.ComputedCols[col], col) + computedExpr := tabMeta.ComputedCols[col] + if computedExpr == nil { + panic(errors.WithHint( + errors.AssertionFailedf("virtual computed column has non-immutable expression"), + "a function referenced by this column may have been altered to non-IMMUTABLE volatility", + )) + } + item := b.factory.ConstructProjectionsItem(computedExpr, col) if !item.ScalarProps().OuterCols.SubsetOf(scanColIDs) { panic(errors.AssertionFailedf("scanned virtual column depends on non-scanned column")) } diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go index 2e2deec1534a..d8b7da7e9c35 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scerrors" "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb" + "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlclustersettings" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" @@ -386,6 +387,8 @@ func replaceFunction( } } + validateVolatilityForDependents(b, fnID, n.Name.Object(), volatility) + // Always replace all option sub-elements, even if using defaults. b.Replace(&scpb.FunctionVolatility{ FunctionID: fnID, @@ -438,6 +441,70 @@ func replaceFunction( // updateDependentTriggers finds all triggers that reference the given function // and updates their inlined function body and dependency tracking to reflect the // new function body. +// validateVolatilityForDependents rejects a volatility weakening when the +// function is referenced by an IMMUTABLE-requiring dependent (computed +// column, expression index, or CHECK constraint). +func validateVolatilityForDependents( + b BuildCtx, fnID catid.DescID, fnName string, newVolatility catpb.Function_Volatility, +) { + if newVolatility == catpb.Function_IMMUTABLE { + return + } + backRefs := b.BackReferences(fnID) + refsFunction := func(ids []catid.DescID) bool { + for _, id := range ids { + if id == fnID { + return true + } + } + return false + } + var depDescID catid.DescID + backRefs.FilterColumnComputeExpression().ForEach( + func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.ColumnComputeExpression) { + if depDescID == 0 && refsFunction(e.UsesFunctionIDs) { + depDescID = e.TableID + } + }, + ) + if depDescID == 0 { + backRefs.FilterCheckConstraint().ForEach( + func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.CheckConstraint) { + if depDescID == 0 && refsFunction(e.UsesFunctionIDs) { + depDescID = e.TableID + } + }, + ) + } + if depDescID == 0 { + backRefs.FilterCheckConstraintUnvalidated().ForEach( + func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.CheckConstraintUnvalidated) { + if depDescID == 0 && refsFunction(e.UsesFunctionIDs) { + depDescID = e.TableID + } + }, + ) + } + if depDescID == 0 { + backRefs.FilterSecondaryIndex().ForEach( + func(_ scpb.Status, _ scpb.TargetStatus, e *scpb.SecondaryIndex) { + if depDescID == 0 && e.EmbeddedExpr != nil && refsFunction(e.EmbeddedExpr.UsesFunctionIDs) { + depDescID = e.TableID + } + }, + ) + } + if depDescID == 0 { + return + } + ns := b.QueryByID(depDescID).FilterNamespace().MustGetOneElement() + panic(pgerror.Newf( + pgcode.DependentObjectsStillExist, + "cannot change volatility of function %q because it is referenced by table %q in a context requiring IMMUTABLE", + fnName, ns.Name, + )) +} + func updateDependentTriggers(b BuildCtx, fnID descpb.ID, fnBodyStr string) { backRefs := b.BackReferences(fnID) backRefs.FilterTriggerFunctionCall().ForEach( From e07872bac2ab4e262384342436950208c1110254 Mon Sep 17 00:00:00 2001 From: virajchogle Date: Wed, 10 Jun 2026 12:39:18 -0400 Subject: [PATCH 2/2] sql/schemachanger: attach updateDependentTriggers doc comment to its function Move the updateDependentTriggers doc comment so it sits adjacent to its function declaration. The validateVolatilityForDependents function was inserted between the comment and its function, causing godoc to attach the comment to the wrong declaration. Release note: None --- .../scbuild/internal/scbuildstmt/create_function.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go index d8b7da7e9c35..b9e457f50c76 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_function.go @@ -438,9 +438,6 @@ func replaceFunction( b.LogEventForExistingTarget(existingFnElem) } -// updateDependentTriggers finds all triggers that reference the given function -// and updates their inlined function body and dependency tracking to reflect the -// new function body. // validateVolatilityForDependents rejects a volatility weakening when the // function is referenced by an IMMUTABLE-requiring dependent (computed // column, expression index, or CHECK constraint). @@ -505,6 +502,9 @@ func validateVolatilityForDependents( )) } +// updateDependentTriggers finds all triggers that reference the given function +// and updates their inlined function body and dependency tracking to reflect the +// new function body. func updateDependentTriggers(b BuildCtx, fnID descpb.ID, fnBodyStr string) { backRefs := b.BackReferences(fnID) backRefs.FilterTriggerFunctionCall().ForEach(