From 28a523b45203bb193ed2dec8b9c7cb1da47bdb08 Mon Sep 17 00:00:00 2001 From: Drew Kimball Date: Thu, 19 Oct 2023 15:03:35 -0600 Subject: [PATCH] sql: disallow refcursor comparisons during type-checking This patch adds back the comparison functions for the `REFCURSOR` data type, since there are various points within the codebase where we rely on their existence. Users are still not allowed to use the comparison functions, but now this is checked during type-checking of the AST, rather than at execution-time. This should avoid internal errors from places in planning and execution that expect comparison overloads to exist, but also maintains parity with postgres. Fixes #112365 Fixes #112642 Fixes #112362 Fixes #112368 Release note: None --- docs/generated/sql/operators.md | 6 +- .../logictest/testdata/logic_test/refcursor | 51 +++++++-------- pkg/sql/sem/tree/eval.go | 9 +-- pkg/sql/sem/tree/type_check.go | 63 +++++++++++++++++++ 4 files changed, 96 insertions(+), 33 deletions(-) diff --git a/docs/generated/sql/operators.md b/docs/generated/sql/operators.md index 9f01f3508286..b7c2af9b6893 100644 --- a/docs/generated/sql/operators.md +++ b/docs/generated/sql/operators.md @@ -193,6 +193,7 @@ oid < intbool oid < oidbool pg_lsn < pg_lsnbool +refcursor < refcursorbool string < stringbool string[] < string[]bool time < timebool @@ -257,6 +258,7 @@ oid <= intbool oid <= oidbool pg_lsn <= pg_lsnbool +refcursor <= refcursorbool string <= stringbool string[] <= string[]bool time <= timebool @@ -320,6 +322,7 @@ oid = intbool oid = oidbool pg_lsn = pg_lsnbool +refcursor = refcursorbool string = stringbool string[] = string[]bool time = timebool @@ -400,6 +403,7 @@ jsonb IN tuplebool oid IN tuplebool pg_lsn IN tuplebool +refcursor IN tuplebool string IN tuplebool time IN tuplebool timestamp IN tuplebool @@ -447,7 +451,7 @@ oid IS NOT DISTINCT FROM intbool oid IS NOT DISTINCT FROM oidbool pg_lsn IS NOT DISTINCT FROM pg_lsnbool -refcursor IS NOT DISTINCT FROM unknownbool +refcursor IS NOT DISTINCT FROM refcursorbool string IS NOT DISTINCT FROM stringbool string[] IS NOT DISTINCT FROM string[]bool time IS NOT DISTINCT FROM timebool diff --git a/pkg/sql/logictest/testdata/logic_test/refcursor b/pkg/sql/logictest/testdata/logic_test/refcursor index 3e1431529696..6f236c3eb864 100644 --- a/pkg/sql/logictest/testdata/logic_test/refcursor +++ b/pkg/sql/logictest/testdata/logic_test/refcursor @@ -394,59 +394,59 @@ NULL NULL NULL foo NULL NULL NULL NULL NULL NULL NULL NULL # REFCURSOR doesn't support any comparisons (with an exception mentioned below). subtest comparisons -# TODO(drewk): The error code should be 42883. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = 'foo'::REFCURSOR; +# TODO(drewk): The error code should be 42883. statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR = NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR < NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR > NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR <= NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR >= NULL; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM 'foo'::TEXT; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM 'foo'::REFCURSOR; statement error pgcode 22023 pq: unsupported comparison operator @@ -514,17 +514,17 @@ true true # Comparison with column is not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM l FROM implicit_types; -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM l FROM implicit_types; # Comparison with typed NULL is not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (NULL::REFCURSOR); -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (NULL::REFCURSOR); # Comparison with CASE expression is not allowed. @@ -534,23 +534,18 @@ SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); statement error pgcode 22023 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE 1 END); -# The CASE operator is folded into an untyped NULL, so it's allowed. -# TODO(drewk): Postgres doesn't allow this case. -query B +# The CASE operator is folded into an untyped NULL. +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); ----- -true -query B +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL END); ----- -false -# The CASE operator is folded into a typed NULL, so it's not allowed. -statement error pgcode 22023 pq: unsupported comparison operator +# The CASE operator is folded into a typed NULL. +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); -statement error pgcode 22023 pq: unsupported comparison operator +statement error pgcode 42883 pq: unsupported comparison operator SELECT 'foo'::REFCURSOR IS NOT DISTINCT FROM (CASE WHEN true THEN NULL ELSE NULL::REFCURSOR END); # Regression test for #112099 - REFCURSOR is not valid as an index column. diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index e9784271e94f..5146bf2d4d5e 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -1542,6 +1542,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeEqFn(types.Jsonb, types.Jsonb, volatility.Immutable), makeEqFn(types.Oid, types.Oid, volatility.Leakproof), makeEqFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeEqFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeEqFn(types.String, types.String, volatility.Leakproof), makeEqFn(types.Time, types.Time, volatility.Leakproof), makeEqFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1601,6 +1602,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeLtFn(types.Interval, types.Interval, volatility.Leakproof), makeLtFn(types.Oid, types.Oid, volatility.Leakproof), makeLtFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeLtFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeLtFn(types.String, types.String, volatility.Leakproof), makeLtFn(types.Time, types.Time, volatility.Leakproof), makeLtFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1659,6 +1661,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeLeFn(types.Interval, types.Interval, volatility.Leakproof), makeLeFn(types.Oid, types.Oid, volatility.Leakproof), makeLeFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeLeFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeLeFn(types.String, types.String, volatility.Leakproof), makeLeFn(types.Time, types.Time, volatility.Leakproof), makeLeFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1738,6 +1741,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Jsonb, types.Jsonb, volatility.Immutable), makeIsFn(types.Oid, types.Oid, volatility.Leakproof), makeIsFn(types.PGLSN, types.PGLSN, volatility.Leakproof), + makeIsFn(types.RefCursor, types.RefCursor, volatility.Leakproof), makeIsFn(types.String, types.String, volatility.Leakproof), makeIsFn(types.Time, types.Time, volatility.Leakproof), makeIsFn(types.TimeTZ, types.TimeTZ, volatility.Leakproof), @@ -1775,10 +1779,6 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeIsFn(types.Void, types.Unknown, volatility.Leakproof), makeIsFn(types.Unknown, types.Void, volatility.Leakproof), - // REFCURSOR cannot be compared with itself, but as a special case, it can - // be compared with NULL using IS NOT DISTINCT FROM. - makeIsFn(types.RefCursor, types.Unknown, volatility.Leakproof), - // Tuple comparison. { LeftType: types.AnyTuple, @@ -1809,6 +1809,7 @@ var CmpOps = cmpOpFixups(map[treecmp.ComparisonOperatorSymbol]*CmpOpOverloads{ makeEvalTupleIn(types.Jsonb, volatility.Leakproof), makeEvalTupleIn(types.Oid, volatility.Leakproof), makeEvalTupleIn(types.PGLSN, volatility.Leakproof), + makeEvalTupleIn(types.RefCursor, volatility.Leakproof), makeEvalTupleIn(types.String, volatility.Leakproof), makeEvalTupleIn(types.Time, volatility.Leakproof), makeEvalTupleIn(types.TimeTZ, volatility.Leakproof), diff --git a/pkg/sql/sem/tree/type_check.go b/pkg/sql/sem/tree/type_check.go index ca16911b703f..5f7e6bc77d02 100644 --- a/pkg/sql/sem/tree/type_check.go +++ b/pkg/sql/sem/tree/type_check.go @@ -928,6 +928,11 @@ func (expr *ComparisonExpr) TypeCheck( expr.Left, expr.Right, ) + if err == nil { + err = checkRefCursorComparison( + expr.SubOperator.Symbol, leftTyped.ResolvedType(), rightTyped.ResolvedType(), + ) + } } else { leftTyped, rightTyped, cmpOp, alwaysNull, err = typeCheckComparisonOp( ctx, @@ -936,6 +941,11 @@ func (expr *ComparisonExpr) TypeCheck( expr.Left, expr.Right, ) + if err == nil { + err = checkRefCursorComparison( + expr.Operator.Symbol, leftTyped.ResolvedType(), rightTyped.ResolvedType(), + ) + } } if err != nil { return nil, err @@ -1503,6 +1513,10 @@ func (expr *NullIfExpr) TypeCheck( leftType, op, rightType) return nil, decorateTypeCheckError(err, "incompatible NULLIF expressions") } + err = checkRefCursorComparison(treecmp.EQ, leftType, rightType) + if err != nil { + return nil, err + } expr.Expr1, expr.Expr2 = typedSubExprs[0], typedSubExprs[1] expr.typ = retType @@ -1584,10 +1598,20 @@ func (expr *RangeCond) TypeCheck( ctx context.Context, semaCtx *SemaContext, desired *types.T, ) (TypedExpr, error) { leftFromTyped, fromTyped, _, _, err := typeCheckComparisonOp(ctx, semaCtx, treecmp.MakeComparisonOperator(treecmp.GT), expr.Left, expr.From) + if err == nil { + err = checkRefCursorComparison( + treecmp.GT, leftFromTyped.ResolvedType(), fromTyped.ResolvedType(), + ) + } if err != nil { return nil, err } leftToTyped, toTyped, _, _, err := typeCheckComparisonOp(ctx, semaCtx, treecmp.MakeComparisonOperator(treecmp.LT), expr.Left, expr.To) + if err == nil { + err = checkRefCursorComparison( + treecmp.LT, leftToTyped.ResolvedType(), toTyped.ResolvedType(), + ) + } if err != nil { return nil, err } @@ -3446,3 +3470,42 @@ func CheckUnsupportedType(ctx context.Context, semaCtx *SemaContext, typ *types. } return semaCtx.UnsupportedTypeChecker.CheckType(ctx, typ) } + +// checkRefCursorComparison checks whether the given types are or contain the +// REFCURSOR data type, which is invalid for comparison. We don't simply remove +// the relevant comparison overloads because we rely on their existence in +// various locations throughout the codebase. +func checkRefCursorComparison(op treecmp.ComparisonOperatorSymbol, left, right *types.T) error { + if left.Family() == types.RefCursorFamily || right.Family() == types.RefCursorFamily { + if (op == treecmp.IsNotDistinctFrom || op == treecmp.IsDistinctFrom) && + (left == types.Unknown || right == types.Unknown) { + // Special case: "REFCURSOR IS [NOT] DISTINCT FROM NULL" is allowed. + return nil + } + return pgerror.Newf(pgcode.UndefinedFunction, + "unsupported comparison operator: %s %s %s", left, op, right, + ) + } + var checkRecursive func(*types.T) bool + checkRecursive = func(typ *types.T) bool { + switch typ.Family() { + case types.RefCursorFamily: + return true + case types.TupleFamily: + for _, t := range typ.TupleContents() { + if checkRecursive(t) { + return true + } + } + case types.ArrayFamily: + return checkRecursive(typ.ArrayContents()) + } + return false + } + if checkRecursive(left) || checkRecursive(right) { + return pgerror.Newf(pgcode.UndefinedFunction, + "could not identify a comparison function for type refcursor", + ) + } + return nil +}