Skip to content

Commit 55108a1

Browse files
committed
optbuilder: prohibit some operations on REFCURSOR
Postgres doesn't allow ordering by and equality comparison on refcursor columns, so this commit adds checks to match that behavior. Release note (bug fix): CockroachDB now prohibits ORDER BY and join equality operations on REFCURSOR type, matching behavior of Postgres.
1 parent e9631b4 commit 55108a1

File tree

6 files changed

+55
-10
lines changed

6 files changed

+55
-10
lines changed

pkg/ccl/logictestccl/testdata/logic_test/refcursor

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,12 @@ SELECT NULLIF('foo'::REFCURSOR, 'bar'::REFCURSOR);
470470
statement error pgcode 42883 pq: unsupported comparison operator
471471
SELECT NULLIF('foo'::REFCURSOR, NULL);
472472

473+
statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
474+
SELECT * FROM t ORDER BY x;
475+
476+
statement error pgcode 42883 pq: operator does not exist: refcursor = refcursor
477+
SELECT * FROM t INNER JOIN t t2 USING (x);
478+
473479
# Regression test for #112010 - REFCURSOR should allow IS NULL and
474480
# IS NOT NULL comparisons.
475481
subtest is_null
@@ -685,22 +691,35 @@ baz NULL baz foo
685691
statement error pgcode 42883 pq: unknown signature
686692
SELECT lag(y) OVER w, lead(y) OVER w, first_value(y) OVER w, last_value(y) OVER w FROM t WINDOW w AS ();
687693

688-
# Array functions are allowed.
694+
# Array functions are allowed (but ordering on REFCURSOR and REFCURSOR[] columns
695+
# is not).
689696
query TT
690-
SELECT array_agg(x ORDER BY x, y), array_cat_agg(y ORDER BY x, y) FROM t;
697+
SELECT array_agg(x), array_cat_agg(y) FROM t;
691698
----
692-
{baz,foo} {bar}
699+
{foo,baz} {bar}
693700

694-
query TT
695-
SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY x, y;
701+
query TT rowsort
702+
SELECT array_append(y, x), array_prepend(x, y) FROM t;
696703
----
697704
{baz} {baz}
698705
{bar,foo} {foo,bar}
699706

700-
query TT
701-
SELECT array_remove(y, 'baz'), array_replace(y, 'baz', 'foo') FROM t ORDER BY x, y;
707+
query TT rowsort
708+
SELECT array_remove(y, 'baz'), array_replace(y, 'baz', 'foo') FROM t;
702709
----
703710
{} {}
704711
{bar} {bar}
705712

713+
statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
714+
SELECT array_agg(x ORDER BY x) FROM t;
715+
716+
statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor\[\]
717+
SELECT array_agg(y ORDER BY y) FROM t;
718+
719+
statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor
720+
SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY x;
721+
722+
statement error pgcode 42883 pq: could not identify an ordering operator for type refcursor\[\]
723+
SELECT array_append(y, x), array_prepend(x, y) FROM t ORDER BY y;
724+
706725
subtest end

pkg/sql/opt/optbuilder/groupby.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -730,6 +730,7 @@ func (b *Builder) buildAggregateFunction(
730730
expr := tree.NewTypedIsNullExpr(e)
731731
b.buildAggArg(expr, &info, tempScope, fromScope)
732732
}
733+
ensureColumnOrderable(e)
733734
b.buildAggArg(e, &info, tempScope, fromScope)
734735
}
735736
}

pkg/sql/opt/optbuilder/join.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,11 @@ func (jb *usingJoinBuilder) addEqualityCondition(leftCol, rightCol *scopeColumn)
482482
"JOIN/USING types %s for left and %s for right cannot be matched for column %q",
483483
leftCol.typ, rightCol.typ, tree.ErrString(&name)))
484484
}
485+
} else if !tree.EqCmpAllowedForEquivalentTypes(leftCol.typ, rightCol.typ) {
486+
panic(pgerror.Newf(pgcode.UndefinedFunction,
487+
"operator does not exist: %s = %s",
488+
leftCol.typ.SQLStandardName(), rightCol.typ.SQLStandardName(),
489+
))
485490
}
486491

487492
// We will create a new "merged" column and hide the original columns.

pkg/sql/opt/optbuilder/orderby.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -325,14 +325,15 @@ func (b *Builder) hasDefaultNullsOrder(order *tree.Order) bool {
325325

326326
func ensureColumnOrderable(e tree.TypedExpr) {
327327
typ := e.ResolvedType()
328+
var arraySuffix string
328329
if typ.Family() == types.ArrayFamily {
329330
typ = typ.ArrayContents()
331+
arraySuffix = "[]"
330332
}
331333
switch typ.Family() {
332334
case types.TSQueryFamily, types.TSVectorFamily, types.PGVectorFamily:
333335
panic(unimplementedWithIssueDetailf(92165, "", "can't order by column type %s", typ.SQLString()))
334-
case types.JsonpathFamily:
335-
panic(pgerror.Newf(pgcode.UndefinedFunction, "could not identify an ordering operator for type jsonpath"))
336-
336+
case types.RefCursorFamily, types.JsonpathFamily:
337+
panic(pgerror.Newf(pgcode.UndefinedFunction, "could not identify an ordering operator for type %s%s", typ.SQLStandardName(), arraySuffix))
337338
}
338339
}

pkg/sql/sem/tree/eval.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp"
1818
"github.com/cockroachdb/cockroach/pkg/sql/sem/volatility"
1919
"github.com/cockroachdb/cockroach/pkg/sql/types"
20+
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
2021
"github.com/cockroachdb/cockroach/pkg/util/iterutil"
2122
"github.com/cockroachdb/cockroach/pkg/util/json"
2223
"github.com/cockroachdb/errors"
@@ -2170,3 +2171,19 @@ func EqualComparisonFunctionExists(leftType, rightType *types.T) bool {
21702171
_, found := CmpOps[treecmp.EQ].LookupImpl(leftType, rightType)
21712172
return found
21722173
}
2174+
2175+
// EqCmpAllowedForEquivalentTypes returns whether an equality comparison is
2176+
// allowed between two equivalent types.
2177+
func EqCmpAllowedForEquivalentTypes(leftType, rightType *types.T) bool {
2178+
if buildutil.CrdbTestBuild {
2179+
if !leftType.Equivalent(rightType) {
2180+
panic(errors.AssertionFailedf(
2181+
"expected equivalent types, found %s, %s",
2182+
leftType.SQLStringForError(), rightType.SQLStringForError(),
2183+
))
2184+
}
2185+
}
2186+
err := runValidations(treecmp.EQ, leftType, rightType,
2187+
[]types.Family{types.RefCursorFamily, types.JsonpathFamily})
2188+
return err == nil
2189+
}

pkg/sql/sem/tree/type_check.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3782,6 +3782,8 @@ var CannotAcceptTriggerErr = pgerror.New(pgcode.FeatureNotSupported,
37823782
// given family, which is invalid for comparison. We don't simply remove
37833783
// the relevant comparison overloads because we rely on their existence in
37843784
// various locations throughout the codebase.
3785+
// TODO(yuzefovich): audit callers of this method to see whether Jsonpath family
3786+
// should be handled in the same way as RefCursor family is.
37853787
func checkComparison(
37863788
op treecmp.ComparisonOperatorSymbol, left, right *types.T, family types.Family,
37873789
) error {

0 commit comments

Comments
 (0)