Skip to content

Commit 5ad7056

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 e90f258 commit 5ad7056

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
@@ -1295,6 +1295,10 @@ parser_coercion_errposition(ParseState *pstate,
12951295
* rather than throwing an error on failure.
12961296
* 'which_expr': if not NULL, receives a pointer to the particular input
12971297
* expression from which the result type was taken.
1298+
*
1299+
* Caution: "failure" just means that there were inputs of different type
1300+
* categories. It is not guaranteed that all the inputs are coercible to the
1301+
* selected type; caller must check that (see verify_common_type).
12981302
*/
12991303
Oid
13001304
select_common_type(ParseState *pstate, List *exprs, const char *context,
@@ -1423,6 +1427,10 @@ select_common_type(ParseState *pstate, List *exprs, const char *context,
14231427
* earlier entries in the array have some preference over later ones.
14241428
* On failure, return InvalidOid if noerror is true, else throw an error.
14251429
*
1430+
* Caution: "failure" just means that there were inputs of different type
1431+
* categories. It is not guaranteed that all the inputs are coercible to the
1432+
* selected type; caller must check that (see verify_common_type_from_oids).
1433+
*
14261434
* Note: neither caller will pass any UNKNOWNOID entries, so the tests
14271435
* for that in this function are dead code. However, they don't cost much,
14281436
* and it seems better to keep this logic as close to select_common_type()
@@ -1545,6 +1553,48 @@ coerce_to_common_type(ParseState *pstate, Node *node,
15451553
return node;
15461554
}
15471555

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

17931843
if (!OidIsValid(anycompatible_typeid))
1794-
return false; /* there's no common supertype */
1844+
return false; /* there's definitely no common supertype */
1845+
1846+
/* We have to verify that the selected type actually works */
1847+
if (!verify_common_type_from_oids(anycompatible_typeid,
1848+
n_anycompatible_args,
1849+
anycompatible_actual_types))
1850+
return false;
17951851

17961852
if (have_anycompatible_nonarray)
17971853
{
@@ -2222,6 +2278,14 @@ enforce_generic_type_consistency(const Oid *actual_arg_types,
22222278
anycompatible_actual_types,
22232279
false);
22242280

2281+
/* We have to verify that the selected type actually works */
2282+
if (!verify_common_type_from_oids(anycompatible_typeid,
2283+
n_anycompatible_args,
2284+
anycompatible_actual_types))
2285+
ereport(ERROR,
2286+
(errcode(ERRCODE_DATATYPE_MISMATCH),
2287+
errmsg("arguments of anycompatible family cannot be cast to a common type")));
2288+
22252289
if (have_anycompatible_array)
22262290
{
22272291
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
@@ -1283,6 +1283,11 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
12831283
allexprs = list_concat(list_make1(lexpr), rnonvars);
12841284
scalar_type = select_common_type(pstate, allexprs, NULL, NULL);
12851285

1286+
/* We have to verify that the selected type actually works */
1287+
if (OidIsValid(scalar_type) &&
1288+
!verify_common_type(scalar_type, allexprs))
1289+
scalar_type = InvalidOid;
1290+
12861291
/*
12871292
* Do we have an array type to use? Aside from the case where there
12881293
* 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 bool check_generic_type_consistency(const Oid *actual_arg_types,
7576
const Oid *declared_arg_types,

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,18 @@ SELECT 0 || ARRAY[1,2] || 3 AS "{0,1,2,3}";
726726
{0,1,2,3}
727727
(1 row)
728728

729+
SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);
730+
?column?
731+
-----------------------------------
732+
{"(1,2)","(3,4)","(1,2)","(3,4)"}
733+
(1 row)
734+
735+
SELECT ROW(1,2) || array_agg(x) FROM (VALUES (ROW(3,4)), (ROW(5,6))) v(x);
736+
?column?
737+
---------------------------
738+
{"(1,2)","(3,4)","(5,6)"}
739+
(1 row)
740+
729741
SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
730742
seqno | i | t
731743
-------+---------------------------------+------------------------------------------------------------------------------------------------------------------------------------

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,3 +231,33 @@ explain (verbose, costs off) select * from bpchar_view
231231
(3 rows)
232232

233233
rollback;
234+
--
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.

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,8 @@ SELECT ARRAY[[['hello','world']]] || ARRAY[[['happy','birthday']]] AS "ARRAY";
311311
SELECT ARRAY[[1,2],[3,4]] || ARRAY[5,6] AS "{{1,2},{3,4},{5,6}}";
312312
SELECT ARRAY[0,0] || ARRAY[1,1] || ARRAY[2,2] AS "{0,0,1,1,2,2}";
313313
SELECT 0 || ARRAY[1,2] || 3 AS "{0,1,2,3}";
314+
SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);
315+
SELECT ROW(1,2) || array_agg(x) FROM (VALUES (ROW(3,4)), (ROW(5,6))) v(x);
314316

315317
SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
316318
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
@@ -101,3 +101,19 @@ explain (verbose, costs off) select * from bpchar_view
101101
where f1::bpchar = 'foo';
102102

103103
rollback;
104+
105+
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));

0 commit comments

Comments
 (0)