Thread: Re: [GENERAL] pg_restore casts check constraints differently

I wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
>> destdb=# ALTER TABLE c ADD CONSTRAINT p_a_check CHECK (a IN ('a', 'b', 'c'));
>> destdb=# \d c
>> ...
>> Check constraints:
>> "p_a_check" CHECK (a::text = ANY (ARRAY['a'::character varying,
>> 'b'::character varying, 'c'::character varying]::text[]))

> Hm.  It seems like the parser is doing something weird with IN there.
> I wonder why you don't get an array of text constants in the IN case.

I poked into this and determined that it happens because transformAExprIn
identifies the array type to use without considering whether an additional
coercion will have to happen before the array elements can be compared to
the lefthand input.

I tried to fix that in a principled fashion, by resolving the actual
comparison operator and using its righthand input type as the array
element type (see first patch attached).  That fixes this case all right,
but it also makes several existing regression test cases fail, for
example:

***************
*** 381,392 ****
     FROM pg_class
     WHERE oid::regclass IN ('a_star', 'c_star')
     ORDER BY 1;
!  relname | has_toast_table
! ---------+-----------------
!  a_star  | t
!  c_star  | t
! (2 rows)
!
  --UPDATE b_star*
  --   SET a = text 'gazpacho'
  --   WHERE aa > 4;
--- 381,389 ----
     FROM pg_class
     WHERE oid::regclass IN ('a_star', 'c_star')
     ORDER BY 1;
! ERROR:  invalid input syntax for type oid: "a_star"
! LINE 3:    WHERE oid::regclass IN ('a_star', 'c_star')
!                                    ^
  --UPDATE b_star*
  --   SET a = text 'gazpacho'
  --   WHERE aa > 4;

The problem is that regclass, like varchar, has no comparison operators
of its own, relying on OID's operators.  So this patch causes us to choose
OID not regclass as the type of the unknown literals, which in this case
seems like a loss of useful behavior.

I'm tempted to just improve the situation for varchar with a complete
kluge, ie the second patch attached.  Thoughts?

            regards, tom lane

diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index ebda55d..8d75c88 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
***************
*** 15,20 ****
--- 15,22 ----

  #include "postgres.h"

+ #include "access/htup_details.h"
+ #include "catalog/pg_operator.h"
  #include "catalog/pg_type.h"
  #include "commands/dbcommands.h"
  #include "miscadmin.h"
***************
*** 35,40 ****
--- 37,43 ----
  #include "parser/parse_agg.h"
  #include "utils/builtins.h"
  #include "utils/lsyscache.h"
+ #include "utils/syscache.h"
  #include "utils/xml.h"


*************** transformAExprIn(ParseState *pstate, A_E
*** 1148,1166 ****
       */
      if (list_length(rnonvars) > 1)
      {
-         List       *allexprs;
          Oid            scalar_type;
          Oid            array_type;

          /*
!          * Try to select a common type for the array elements.  Note that
!          * since the LHS' type is first in the list, it will be preferred when
!          * there is doubt (eg, when all the RHS items are unknown literals).
!          *
!          * Note: use list_concat here not lcons, to avoid damaging rnonvars.
           */
!         allexprs = list_concat(list_make1(lexpr), rnonvars);
!         scalar_type = select_common_type(pstate, allexprs, NULL, NULL);

          /*
           * Do we have an array type to use?  Aside from the case where there
--- 1151,1225 ----
       */
      if (list_length(rnonvars) > 1)
      {
          Oid            scalar_type;
          Oid            array_type;
+         Node       *which_expr;

          /*
!          * Try to select a common type for the array elements.  In the general
!          * case we should let select_common_type() do this.  But if the RHS is
!          * all unknown literals, select_common_type() will return TEXT which
!          * we do not want here: we want to let oper() see the unknown type and
!          * resolve it.  So, if it said TEXT but the input is really UNKNOWN,
!          * go back to UNKNOWN.
           */
!         scalar_type = select_common_type(pstate, rnonvars, NULL, &which_expr);
!
!         if (scalar_type == TEXTOID && exprType(which_expr) == UNKNOWNOID)
!             scalar_type = UNKNOWNOID;
!
!         /*
!          * If we identified a common type, check to see if that's actually
!          * what the operator wants, and if not change to what it does want.
!          * This takes care of resolving UNKNOWN as some appropriate type.
!          * Also, if the operator would require a coercion, we might as well do
!          * the coercion on the individual array elements rather than having to
!          * apply an array-level coercion.
!          */
!         if (OidIsValid(scalar_type))
!         {
!             /*
!              * This is a cut-down version of make_scalar_array_op()'s logic.
!              * We need not bother to duplicate error checks it will make.
!              */
!             Oid            ltypeId,
!                         rtypeId;
!             Operator    tup;
!             Form_pg_operator opform;
!             Oid            actual_arg_types[2];
!             Oid            declared_arg_types[2];
!
!             ltypeId = exprType(lexpr);
!             rtypeId = scalar_type;
!
!             /* Resolve the operator */
!             tup = oper(pstate, a->name, ltypeId, rtypeId, false, a->location);
!             opform = (Form_pg_operator) GETSTRUCT(tup);
!
!             actual_arg_types[0] = ltypeId;
!             actual_arg_types[1] = rtypeId;
!             declared_arg_types[0] = opform->oprleft;
!             declared_arg_types[1] = opform->oprright;
!
!             /*
!              * enforce consistency with polymorphic argument and return types,
!              * possibly adjusting return type or declared_arg_types
!              */
!             (void) enforce_generic_type_consistency(actual_arg_types,
!                                                     declared_arg_types,
!                                                     2,
!                                                     opform->oprresult,
!                                                     false);
!
!             /*
!              * If operator is polymorphic, assume scalar_type is OK as is,
!              * otherwise absorb the correct input type.
!              */
!             if (!IsPolymorphicType(declared_arg_types[1]))
!                 scalar_type = declared_arg_types[1];
!
!             ReleaseSysCache(tup);
!         }

          /*
           * Do we have an array type to use?  Aside from the case where there
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index ebda55d..7f314f5 100644
*** a/src/backend/parser/parse_expr.c
--- b/src/backend/parser/parse_expr.c
*************** transformAExprIn(ParseState *pstate, A_E
*** 1163,1168 ****
--- 1163,1181 ----
          scalar_type = select_common_type(pstate, allexprs, NULL, NULL);

          /*
+          * If the common type is varchar, use text, to skip the inevitable
+          * coercion later (since varchar depends on text's operators).  This
+          * is pretty ugly, but it's hard to see how to do it in a more
+          * principled way; there are seemingly-equivalent cases where we do
+          * NOT want to substitute the comparison operator's actual input type.
+          * For example, replacing regclass by oid would break several useful
+          * cases in the regression tests.  Varchar is common enough that it
+          * seems worth a kluge here.
+          */
+         if (scalar_type == VARCHAROID)
+             scalar_type = TEXTOID;
+
+         /*
           * 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
           * type is RECORD, because the RowExpr comparison logic below can cope

On Thu, Mar 31, 2016 at 1:00 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> Amit Langote <amitlangote09@gmail.com> writes:
>>> destdb=# ALTER TABLE c ADD CONSTRAINT p_a_check CHECK (a IN ('a', 'b', 'c'));
>>> destdb=# \d c
>>> ...
>>> Check constraints:
>>> "p_a_check" CHECK (a::text = ANY (ARRAY['a'::character varying,
>>> 'b'::character varying, 'c'::character varying]::text[]))
>
>> Hm.  It seems like the parser is doing something weird with IN there.
>> I wonder why you don't get an array of text constants in the IN case.
>
> I poked into this and determined that it happens because transformAExprIn
> identifies the array type to use without considering whether an additional
> coercion will have to happen before the array elements can be compared to
> the lefthand input.
>
> I tried to fix that in a principled fashion, by resolving the actual
> comparison operator and using its righthand input type as the array
> element type (see first patch attached).  That fixes this case all right,
> but it also makes several existing regression test cases fail, for
> example:
>
> ***************
> *** 381,392 ****
>      FROM pg_class
>      WHERE oid::regclass IN ('a_star', 'c_star')
>      ORDER BY 1;
> !  relname | has_toast_table
> ! ---------+-----------------
> !  a_star  | t
> !  c_star  | t
> ! (2 rows)
> !
>   --UPDATE b_star*
>   --   SET a = text 'gazpacho'
>   --   WHERE aa > 4;
> --- 381,389 ----
>      FROM pg_class
>      WHERE oid::regclass IN ('a_star', 'c_star')
>      ORDER BY 1;
> ! ERROR:  invalid input syntax for type oid: "a_star"
> ! LINE 3:    WHERE oid::regclass IN ('a_star', 'c_star')
> !                                    ^
>   --UPDATE b_star*
>   --   SET a = text 'gazpacho'
>   --   WHERE aa > 4;
>
> The problem is that regclass, like varchar, has no comparison operators
> of its own, relying on OID's operators.  So this patch causes us to choose
> OID not regclass as the type of the unknown literals, which in this case
> seems like a loss of useful behavior.

Agreed; no need to break that.

> I'm tempted to just improve the situation for varchar with a complete
> kluge, ie the second patch attached.  Thoughts?

Fixes for me.

Thanks,
Amit