Re: get_fn_expr_variadic considered harmful

I wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> Well, that argues for the choice of trying to make them equivalent
>> again, I suppose, but it sounds like there are some nasty edge cases
>> that won't easily be filed down.  I think your idea of redefining
>> funcvariadic to be true only for VARIADIC ANY is probably a promising
>> approach to that solution, but as you say it leaves some problems
>> unsolved.

> I think what we'll have to do is tell complainants to recreate any
> affected indexes or rules after installing 9.3.5.  Given the relatively
> small number of complaints, I don't think it's worth working harder,
> nor taking risks like inserting catalog lookups into readfuncs.c.

After some thought, it seems to me that the best solution is to redefine
funcvariadic as meaning "the last actual argument is a variadic array",
which means it will always be true for a VARIADIC non-ANY function.
In HEAD, that leads to a nice patch that actually simplifies the logic
in ruleutils.c, as attached.  In 9.3, we will not be able to assume that
funcvariadic has that meaning, so we'll have to use the existing
decompilation logic (which basically ignores funcvariadic unless it's a
VARIADIC ANY function).  I've not made that version of the patch yet but
it should be pretty straightforward.

            regards, tom lane

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 32c0135..e54d46d 100644
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
*************** deparseFuncExpr(FuncExpr *node, deparse_
*** 1548,1562 ****
      procform = (Form_pg_proc) GETSTRUCT(proctup);

      /* Check if need to print VARIADIC (cf. ruleutils.c) */
!     if (OidIsValid(procform->provariadic))
!     {
!         if (procform->provariadic != ANYOID)
!             use_variadic = true;
!         else
!             use_variadic = node->funcvariadic;
!     }
!     else
!         use_variadic = false;

      /* Print schema name only if it's not pg_catalog */
      if (procform->pronamespace != PG_CATALOG_NAMESPACE)
--- 1548,1554 ----
      procform = (Form_pg_proc) GETSTRUCT(proctup);

      /* Check if need to print VARIADIC (cf. ruleutils.c) */
!     use_variadic = node->funcvariadic;

      /* Print schema name only if it's not pg_catalog */
      if (procform->pronamespace != PG_CATALOG_NAMESPACE)
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 2b4ade0..941b101 100644
*** a/doc/src/sgml/xfunc.sgml
--- b/doc/src/sgml/xfunc.sgml
*************** CREATE OR REPLACE FUNCTION retcomposite(
*** 3152,3160 ****
       is zero based.  <function>get_call_result_type</> can also be used
       as an alternative to <function>get_fn_expr_rettype</>.
       There is also <function>get_fn_expr_variadic</>, which can be used to
!      find out whether the call contained an explicit <literal>VARIADIC</>
!      keyword.  This is primarily useful for <literal>VARIADIC "any"</>
!      functions, as described below.
      </para>

      <para>
--- 3152,3161 ----
       is zero based.  <function>get_call_result_type</> can also be used
       as an alternative to <function>get_fn_expr_rettype</>.
       There is also <function>get_fn_expr_variadic</>, which can be used to
!      find out whether variadic arguments have been merged into an array.
!      This is primarily useful for <literal>VARIADIC "any"</> functions,
!      since such merging will always have occurred for variadic functions
!      taking ordinary array types.
      </para>

      <para>
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 5934ab0..cc46084 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*************** ParseFuncOrColumn(ParseState *pstate, Li
*** 556,561 ****
--- 556,572 ----
      make_fn_arguments(pstate, fargs, actual_arg_types, declared_arg_types);

      /*
+      * If the function isn't actually variadic, forget any VARIADIC decoration
+      * on the call.  (Perhaps we should throw an error instead, but
+      * historically we've allowed people to write that.)
+      */
+     if (!OidIsValid(vatype))
+     {
+         Assert(nvargs == 0);
+         func_variadic = false;
+     }
+
+     /*
       * If it's a variadic function call, transform the last nvargs arguments
       * into an array --- unless it's an "any" variadic.
       */
*************** ParseFuncOrColumn(ParseState *pstate, Li
*** 584,589 ****
--- 595,605 ----
          newa->location = exprLocation((Node *) vargs);

          fargs = lappend(fargs, newa);
+
+         /* We could not have had VARIADIC marking before ... */
+         Assert(!func_variadic);
+         /* ... but now, it's a VARIADIC call */
+         func_variadic = true;
      }

      /*
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 566b4c9..b1bac86 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*************** static char *get_relation_name(Oid relid
*** 401,407 ****
  static char *generate_relation_name(Oid relid, List *namespaces);
  static char *generate_function_name(Oid funcid, int nargs,
                         List *argnames, Oid *argtypes,
!                        bool was_variadic, bool *use_variadic_p);
  static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
  static text *string_to_text(char *str);
  static char *flatten_reloptions(Oid relid);
--- 401,407 ----
  static char *generate_relation_name(Oid relid, List *namespaces);
  static char *generate_function_name(Oid funcid, int nargs,
                         List *argnames, Oid *argtypes,
!                        bool has_variadic, bool *use_variadic_p);
  static char *generate_operator_name(Oid operid, Oid arg1, Oid arg2);
  static text *string_to_text(char *str);
  static char *flatten_reloptions(Oid relid);
*************** generate_relation_name(Oid relid, List *
*** 8849,8864 ****
   *
   * If we're dealing with a potentially variadic function (in practice, this
   * means a FuncExpr or Aggref, not some other way of calling a function), then
!  * was_variadic must specify whether VARIADIC appeared in the original call,
   * and *use_variadic_p will be set to indicate whether to print VARIADIC in
!  * the output.    For non-FuncExpr cases, was_variadic should be FALSE and
   * use_variadic_p can be NULL.
   *
   * The result includes all necessary quoting and schema-prefixing.
   */
  static char *
  generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
!                        bool was_variadic, bool *use_variadic_p)
  {
      char       *result;
      HeapTuple    proctup;
--- 8849,8864 ----
   *
   * If we're dealing with a potentially variadic function (in practice, this
   * means a FuncExpr or Aggref, not some other way of calling a function), then
!  * has_variadic must specify whether variadic arguments have been merged,
   * and *use_variadic_p will be set to indicate whether to print VARIADIC in
!  * the output.    For non-FuncExpr cases, has_variadic should be FALSE and
   * use_variadic_p can be NULL.
   *
   * The result includes all necessary quoting and schema-prefixing.
   */
  static char *
  generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
!                        bool has_variadic, bool *use_variadic_p)
  {
      char       *result;
      HeapTuple    proctup;
*************** generate_function_name(Oid funcid, int n
*** 8884,8915 ****
       * Determine whether VARIADIC should be printed.  We must do this first
       * since it affects the lookup rules in func_get_detail().
       *
!      * Currently, we always print VARIADIC if the function is variadic and
!      * takes a variadic type other than ANY.  (In principle, if VARIADIC
!      * wasn't originally specified and the array actual argument is
!      * deconstructable, we could print the array elements separately and not
!      * print VARIADIC, thus more nearly reproducing the original input.  For
!      * the moment that seems like too much complication for the benefit.)
!      * However, if the function takes VARIADIC ANY, then the parser didn't
!      * fold the arguments together into an array, so we must print VARIADIC if
!      * and only if it was used originally.
       */
      if (use_variadic_p)
      {
!         if (OidIsValid(procform->provariadic))
!         {
!             if (procform->provariadic != ANYOID)
!                 use_variadic = true;
!             else
!                 use_variadic = was_variadic;
!         }
!         else
!             use_variadic = false;
          *use_variadic_p = use_variadic;
      }
      else
      {
!         Assert(!was_variadic);
          use_variadic = false;
      }

--- 8884,8910 ----
       * Determine whether VARIADIC should be printed.  We must do this first
       * since it affects the lookup rules in func_get_detail().
       *
!      * Currently, we always print VARIADIC if the function has a merged
!      * variadic-array argument.  Note that this is always the case for
!      * functions taking a VARIADIC argument type other than VARIADIC ANY.
!      *
!      * In principle, if VARIADIC wasn't originally specified and the array
!      * actual argument is deconstructable, we could print the array elements
!      * separately and not print VARIADIC, thus more nearly reproducing the
!      * original input.  For the moment that seems like too much complication
!      * for the benefit, and anyway we do not know whether VARIADIC was
!      * originally specified if it's a non-ANY type.
       */
      if (use_variadic_p)
      {
!         /* Parser should not have set funcvariadic unless fn is variadic */
!         Assert(!has_variadic || OidIsValid(procform->provariadic));
!         use_variadic = has_variadic;
          *use_variadic_p = use_variadic;
      }
      else
      {
!         Assert(!has_variadic);
          use_variadic = false;
      }

diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index c7024b2..1fe45da 100644
*** a/src/backend/utils/fmgr/fmgr.c
--- b/src/backend/utils/fmgr/fmgr.c
*************** get_call_expr_arg_stable(Node *expr, int
*** 2449,2454 ****
--- 2449,2456 ----
   * Get the VARIADIC flag from the function invocation
   *
   * Returns false (the default assumption) if information is not available
+  *
+  * Note this is generally only of interest to VARIADIC ANY functions
   */
  bool
  get_fn_expr_variadic(FmgrInfo *flinfo)
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 3c74fa0..e1a04c8 100644
*** a/src/include/catalog/catversion.h
--- b/src/include/catalog/catversion.h
***************
*** 53,58 ****
   */

  /*                            yyyymmddN */
! #define CATALOG_VERSION_NO    201403261

  #endif
--- 53,58 ----
   */

  /*                            yyyymmddN */
! #define CATALOG_VERSION_NO    201404031

  #endif
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 4992bc0..9cce60b 100644
*** a/src/include/nodes/primnodes.h
--- b/src/include/nodes/primnodes.h
*************** typedef struct Aggref
*** 257,263 ****
      List       *aggdistinct;    /* DISTINCT (list of SortGroupClause) */
      Expr       *aggfilter;        /* FILTER expression, if any */
      bool        aggstar;        /* TRUE if argument list was really '*' */
!     bool        aggvariadic;    /* TRUE if VARIADIC was used in call */
      char        aggkind;        /* aggregate kind (see pg_aggregate.h) */
      Index        agglevelsup;    /* > 0 if agg belongs to outer query */
      int            location;        /* token location, or -1 if unknown */
--- 257,264 ----
      List       *aggdistinct;    /* DISTINCT (list of SortGroupClause) */
      Expr       *aggfilter;        /* FILTER expression, if any */
      bool        aggstar;        /* TRUE if argument list was really '*' */
!     bool        aggvariadic;    /* true if variadic arguments have been
!                                  * combined into an array last argument */
      char        aggkind;        /* aggregate kind (see pg_aggregate.h) */
      Index        agglevelsup;    /* > 0 if agg belongs to outer query */
      int            location;        /* token location, or -1 if unknown */
*************** typedef struct FuncExpr
*** 358,364 ****
      Oid            funcid;            /* PG_PROC OID of the function */
      Oid            funcresulttype; /* PG_TYPE OID of result value */
      bool        funcretset;        /* true if function returns set */
!     bool        funcvariadic;    /* true if VARIADIC was used in call */
      CoercionForm funcformat;    /* how to display this function call */
      Oid            funccollid;        /* OID of collation of result */
      Oid            inputcollid;    /* OID of collation that function should use */
--- 359,366 ----
      Oid            funcid;            /* PG_PROC OID of the function */
      Oid            funcresulttype; /* PG_TYPE OID of result value */
      bool        funcretset;        /* true if function returns set */
!     bool        funcvariadic;    /* true if variadic arguments have been
!                                  * combined into an array last argument */
      CoercionForm funcformat;    /* how to display this function call */
      Oid            funccollid;        /* OID of collation of result */
      Oid            inputcollid;    /* OID of collation that function should use */