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..b9e457f50c76 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, @@ -435,6 +438,70 @@ func replaceFunction( b.LogEventForExistingTarget(existingFnElem) } +// 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, + )) +} + // updateDependentTriggers finds all triggers that reference the given function // and updates their inlined function body and dependency tracking to reflect the // new function body.