From 1044a948e52a1bd059e2481414c6e8550d92d0d2 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Thu, 3 Aug 2023 13:58:50 -0400 Subject: [PATCH 1/5] sql/sem/tree: simplify SemaCtx reject flag checks Release note: None --- pkg/sql/sem/tree/type_check.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 0dc19c51df78..1dde544e5c92 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -606,7 +606,7 @@ func (expr *CastExpr) TypeCheck( castFrom := typedSubExpr.ResolvedType() allowStable := true context := "" - if semaCtx != nil && semaCtx.Properties.required.rejectFlags&RejectStableOperators != 0 { + if semaCtx != nil && semaCtx.Properties.IsSet(RejectStableOperators) { allowStable = false context = semaCtx.Properties.required.context } @@ -962,12 +962,12 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD return err } if expr.IsWindowFunctionApplication() { - if sc.Properties.required.rejectFlags&RejectWindowApplications != 0 { + if sc.Properties.IsSet(RejectWindowApplications) { return NewInvalidFunctionUsageError(WindowClass, sc.Properties.required.context) } if sc.Properties.Derived.InWindowFunc && - sc.Properties.required.rejectFlags&RejectNestedWindowFunctions != 0 { + sc.Properties.IsSet(RejectNestedWindowFunctions) { return pgerror.Newf(pgcode.Windowing, "window function calls cannot be nested") } sc.Properties.Derived.SeenWindowApplication = true @@ -976,10 +976,10 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD // we have an aggregation. if fnCls == AggregateClass { if sc.Properties.Derived.inFuncExpr && - sc.Properties.required.rejectFlags&RejectNestedAggregates != 0 { + sc.Properties.IsSet(RejectNestedAggregates) { return NewAggInAggError() } - if sc.Properties.required.rejectFlags&RejectAggregates != 0 { + if sc.Properties.IsSet(RejectAggregates) { return NewInvalidFunctionUsageError(AggregateClass, sc.Properties.required.context) } sc.Properties.Derived.SeenAggregate = true @@ -987,10 +987,10 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD } if fnCls == GeneratorClass { if sc.Properties.Derived.inFuncExpr && - sc.Properties.required.rejectFlags&RejectNestedGenerators != 0 { + sc.Properties.IsSet(RejectNestedGenerators) { return NewInvalidNestedSRFError(sc.Properties.required.context) } - if sc.Properties.required.rejectFlags&RejectGenerators != 0 { + if sc.Properties.IsSet(RejectGenerators) { return NewInvalidFunctionUsageError(GeneratorClass, sc.Properties.required.context) } sc.Properties.Derived.SeenGenerator = true @@ -1023,7 +1023,7 @@ func (sc *SemaContext) checkVolatility(v volatility.V) error { } switch v { case volatility.Volatile: - if sc.Properties.required.rejectFlags&RejectVolatileFunctions != 0 { + if sc.Properties.IsSet(RejectVolatileFunctions) { // The code FeatureNotSupported is a bit misleading here, // because we probably can't support the feature at all. However // this error code matches PostgreSQL's in the same conditions. @@ -1031,7 +1031,7 @@ func (sc *SemaContext) checkVolatility(v volatility.V) error { "volatile functions are not allowed in %s", sc.Properties.required.context) } case volatility.Stable: - if sc.Properties.required.rejectFlags&RejectStableOperators != 0 { + if sc.Properties.IsSet(RejectStableOperators) { return NewContextDependentOpsNotAllowedError(sc.Properties.required.context) } } @@ -1529,7 +1529,7 @@ func (expr *RangeCond) TypeCheck( // TypeCheck implements the Expr interface. func (expr *Subquery) TypeCheck(_ context.Context, sc *SemaContext, _ *types.T) (TypedExpr, error) { - if sc != nil && sc.Properties.required.rejectFlags&RejectSubqueries != 0 { + if sc != nil && sc.Properties.IsSet(RejectSubqueries) { return nil, pgerror.Newf(pgcode.FeatureNotSupported, "subqueries are not allowed in %s", sc.Properties.required.context) } From b61e511413226dda43330723e1c4b286196e3c4a Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 4 Aug 2023 12:01:33 -0400 Subject: [PATCH 2/5] sql/sem/tree: split derived SemaContext properties from contextual info Properties derived about expressions during semantic analysis are communicated to callers via ScalarProperties. Prior to this commit, this type was also used to provide contextual information while traversing sub-expressions during semantic analysis. For example, it would indicate whether the current expression is a descendent of a window function expression. These two types of information, derived and contextual, are fundamentally different. Derived properties bubble up from the bottom of the tree to the top, while context propagates downward into sub-expressions. This difference made it difficult to maintaining them correctly in a single type and difficult to reason about. This commit introduces the ScalarScene type which is used for providing internal contextual information during semantic analysis. Release note: None --- pkg/sql/opt/optbuilder/scope.go | 13 +--- pkg/sql/opt/optbuilder/testdata/aggregate | 2 +- pkg/sql/sem/tree/type_check.go | 92 +++++++++++++++-------- 3 files changed, 65 insertions(+), 42 deletions(-) diff --git a/pkg/sql/opt/optbuilder/scope.go b/pkg/sql/opt/optbuilder/scope.go index ad44fab80ab3..d1ff62b2ae71 100644 --- a/pkg/sql/opt/optbuilder/scope.go +++ b/pkg/sql/opt/optbuilder/scope.go @@ -1358,15 +1358,10 @@ func (s *scope) replaceWindowFn(f *tree.FuncExpr, def *tree.ResolvedFunctionDefi // We will be performing type checking on expressions from PARTITION BY and // ORDER BY clauses below, and we need the semantic context to know that we // are in a window function. InWindowFunc is updated when type checking - // FuncExpr above, but it is reset upon returning from that, so we need to do - // this update manually. - defer func(ctx *tree.SemaContext, prevWindow bool) { - ctx.Properties.Derived.InWindowFunc = prevWindow - }( - s.builder.semaCtx, - s.builder.semaCtx.Properties.Derived.InWindowFunc, - ) - s.builder.semaCtx.Properties.Derived.InWindowFunc = true + // FuncExpr above, but it is reset upon returning from that, so we need to + // do this update manually. + defer s.builder.semaCtx.Properties.Ancestors.PopTo(s.builder.semaCtx.Properties.Ancestors) + s.builder.semaCtx.Properties.Ancestors.Push(tree.WindowFuncAncestor) oldPartitions := f.WindowDef.Partitions f.WindowDef.Partitions = make(tree.Exprs, len(oldPartitions)) diff --git a/pkg/sql/opt/optbuilder/testdata/aggregate b/pkg/sql/opt/optbuilder/testdata/aggregate index f3ea910ebd01..7f2e2cc0a09b 100644 --- a/pkg/sql/opt/optbuilder/testdata/aggregate +++ b/pkg/sql/opt/optbuilder/testdata/aggregate @@ -2616,7 +2616,7 @@ sort # Grouping columns cannot be reused inside an aggregate input expression # because the aggregate input expressions and grouping expressions are -# built as part of the same projection. +# built as part of the same projection. build SELECT max((k+v)/(k-v)) AS r, (k+v)*(k-v) AS s FROM kv GROUP BY k+v, k-v ---- diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 1dde544e5c92..25c544aa9ef2 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -82,17 +82,21 @@ type SemaContext struct { // Restore() method, see below. type SemaProperties struct { // required constraints type checking to only accept certain kinds - // of expressions. See SetConstraint + // of expressions. See Require. required semaRequirements // Derived is populated during semantic analysis with properties // from the expression being analyzed. The caller is responsible // for re-initializing this when needed. Derived ScalarProperties + + // Ancestors is mutated during semantic analysis to provide contextual + // information for each descendent during traversal of sub-expressions. + Ancestors ScalarAncestors } type semaRequirements struct { - // context is the name of the semantic anlysis context, for use in + // context is the name of the semantic analysis context, for use in // error messages. context string @@ -102,11 +106,14 @@ type semaRequirements struct { rejectFlags SemaRejectFlags } -// Require resets the derived properties and sets required constraints. +// Require resets the derived properties and the scalar ancestors, and sets +// required constraints. It must only be called before starting semantic +// analysis and during traversal by semantic analysis itself. func (s *SemaProperties) Require(context string, rejectFlags SemaRejectFlags) { s.required.context = context s.required.rejectFlags = rejectFlags s.Derived.Clear() + s.Ancestors.clear() } // IsSet checks if the given rejectFlag is set as a required property. @@ -180,16 +187,6 @@ type ScalarProperties struct { // SeenGenerator is set to true if the expression originally // contained a SRF. SeenGenerator bool - - // inFuncExpr is temporarily set to true while type checking the - // parameters of a function. Used to process RejectNestedGenerators - // properly. - inFuncExpr bool - - // InWindowFunc is temporarily set to true while type checking the - // parameters of a window function in order to reject nested window - // functions. - InWindowFunc bool } // Clear resets the scalar properties to defaults. @@ -197,6 +194,45 @@ func (sp *ScalarProperties) Clear() { *sp = ScalarProperties{} } +// ScalarAncestors provides context for the current scalar expression during +// semantic analysis. Ancestors are temporarily modified by expressions so that +// their descendent expressions can be analyzed with respect to their ancestors. +type ScalarAncestors byte + +const ( + // FuncExprAncestor is temporarily added to ScalarAncestors while type + // checking the parameters of a function. Used to process + // RejectNestedGenerators properly. + FuncExprAncestor ScalarAncestors = 1 << iota + + // WindowFuncAncestor is temporarily added to ScalarAncestors while type + // checking the parameters of a window function in order to reject nested + // window functions. + WindowFuncAncestor +) + +// Push adds the given ancestor to s. +func (s *ScalarAncestors) Push(other ScalarAncestors) { + *s = *s | other +} + +// Has returns true if s has the given ancestor. +func (s ScalarAncestors) Has(other ScalarAncestors) bool { + return s&other != 0 +} + +// PopTo returns s to the given set of ancestors. Use with: +// +// defer semaCtx.Properties.Ancestors.PopTo(semaCtx.Properties.Ancestors) +func (s *ScalarAncestors) PopTo(orig ScalarAncestors) { + *s = orig +} + +// clear resets s to the default set of ancestors. +func (s *ScalarAncestors) clear() { + *s = 0 +} + // MakeSemaContext initializes a simple SemaContext suitable // for "lightweight" type checking such as the one performed for default // expressions. @@ -966,7 +1002,7 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD return NewInvalidFunctionUsageError(WindowClass, sc.Properties.required.context) } - if sc.Properties.Derived.InWindowFunc && + if sc.Properties.Ancestors.Has(WindowFuncAncestor) && sc.Properties.IsSet(RejectNestedWindowFunctions) { return pgerror.Newf(pgcode.Windowing, "window function calls cannot be nested") } @@ -975,7 +1011,7 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD // If it is an aggregate function *not used OVER a window*, then // we have an aggregation. if fnCls == AggregateClass { - if sc.Properties.Derived.inFuncExpr && + if sc.Properties.Ancestors.Has(FuncExprAncestor) && sc.Properties.IsSet(RejectNestedAggregates) { return NewAggInAggError() } @@ -986,7 +1022,7 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD } } if fnCls == GeneratorClass { - if sc.Properties.Derived.inFuncExpr && + if sc.Properties.Ancestors.Has(FuncExprAncestor) && sc.Properties.IsSet(RejectNestedGenerators) { return NewInvalidNestedSRFError(sc.Properties.required.context) } @@ -1080,23 +1116,15 @@ func (expr *FuncExpr) TypeCheck( } if semaCtx != nil { - // We'll need to remember we are in a function application to - // generate suitable errors in checkFunctionUsage(). We cannot - // set ctx.inFuncExpr earlier (in particular not before the call - // to checkFunctionUsage() above) because the top-level FuncExpr - // must be acceptable even if it is a SRF and - // RejectNestedGenerators is set. - defer func(semaCtx *SemaContext, prevFunc bool, prevWindow bool) { - semaCtx.Properties.Derived.inFuncExpr = prevFunc - semaCtx.Properties.Derived.InWindowFunc = prevWindow - }( - semaCtx, - semaCtx.Properties.Derived.inFuncExpr, - semaCtx.Properties.Derived.InWindowFunc, - ) - semaCtx.Properties.Derived.inFuncExpr = true + // We'll need to remember we are in a function application to generate + // suitable errors in checkFunctionUsage(). We cannot enter + // FuncExprAncestor earlier (in particular not before the call to + // checkFunctionUsage() above) because the top-level FuncExpr must be + // acceptable even if it is a SRF and RejectNestedGenerators is set. + defer semaCtx.Properties.Ancestors.PopTo(semaCtx.Properties.Ancestors) + semaCtx.Properties.Ancestors.Push(FuncExprAncestor) if expr.WindowDef != nil { - semaCtx.Properties.Derived.InWindowFunc = true + semaCtx.Properties.Ancestors.Push(WindowFuncAncestor) } } From 82109dfac23c2e1cd2aefec024ba8559e30c109f Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 4 Aug 2023 12:39:24 -0400 Subject: [PATCH 3/5] sql/sem/tree: do not Restore SemaRejectFlags during semantic analysis This commit fixes a bug introduced in #105582 that caused SemaRejectFlags to be restored during semantic analysis, preventing the analysis from detecting some forms of invalid expressions. Fixes #108166 There is no release note because the related bug does not exist in any releases. Release note: None --- pkg/sql/logictest/testdata/logic_test/delete | 12 ++++++++ pkg/sql/logictest/testdata/logic_test/srfs | 12 ++++---- pkg/sql/logictest/testdata/logic_test/update | 12 ++++++++ pkg/sql/opt/optbuilder/srfs.go | 3 ++ pkg/sql/sem/tree/type_check.go | 29 ++++++++++---------- 5 files changed, 47 insertions(+), 21 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/delete b/pkg/sql/logictest/testdata/logic_test/delete index 66f63b3a6e69..5fb8fa05bb60 100644 --- a/pkg/sql/logictest/testdata/logic_test/delete +++ b/pkg/sql/logictest/testdata/logic_test/delete @@ -576,3 +576,15 @@ statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORD DELETE FROM t107634 ORDER BY sum(a) LIMIT 1; subtest end + +# Regression test for #108166. Do not allow aggregate functions in ORDER BY when +# the function is wrapped by a conditional expression. +subtest regression_108166 + +statement ok +CREATE TABLE t108166 (a INT) + +statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORDER BY in DELETE +DELETE FROM t108166 ORDER BY COALESCE(sum(a), 1) LIMIT 1; + +subtest end diff --git a/pkg/sql/logictest/testdata/logic_test/srfs b/pkg/sql/logictest/testdata/logic_test/srfs index 824c03836bde..19244444b8d4 100644 --- a/pkg/sql/logictest/testdata/logic_test/srfs +++ b/pkg/sql/logictest/testdata/logic_test/srfs @@ -1322,16 +1322,16 @@ subtest generator-syntax # Regression test for #97119 and #94890 - return syntax error when CASE or # COALESCE is used with a set-generating function as argument. -statement error pq: set-returning functions are not allowed in CASE +statement error pq: set-returning functions are not allowed in conditional expressions SELECT CASE generate_series(1, 3) WHEN 3 THEN 0 ELSE 1 END; -statement error pq: set-returning functions are not allowed in CASE +statement error pq: set-returning functions are not allowed in conditional expressions SELECT CASE WHEN true THEN generate_series(1, 3) ELSE 1 END; -statement error pq: set-returning functions are not allowed in CASE +statement error pq: set-returning functions are not allowed in conditional expressions SELECT CASE WHEN false THEN 1 ELSE generate_series(1, 3) END; -statement error pq: set-returning functions are not allowed in COALESCE +statement error pq: set-returning functions are not allowed in conditional expressions SELECT COALESCE(generate_series(1, 10)); # A subquery with a generator function is allowed within CASE and COALESCE. @@ -1376,12 +1376,12 @@ SELECT COALESCE(sum(x) OVER ()) FROM xy; 15 # IF does not allow generator functions. -statement error pq: set-returning functions are not allowed in IF +statement error pq: set-returning functions are not allowed in conditional expressions SELECT IF(x > y, generate_series(1, 3), 0) FROM xy; # IFNULL does not allow generator functions. Note that the error mentions # COALESCE because IFNULL is parsed directly as a COALESCE expression. -statement error pq: set-returning functions are not allowed in COALESCE +statement error pq: set-returning functions are not allowed in conditional expressions SELECT IFNULL(1, generate_series(1, 2)); # NULLIF allows generator functions. diff --git a/pkg/sql/logictest/testdata/logic_test/update b/pkg/sql/logictest/testdata/logic_test/update index 6feaa006b02c..f9757512505b 100644 --- a/pkg/sql/logictest/testdata/logic_test/update +++ b/pkg/sql/logictest/testdata/logic_test/update @@ -672,3 +672,15 @@ statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORD UPDATE t107634 SET a = 1 ORDER BY sum(a) LIMIT 1; subtest end + +# Regression test for #108166. Do not allow aggregate functions in ORDER BY when +# the function is wrapped by a conditional expression. +subtest regression_108166 + +statement ok +CREATE TABLE t108166 (a INT) + +statement error pgcode 42803 sum\(\): aggregate functions are not allowed in ORDER BY in UPDATE +UPDATE t108166 SET a = 1 ORDER BY COALESCE(sum(a), 1) LIMIT 1; + +subtest end diff --git a/pkg/sql/opt/optbuilder/srfs.go b/pkg/sql/opt/optbuilder/srfs.go index 471191265e85..76965329580a 100644 --- a/pkg/sql/opt/optbuilder/srfs.go +++ b/pkg/sql/opt/optbuilder/srfs.go @@ -50,6 +50,9 @@ func (s *srf) TypeCheck( // invalid usage here. return nil, tree.NewInvalidFunctionUsageError(tree.GeneratorClass, ctx.TypeCheckContext()) } + if ctx.Properties.Ancestors.Has(tree.ConditionalAncestor) { + return nil, tree.NewInvalidFunctionUsageError(tree.GeneratorClass, "conditional expressions") + } if ctx.Properties.Derived.SeenGenerator { // This error happens if this srf struct is nested inside a raw srf that // has not yet been replaced. This is possible since scope.replaceSRF first diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index 25c544aa9ef2..1eedea54d05c 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -209,6 +209,11 @@ const ( // checking the parameters of a window function in order to reject nested // window functions. WindowFuncAncestor + + // ConditionalAncestor is temporarily added to ScalarAncestors while type + // checking condition expressions CASE, COALESCE, and IF. Used to reject + // set-returning functions within conditional expressions. + ConditionalAncestor ) // Push adds the given ancestor to s. @@ -424,11 +429,8 @@ func (expr *CaseExpr) TypeCheck( ctx context.Context, semaCtx *SemaContext, desired *types.T, ) (TypedExpr, error) { if semaCtx != nil { - // We need to save and restore the previous value of the field in - // semaCtx in case we are recursively called within a subquery - // context. - defer semaCtx.Properties.Restore(semaCtx.Properties) - semaCtx.Properties.Require("CASE", RejectGenerators) + defer semaCtx.Properties.Ancestors.PopTo(semaCtx.Properties.Ancestors) + semaCtx.Properties.Ancestors.Push(ConditionalAncestor) } var err error tmpExprs := make([]Expr, 0, len(expr.Whens)+1) @@ -877,11 +879,8 @@ func (expr *CoalesceExpr) TypeCheck( ctx context.Context, semaCtx *SemaContext, desired *types.T, ) (TypedExpr, error) { if semaCtx != nil { - // We need to save and restore the previous value of the field in - // semaCtx in case we are recursively called within a subquery - // context. - defer semaCtx.Properties.Restore(semaCtx.Properties) - semaCtx.Properties.Require("COALESCE", RejectGenerators) + defer semaCtx.Properties.Ancestors.PopTo(semaCtx.Properties.Ancestors) + semaCtx.Properties.Ancestors.Push(ConditionalAncestor) } typedSubExprs, retType, err := typeCheckSameTypedExprs(ctx, semaCtx, desired, expr.Exprs...) if err != nil { @@ -1029,6 +1028,9 @@ func (sc *SemaContext) checkFunctionUsage(expr *FuncExpr, def *ResolvedFunctionD if sc.Properties.IsSet(RejectGenerators) { return NewInvalidFunctionUsageError(GeneratorClass, sc.Properties.required.context) } + if sc.Properties.Ancestors.Has(ConditionalAncestor) { + return NewInvalidFunctionUsageError(GeneratorClass, "conditional expressions") + } sc.Properties.Derived.SeenGenerator = true } return nil @@ -1355,11 +1357,8 @@ func (expr *IfExpr) TypeCheck( ctx context.Context, semaCtx *SemaContext, desired *types.T, ) (TypedExpr, error) { if semaCtx != nil { - // We need to save and restore the previous value of the field in - // semaCtx in case we are recursively called within a subquery - // context. - defer semaCtx.Properties.Restore(semaCtx.Properties) - semaCtx.Properties.Require("IF", RejectGenerators) + defer semaCtx.Properties.Ancestors.PopTo(semaCtx.Properties.Ancestors) + semaCtx.Properties.Ancestors.Push(ConditionalAncestor) } typedCond, err := typeCheckAndRequireBoolean(ctx, semaCtx, expr.Cond, "IF condition") if err != nil { From 9fd51a045a68efbf324216e6cbf057965034dbb2 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 4 Aug 2023 12:58:19 -0400 Subject: [PATCH 4/5] sql: do not allow subqueries to be cast to enums in views and UDFs This commit is a follow-up to #106868 after additional reproductions of the original bug were found. For now, we disallow any CAST expressions that contain a subquery in the input and the target type is an ENUM. I've created #108184 to track this limitation. Fixes #107654 There is no release note because the release note from #106868 should be sufficient. Release note: None --- pkg/sql/create_function.go | 2 +- pkg/sql/create_view.go | 30 ++++++++----------- .../testdata/logic_test/udf_regressions | 24 +++++++-------- .../testdata/logic_test/udf_unsupported | 1 - pkg/sql/logictest/testdata/logic_test/views | 26 +++++++--------- .../schemachanger/scbuild/builder_state.go | 21 +++++-------- 6 files changed, 41 insertions(+), 63 deletions(-) diff --git a/pkg/sql/create_function.go b/pkg/sql/create_function.go index f5de663ce0b9..53e0eca05fdf 100644 --- a/pkg/sql/create_function.go +++ b/pkg/sql/create_function.go @@ -443,7 +443,7 @@ func setFuncOptions( return err } typeReplacedFuncBody, err := serializeUserDefinedTypes( - params.ctx, params.p.SemaCtx(), seqReplacedFuncBody, true, /* multiStmt */ + params.ctx, params.p.SemaCtx(), seqReplacedFuncBody, true /* multiStmt */, "UDFs", ) if err != nil { return err diff --git a/pkg/sql/create_view.go b/pkg/sql/create_view.go index ebddc530d523..888939e475b5 100644 --- a/pkg/sql/create_view.go +++ b/pkg/sql/create_view.go @@ -411,7 +411,8 @@ func makeViewTableDesc( desc.ViewQuery = sequenceReplacedQuery } - typeReplacedQuery, err := serializeUserDefinedTypes(ctx, semaCtx, desc.ViewQuery, false /* multiStmt */) + typeReplacedQuery, err := serializeUserDefinedTypes(ctx, semaCtx, desc.ViewQuery, + false /* multiStmt */, "view queries") if err != nil { return tabledesc.Mutable{}, err } @@ -490,7 +491,7 @@ func replaceSeqNamesWithIDs( // and serialize any user defined types, so that renaming the type // does not corrupt the view. func serializeUserDefinedTypes( - ctx context.Context, semaCtx *tree.SemaContext, queries string, multiStmt bool, + ctx context.Context, semaCtx *tree.SemaContext, queries string, multiStmt bool, parentType string, ) (string, error) { replaceFunc := func(expr tree.Expr) (recurse bool, newExpr tree.Expr, err error) { var innerExpr tree.Expr @@ -505,20 +506,6 @@ func serializeUserDefinedTypes( default: return true, expr, nil } - // We cannot type-check subqueries without using optbuilder, and there - // is no need to because we only need to rewrite string values that are - // directly cast to enums. For example, we must rewrite the 'foo' in: - // - // SELECT 'foo'::myenum - // - // We don't need to rewrite the 'foo' in the query below, which can be - // corrupted by renaming the 'foo' value in the myenum type. - // - // SELECT (SELECT 'foo')::myenum - // - if _, ok := innerExpr.(*tree.Subquery); ok { - return true, expr, nil - } // semaCtx may be nil if this is a virtual view being created at // init time. var typeResolver tree.TypeReferenceResolver @@ -533,6 +520,14 @@ func serializeUserDefinedTypes( if !typ.UserDefined() { return true, expr, nil } + { + // We cannot type-check subqueries without using optbuilder, so we + // currently do not support casting expressions with subqueries to + // UDTs. + context := "casts to enums within " + parentType + defer semaCtx.Properties.Restore(semaCtx.Properties) + semaCtx.Properties.Require(context, tree.RejectSubqueries) + } texpr, err := innerExpr.TypeCheck(ctx, semaCtx, typ) if err != nil { return false, expr, err @@ -603,7 +598,8 @@ func (p *planner) replaceViewDesc( toReplace.ViewQuery = updatedQuery } - typeReplacedQuery, err := serializeUserDefinedTypes(ctx, p.SemaCtx(), toReplace.ViewQuery, false /* multiStmt */) + typeReplacedQuery, err := serializeUserDefinedTypes(ctx, p.SemaCtx(), toReplace.ViewQuery, + false /* multiStmt */, "view queries") if err != nil { return nil, err } diff --git a/pkg/sql/logictest/testdata/logic_test/udf_regressions b/pkg/sql/logictest/testdata/logic_test/udf_regressions index 25bfcb0adc67..15a94ee19967 100644 --- a/pkg/sql/logictest/testdata/logic_test/udf_regressions +++ b/pkg/sql/logictest/testdata/logic_test/udf_regressions @@ -554,29 +554,25 @@ $$ subtest end -# Regression test for #105259. Do not type-check subqueries in UDFs outside -# optbuilder. Doing so can cause internal errors. +# Regression tests for #105259 and #107654. Do not type-check subqueries in UDFs +# outside optbuilder. Doing so can cause internal errors. subtest regression_105259 statement ok CREATE TYPE e105259 AS ENUM ('foo'); -statement ok +statement error pgcode 0A000 subqueries are not allowed in casts to enums within UDFs CREATE FUNCTION f() RETURNS VOID LANGUAGE SQL AS $$ SELECT (SELECT 'foo')::e105259; SELECT NULL; $$ -query T -SELECT f() ----- -NULL - -statement ok -ALTER TYPE e105259 RENAME VALUE 'foo' TO 'bar' - -# Renaming the enum value corrupts the UDF. This is expected behavior. -statement error pgcode 22P02 invalid input value for enum e105259: "foo" -SELECT f() +statement error pgcode 0A000 subqueries are not allowed in casts to enums within UDFs +CREATE FUNCTION f() RETURNS VOID LANGUAGE SQL AS $$ + SELECT ( + CASE WHEN true THEN (SELECT 'foo') ELSE NULL END + )::e105259; + SELECT NULL; +$$ subtest end diff --git a/pkg/sql/logictest/testdata/logic_test/udf_unsupported b/pkg/sql/logictest/testdata/logic_test/udf_unsupported index 5a74f495bd96..ca200d987c74 100644 --- a/pkg/sql/logictest/testdata/logic_test/udf_unsupported +++ b/pkg/sql/logictest/testdata/logic_test/udf_unsupported @@ -183,4 +183,3 @@ statement error pgcode 0A000 unimplemented: cannot create UDFs under a temporary CREATE FUNCTION $temp_schema_102964.f_102964 () RETURNS INT AS 'SELECT 1' LANGUAGE sql; subtest end - diff --git a/pkg/sql/logictest/testdata/logic_test/views b/pkg/sql/logictest/testdata/logic_test/views index 9fcdca2bfacb..cbbfb5fc054c 100644 --- a/pkg/sql/logictest/testdata/logic_test/views +++ b/pkg/sql/logictest/testdata/logic_test/views @@ -1900,28 +1900,22 @@ SELECT * FROM v104927 subtest end -# Regression test for #105259. Do not type-check subqueries in views outside -# optbuilder. Doing so can cause internal errors. -subtest regression_105259 +# Regression tests for #105259 and #107654. Do not type-check subqueries in +# views outside optbuilder. Doing so can cause internal errors. +subtest regression_105259_107654 statement ok CREATE TYPE e105259 AS ENUM ('foo'); -statement ok -CREATE VIEW v105259 AS +statement error pgcode 0A000 subqueries are not allowed in casts to enums within view queries +CREATE VIEW v AS SELECT (SELECT 'foo')::e105259 -query T -SELECT * FROM v105259 ----- -foo - -statement ok -ALTER TYPE e105259 RENAME VALUE 'foo' TO 'bar' - -# Renaming the enum value corrupts the view. This is expected behavior. -statement error pgcode 22P02 invalid input value for enum e105259: "foo" -SELECT * FROM v105259 +statement error pgcode 0A000 subqueries are not allowed in casts to enums within view queries +CREATE VIEW v AS +SELECT ( + CASE WHEN true THEN (SELECT 'foo') ELSE NULL END +)::e105259 subtest end diff --git a/pkg/sql/schemachanger/scbuild/builder_state.go b/pkg/sql/schemachanger/scbuild/builder_state.go index 18d9a5273a70..6ba485d46ca0 100644 --- a/pkg/sql/schemachanger/scbuild/builder_state.go +++ b/pkg/sql/schemachanger/scbuild/builder_state.go @@ -1591,20 +1591,6 @@ func (b *builderState) serializeUserDefinedTypes(queryStr string) string { default: return true, expr, nil } - // We cannot type-check subqueries without using optbuilder, and there - // is no need to because we only need to rewrite string values that are - // directly cast to enums. For example, we must rewrite the 'foo' in: - // - // SELECT 'foo'::myenum - // - // We don't need to rewrite the 'foo' in the query below, which can be - // corrupted by renaming the 'foo' value in the myenum type. - // - // SELECT (SELECT 'foo')::myenum - // - if _, ok := innerExpr.(*tree.Subquery); ok { - return true, expr, nil - } var typ *types.T typ, err = tree.ResolveType(b.ctx, typRef, b.semaCtx.TypeResolver) if err != nil { @@ -1613,6 +1599,13 @@ func (b *builderState) serializeUserDefinedTypes(queryStr string) string { if !typ.UserDefined() { return true, expr, nil } + { + // We cannot type-check subqueries without using optbuilder, so we + // currently do not support casting expressions with subqueries to + // UDTs. + defer b.semaCtx.Properties.Restore(b.semaCtx.Properties) + b.semaCtx.Properties.Require("casts to enums within UDFs", tree.RejectSubqueries) + } texpr, err := innerExpr.TypeCheck(b.ctx, b.semaCtx, typ) if err != nil { return false, expr, err From 25c344c54bd4cf44ce80cea1c58b0924a4509a20 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Fri, 4 Aug 2023 13:06:51 -0400 Subject: [PATCH 5/5] sql/randgen: fix typo in comment Release note: None --- pkg/sql/randgen/expr.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/sql/randgen/expr.go b/pkg/sql/randgen/expr.go index 86403cc592cd..412710e55653 100644 --- a/pkg/sql/randgen/expr.go +++ b/pkg/sql/randgen/expr.go @@ -161,8 +161,8 @@ func randExpr( } } if len(cols) > 1 { - // If any of the columns are nullable, set the computed column to be - // nullable. + // If any of the columns are nullable, the resulting expression + // could be null. for _, x := range cols { if x.Nullable.Nullability != tree.NotNull { nullability = x.Nullable.Nullability