Skip to content

Commit

Permalink
sql: disallow refcursor comparisons during type-checking
Browse files Browse the repository at this point in the history 
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 cockroachdb#112365
Fixes cockroachdb#112642
Fixes cockroachdb#112362
Fixes cockroachdb#112368

Release note: None
  • Loading branch information
@DrewKimball
DrewKimball committed Oct 19, 2023
1 parent 11cb0ae commit 28a523b
Show file tree
Hide file tree
Showing 4 changed files with 96 additions and 33 deletions.
6 changes: 5 additions & 1 deletion docs/generated/sql/operators.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@
<tr><td>oid <code><</code> <a href="int.html">int</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td>oid <code><</code> oid</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>pg_lsn <code><</code> pg_lsn</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code><</code> refcursor</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string</a> <code><</code> <a href="string.html">string</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string[]</a> <code><</code> <a href="string.html">string[]</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="time.html">time</a> <code><</code> <a href="time.html">time</a></td><td><a href="bool.html">bool</a></td></tr>
Expand Down Expand Up @@ -257,6 +258,7 @@
<tr><td>oid <code><=</code> <a href="int.html">int</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td>oid <code><=</code> oid</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>pg_lsn <code><=</code> pg_lsn</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code><=</code> refcursor</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string</a> <code><=</code> <a href="string.html">string</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string[]</a> <code><=</code> <a href="string.html">string[]</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="time.html">time</a> <code><=</code> <a href="time.html">time</a></td><td><a href="bool.html">bool</a></td></tr>
Expand Down Expand Up @@ -320,6 +322,7 @@
<tr><td>oid <code>=</code> <a href="int.html">int</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td>oid <code>=</code> oid</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>pg_lsn <code>=</code> pg_lsn</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code>=</code> refcursor</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string</a> <code>=</code> <a href="string.html">string</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string[]</a> <code>=</code> <a href="string.html">string[]</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="time.html">time</a> <code>=</code> <a href="time.html">time</a></td><td><a href="bool.html">bool</a></td></tr>
Expand Down Expand Up @@ -400,6 +403,7 @@
<tr><td>jsonb <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>oid <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>pg_lsn <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string</a> <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="time.html">time</a> <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="timestamp.html">timestamp</a> <code>IN</code> tuple</td><td><a href="bool.html">bool</a></td></tr>
Expand Down Expand Up @@ -447,7 +451,7 @@
<tr><td>oid <code>IS NOT DISTINCT FROM</code> <a href="int.html">int</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td>oid <code>IS NOT DISTINCT FROM</code> oid</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>pg_lsn <code>IS NOT DISTINCT FROM</code> pg_lsn</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code>IS NOT DISTINCT FROM</code> unknown</td><td><a href="bool.html">bool</a></td></tr>
<tr><td>refcursor <code>IS NOT DISTINCT FROM</code> refcursor</td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string</a> <code>IS NOT DISTINCT FROM</code> <a href="string.html">string</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="string.html">string[]</a> <code>IS NOT DISTINCT FROM</code> <a href="string.html">string[]</a></td><td><a href="bool.html">bool</a></td></tr>
<tr><td><a href="time.html">time</a> <code>IS NOT DISTINCT FROM</code> <a href="time.html">time</a></td><td><a href="bool.html">bool</a></td></tr>
Expand Down
51 changes: 23 additions & 28 deletions pkg/sql/logictest/testdata/logic_test/refcursor
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand Down
9 changes: 5 additions & 4 deletions pkg/sql/sem/tree/eval.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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),
Expand Down
63 changes: 63 additions & 0 deletions pkg/sql/sem/tree/type_check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

0 comments on commit 28a523b

Please sign in to comment.