Re: BUG #17387: Working in PG13 but not in PGH14: array_agg(RECORD)

I wrote:
> We need something like the attached, but I'm going to nose
> around for other oversights.

Sure enough, transformAExprIn has a related bug: if all the IN
arguments are of the same typcategory, it will try to stuff them
all into an array, whether or not they're actually coercible
to a common type.  We should fall back to the x = v1 OR x = v2 ...
interpretation when that's not possible.  I failed to come up
with a simple example in which that leads to success; odds are
that there's no suitable = operators either.  But it's probably
possible with some weird set of user-defined types.  In any
case, the intent of the existing code is clearly that this
should happen.

That bug is ancient, more than 10 years old.  Given the lack
of field complaints I'm not terribly worried about back-patching,
but I suppose it can go into v14 along with the regression fix.

            regards, tom lane

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index d674e31405..c4e958e4aa 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -1298,6 +1298,10 @@ parser_coercion_errposition(ParseState *pstate,
  * rather than throwing an error on failure.
  * 'which_expr': if not NULL, receives a pointer to the particular input
  * expression from which the result type was taken.
+ *
+ * Caution: "failure" just means that there were inputs of different type
+ * categories.  It is not guaranteed that all the inputs are coercible to the
+ * selected type; caller must check that (see verify_common_type).
  */
 Oid
 select_common_type(ParseState *pstate, List *exprs, const char *context,
@@ -1426,6 +1430,10 @@ select_common_type(ParseState *pstate, List *exprs, const char *context,
  * earlier entries in the array have some preference over later ones.
  * On failure, return InvalidOid if noerror is true, else throw an error.
  *
+ * Caution: "failure" just means that there were inputs of different type
+ * categories.  It is not guaranteed that all the inputs are coercible to the
+ * selected type; caller must check that (see verify_common_type_from_oids).
+ *
  * Note: neither caller will pass any UNKNOWNOID entries, so the tests
  * for that in this function are dead code.  However, they don't cost much,
  * 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,
     return node;
 }

+/*
+ * verify_common_type()
+ *        Verify that all input types can be coerced to a proposed common type.
+ *        Return true if so, false if not all coercions are possible.
+ *
+ * Most callers of select_common_type() don't need to do this explicitly
+ * because the checks will happen while trying to convert input expressions
+ * to the right type, e.g. in coerce_to_common_type().  However, if a separate
+ * check step is needed to validate the applicability of the common type, call
+ * this.
+ */
+bool
+verify_common_type(Oid common_type, List *exprs)
+{
+    ListCell   *lc;
+
+    foreach(lc, exprs)
+    {
+        Node       *nexpr = (Node *) lfirst(lc);
+        Oid            ntype = exprType(nexpr);
+
+        if (!can_coerce_type(1, &ntype, &common_type, COERCION_IMPLICIT))
+            return false;
+    }
+    return true;
+}
+
+/*
+ * verify_common_type_from_oids()
+ *        As above, but work from an array of type OIDs.
+ */
+static bool
+verify_common_type_from_oids(Oid common_type, int nargs, const Oid *typeids)
+{
+    for (int i = 0; i < nargs; i++)
+    {
+        if (!can_coerce_type(1, &typeids[i], &common_type, COERCION_IMPLICIT))
+            return false;
+    }
+    return true;
+}
+
 /*
  * select_common_typmod()
  *        Determine the common typmod of a list of input expressions.
@@ -1917,7 +1967,13 @@ check_generic_type_consistency(const Oid *actual_arg_types,
                                          true);

         if (!OidIsValid(anycompatible_typeid))
-            return false;        /* there's no common supertype */
+            return false;        /* there's definitely no common supertype */
+
+        /* We have to verify that the selected type actually works */
+        if (!verify_common_type_from_oids(anycompatible_typeid,
+                                          n_anycompatible_args,
+                                          anycompatible_actual_types))
+            return false;

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

+            /* We have to verify that the selected type actually works */
+            if (!verify_common_type_from_oids(anycompatible_typeid,
+                                              n_anycompatible_args,
+                                              anycompatible_actual_types))
+                ereport(ERROR,
+                        (errcode(ERRCODE_DATATYPE_MISMATCH),
+                         errmsg("arguments of anycompatible family cannot be cast to a common type")));
+
             if (have_anycompatible_array)
             {
                 anycompatible_array_typeid = get_array_type(anycompatible_typeid);
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index c4aaf37727..1c09ea24cd 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -1121,6 +1121,11 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
         allexprs = list_concat(list_make1(lexpr), rnonvars);
         scalar_type = select_common_type(pstate, allexprs, NULL, NULL);

+        /* We have to verify that the selected type actually works */
+        if (OidIsValid(scalar_type) &&
+            !verify_common_type(scalar_type, allexprs))
+            scalar_type = InvalidOid;
+
         /*
          * Do we have an array type to use?  Aside from the case where there
          * isn't one, we don't risk using ScalarArrayOpExpr when the common
diff --git a/src/include/parser/parse_coerce.h b/src/include/parser/parse_coerce.h
index fe046a2c03..b105c7da90 100644
--- a/src/include/parser/parse_coerce.h
+++ b/src/include/parser/parse_coerce.h
@@ -70,6 +70,7 @@ extern Oid    select_common_type(ParseState *pstate, List *exprs,
 extern Node *coerce_to_common_type(ParseState *pstate, Node *node,
                                    Oid targetTypeId,
                                    const char *context);
+extern bool verify_common_type(Oid common_type, List *exprs);

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

diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 3e3a1beaab..18e4e25bf4 100644
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -747,6 +747,12 @@ SELECT ARRAY[1.1] || ARRAY[2,3,4];
  {1.1,2,3,4}
 (1 row)

+SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);
+             ?column?
+-----------------------------------
+ {"(1,2)","(3,4)","(1,2)","(3,4)"}
+(1 row)
+
 SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
  seqno |                i                |                                                                 t
                                                       

-------+---------------------------------+------------------------------------------------------------------------------------------------------------------------------------
diff --git a/src/test/regress/expected/expressions.out b/src/test/regress/expected/expressions.out
index 6406fb3a76..84ab7771dd 100644
--- a/src/test/regress/expected/expressions.out
+++ b/src/test/regress/expected/expressions.out
@@ -232,6 +232,36 @@ explain (verbose, costs off) select * from bpchar_view

 rollback;
 --
+-- Ordinarily, IN/NOT IN can be converted to a ScalarArrayOpExpr
+-- with a suitably-chosen array type.
+--
+explain (verbose, costs off)
+select random() IN (1, 4, 8.0);
+                         QUERY PLAN
+------------------------------------------------------------
+ Result
+   Output: (random() = ANY ('{1,4,8}'::double precision[]))
+(2 rows)
+
+explain (verbose, costs off)
+select random()::int IN (1, 4, 8.0);
+                                QUERY PLAN
+---------------------------------------------------------------------------
+ Result
+   Output: (((random())::integer)::numeric = ANY ('{1,4,8.0}'::numeric[]))
+(2 rows)
+
+-- However, if there's not a common supertype for the IN elements,
+-- we should instead try to produce "x = v1 OR x = v2 OR ...".
+-- In most cases that'll fail for lack of all the requisite = operators,
+-- but it can succeed sometimes.  So this should complain about lack of
+-- an = operator, not about cast failure.
+select '(0,0)'::point in ('(0,0,0,0)'::box, point(0,0));
+ERROR:  operator does not exist: point = box
+LINE 1: select '(0,0)'::point in ('(0,0,0,0)'::box, point(0,0));
+                              ^
+HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.
+--
 -- Tests for ScalarArrayOpExpr with a hashfn
 --
 -- create a stable function so that the tests below are not
diff --git a/src/test/regress/sql/arrays.sql b/src/test/regress/sql/arrays.sql
index 912233ef96..03e56bf924 100644
--- a/src/test/regress/sql/arrays.sql
+++ b/src/test/regress/sql/arrays.sql
@@ -317,6 +317,7 @@ SELECT ARRAY[[1,2],[3,4]] || ARRAY[5,6] AS "{{1,2},{3,4},{5,6}}";
 SELECT ARRAY[0,0] || ARRAY[1,1] || ARRAY[2,2] AS "{0,0,1,1,2,2}";
 SELECT 0 || ARRAY[1,2] || 3 AS "{0,1,2,3}";
 SELECT ARRAY[1.1] || ARRAY[2,3,4];
+SELECT array_agg(x) || array_agg(x) FROM (VALUES (ROW(1,2)), (ROW(3,4))) v(x);

 SELECT * FROM array_op_test WHERE i @> '{32}' ORDER BY seqno;
 SELECT * FROM array_op_test WHERE i && '{32}' ORDER BY seqno;
diff --git a/src/test/regress/sql/expressions.sql b/src/test/regress/sql/expressions.sql
index 03b88bde7a..755370f358 100644
--- a/src/test/regress/sql/expressions.sql
+++ b/src/test/regress/sql/expressions.sql
@@ -103,6 +103,22 @@ explain (verbose, costs off) select * from bpchar_view
 rollback;


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