Skip to content

Commit 8e2e0f7

Browse files
tglsfdctglsfdc
committed
Fix failure to validate the result of select_common_type().
Although select_common_type() has a failure-return convention, an apparent successful return just provides a type OID that *might* work as a common supertype; we've not validated that the required casts actually exist. In the mainstream use-cases that doesn't matter, because we'll proceed to invoke coerce_to_common_type() on each input, which will fail appropriately if the proposed common type doesn't actually work. However, a few callers didn't read the (nonexistent) fine print, and thought that if they got back a nonzero OID then the coercions were sure to work. This affects in particular the recently-added "anycompatible" polymorphic types; we might think that a function/operator using such types matches cases it really doesn't. A likely end result of that is unexpected "ambiguous operator" errors, as for example in bug #17387 from James Inform. Another, much older, case is that the parser might try to transform an "x IN (list)" construct to a ScalarArrayOpExpr even when the list elements don't actually have a common supertype. It doesn't seem desirable to add more checking to select_common_type itself, as that'd just slow down the mainstream use-cases. Instead, write a separate function verify_common_type that performs the missing checks, and add a call to that where necessary. Likewise add verify_common_type_from_oids to go with select_common_type_from_oids. Back-patch to v13 where the "anycompatible" types came in. (The symptom complained of in bug #17387 doesn't appear till v14, but that's just because we didn't get around to converting || to use anycompatible till then.) In principle the "x IN (list)" fix could go back all the way, but I'm not currently convinced that it makes much difference in real-world cases, so I won't bother for now. Discussion: https://postgr.es/m/17387-5dfe54b988444963@postgresql.org
1 parent 5ecd018 commit 8e2e0f7

File tree

7 files changed

+131
-1
lines changed

7 files changed

+131
-1
lines changed

‎src/backend/parser/parse_coerce.c

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1298,6 +1298,10 @@ parser_coercion_errposition(ParseState *pstate,
12981298
* rather than throwing an error on failure.
12991299
* 'which_expr': if not NULL, receives a pointer to the particular input
13001300
* expression from which the result type was taken.
1301+
*
1302+
* Caution: "failure" just means that there were inputs of different type
1303+
* categories. It is not guaranteed that all the inputs are coercible to the
1304+
* selected type; caller must check that (see verify_common_type).
13011305
*/
13021306
Oid
13031307
select_common_type(ParseState *pstate, List *exprs, const char *context,
@@ -1426,6 +1430,10 @@ select_common_type(ParseState *pstate, List *exprs, const char *context,
14261430
* earlier entries in the array have some preference over later ones.
14271431
* On failure, return InvalidOid if noerror is true, else throw an error.
14281432
*
1433+
* Caution: "failure" just means that there were inputs of different type
1434+
* categories. It is not guaranteed that all the inputs are coercible to the
1435+
* selected type; caller must check that (see verify_common_type_from_oids).
1436+
*
14291437
* Note: neither caller will pass any UNKNOWNOID entries, so the tests
14301438
* for that in this function are dead code. However, they don't cost much,
14311439
* and it seems better to keep this logic as close to select_common_type()
@@ -1548,6 +1556,48 @@ coerce_to_common_type(ParseState *pstate, Node *node,
15481556
return node;
15491557
}
15501558

1559+
/*
1560+
* verify_common_type()
1561+
* Verify that all input types can be coerced to a proposed common type.
1562+
* Return true if so, false if not all coercions are possible.
1563+
*
1564+
* Most callers of select_common_type() don't need to do this explicitly
1565+
* because the checks will happen while trying to convert input expressions
1566+
* to the right type, e.g. in coerce_to_common_type(). However, if a separate
1567+
* check step is needed to validate the applicability of the common type, call
1568+
* this.
1569+
*/
1570+
bool
1571+
verify_common_type(Oid common_type, List *exprs)
1572+
{
1573+
ListCell *lc;
1574+
1575+
foreach(lc, exprs)
1576+
{
1577+
Node *nexpr = (Node *) lfirst(lc);
1578+
Oid ntype = exprType(nexpr);
1579+
1580+
if (!can_coerce_type(1, &ntype, &common_type, COERCION_IMPLICIT))
1581+
return false;
1582+
}
1583+
return true;
1584+
}
1585+
1586+
/*
1587+
* verify_common_type_from_oids()
1588+
* As above, but work from an array of type OIDs.
1589+
*/
1590+
static bool
1591+
verify_common_type_from_oids(Oid common_type, int nargs, const Oid *typeids)
1592+
{
1593+
for (int i = 0; i < nargs; i++)
1594+
{
1595+
if (!can_coerce_type(1, &typeids[i], &common_type, COERCION_IMPLICIT))
1596+
return false;
1597+
}
1598+
return true;
1599+
}
1600+
15511601
/*
15521602
* select_common_typmod()
15531603
* Determine the common typmod of a list of input expressions.
@@ -1917,7 +1967,13 @@ check_generic_type_consistency(const Oid *actual_arg_types,
19171967
true);
19181968

19191969
if (!OidIsValid(anycompatible_typeid))
1920-
return false; /* there's no common supertype */
1970+
return false; /* there's definitely no common supertype */
1971+
1972+
/* We have to verify that the selected type actually works */
1973+
if (!verify_common_type_from_oids(anycompatible_typeid,
1974+
n_anycompatible_args,
1975+
anycompatible_actual_types))
1976+
return false;
19211977

19221978
if (have_anycompatible_nonarray)
19231979
{
@@ -2494,6 +2550,14 @@ enforce_generic_type_consistency(const Oid *actual_arg_types,
24942550
anycompatible_actual_types,
24952551
false);
24962552

2553+
/* We have to verify that the selected type actually works */
2554+
if (!verify_common_type_from_oids(anycompatible_typeid,
2555+
n_anycompatible_args,
2556+
anycompatible_actual_types))
2557+
ereport(ERROR,
2558+
(errcode(ERRCODE_DATATYPE_MISMATCH),
2559+
errmsg("arguments of anycompatible family cannot be cast to a common type")));
2560+
24972561
if (have_anycompatible_array)
24982562
{
24992563
anycompatible_array_typeid = get_array_type(anycompatible_typeid);

‎src/backend/parser/parse_expr.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1121,6 +1121,11 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
11211121
allexprs = list_concat(list_make1(lexpr), rnonvars);
11221122
scalar_type = select_common_type(pstate, allexprs, NULL, NULL);
11231123

1124+
/* We have to verify that the selected type actually works */
1125+
if (OidIsValid(scalar_type) &&
1126+
!verify_common_type(scalar_type, allexprs))
1127+
scalar_type = InvalidOid;
1128+
11241129
/*
11251130
* Do we have an array type to use? Aside from the case where there
11261131
* isn't one, we don't risk using ScalarArrayOpExpr when the common

‎src/include/parser/parse_coerce.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ extern Oid select_common_type(ParseState *pstate, List *exprs,
7070
extern Node *coerce_to_common_type(ParseState *pstate, Node *node,
7171
Oid targetTypeId,
7272
const char *context);
73+
extern bool verify_common_type(Oid common_type, List *exprs);
7374

7475
extern int32 select_common_typmod(ParseState *pstate, List *exprs, Oid common_type);
7576

‎src/test/regress/expected/arrays.out

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -747,6 +747,18 @@ SELECT ARRAY[1.1] || ARRAY[2,3,4];
747747
{1.1,2,3,4}
748748
(1 row)
749749

750+
SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);
751+
?column?
752+
-----------------------------------
753+
{"(1,2)","(3,4)","(1,2)","(3,4)"}
754+
(1 row)
755+
756+
SELECT ROW(1,2) || array_agg(x) FROM (VALUES (ROW(3,4)), (ROW(5,6))) v(x);
757+
?column?
758+
---------------------------
759+
{"(1,2)","(3,4)","(5,6)"}
760+
(1 row)
761+
750762
SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
751763
seqno | i | t
752764
-------+---------------------------------+------------------------------------------------------------------------------------------------------------------------------------

‎src/test/regress/expected/expressions.out

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,36 @@ explain (verbose, costs off) select * from bpchar_view
232232

233233
rollback;
234234
--
235+
-- Ordinarily, IN/NOT IN can be converted to a ScalarArrayOpExpr
236+
-- with a suitably-chosen array type.
237+
--
238+
explain (verbose, costs off)
239+
select random() IN (1, 4, 8.0);
240+
QUERY PLAN
241+
------------------------------------------------------------
242+
Result
243+
Output: (random() = ANY ('{1,4,8}'::double precision[]))
244+
(2 rows)
245+
246+
explain (verbose, costs off)
247+
select random()::int IN (1, 4, 8.0);
248+
QUERY PLAN
249+
---------------------------------------------------------------------------
250+
Result
251+
Output: (((random())::integer)::numeric = ANY ('{1,4,8.0}'::numeric[]))
252+
(2 rows)
253+
254+
-- However, if there's not a common supertype for the IN elements,
255+
-- we should instead try to produce "x = v1 OR x = v2 OR ...".
256+
-- In most cases that'll fail for lack of all the requisite = operators,
257+
-- but it can succeed sometimes. So this should complain about lack of
258+
-- an = operator, not about cast failure.
259+
select '(0,0)'::point in ('(0,0,0,0)'::box, point(0,0));
260+
ERROR: operator does not exist: point = box
261+
LINE 1: select '(0,0)'::point in ('(0,0,0,0)'::box, point(0,0));
262+
^
263+
HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
264+
--
235265
-- Tests for ScalarArrayOpExpr with a hashfn
236266
--
237267
-- create a stable function so that the tests below are not

‎src/test/regress/sql/arrays.sql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -317,6 +317,8 @@ SELECT ARRAY[[1,2],[3,4]] || ARRAY[5,6] AS "{{1,2},{3,4},{5,6}}";
317317
SELECT ARRAY[0,0] || ARRAY[1,1] || ARRAY[2,2] AS "{0,0,1,1,2,2}";
318318
SELECT 0 || ARRAY[1,2] || 3 AS "{0,1,2,3}";
319319
SELECT ARRAY[1.1] || ARRAY[2,3,4];
320+
SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);
321+
SELECT ROW(1,2) || array_agg(x) FROM (VALUES (ROW(3,4)), (ROW(5,6))) v(x);
320322

321323
SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
322324
SELECT * FROM array_op_test WHERE i && '{32}' ORDER BY seqno;

‎src/test/regress/sql/expressions.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,22 @@ explain (verbose, costs off) select * from bpchar_view
103103
rollback;
104104

105105

106+
--
107+
-- Ordinarily, IN/NOT IN can be converted to a ScalarArrayOpExpr
108+
-- with a suitably-chosen array type.
109+
--
110+
explain (verbose, costs off)
111+
select random() IN (1, 4, 8.0);
112+
explain (verbose, costs off)
113+
select random()::int IN (1, 4, 8.0);
114+
-- However, if there's not a common supertype for the IN elements,
115+
-- we should instead try to produce "x = v1 OR x = v2 OR ...".
116+
-- In most cases that'll fail for lack of all the requisite = operators,
117+
-- but it can succeed sometimes. So this should complain about lack of
118+
-- an = operator, not about cast failure.
119+
select '(0,0)'::point in ('(0,0,0,0)'::box, point(0,0));
120+
121+
106122
--
107123
-- Tests for ScalarArrayOpExpr with a hashfn
108124
--

0 commit comments

Comments
 (0)