]> granicus.if.org Git - postgresql/commitdiff
Fix non-equivalence of VARIADIC and non-VARIADIC function call formats.
authorTom Lane <tgl@sss.pgh.pa.us>
Fri, 4 Apr 2014 02:02:24 +0000 (22:02 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Fri, 4 Apr 2014 02:02:24 +0000 (22:02 -0400)
For variadic functions (other than VARIADIC ANY), the syntaxes foo(x,y,...)
and foo(VARIADIC ARRAY[x,y,...]) should be considered equivalent, since the
former is converted to the latter at parse time.  They have indeed been
equivalent, in all releases before 9.3.  However, commit 75b39e790 made an
ill-considered decision to record which syntax had been used in FuncExpr
nodes, and then to make equal() test that in checking node equality ---
which caused the syntaxes to not be seen as equivalent by the planner.
This is the underlying cause of bug #9817 from Dmitry Ryabov.

It might seem that a quick fix would be to make equal() disregard
FuncExpr.funcvariadic, but the same commit made that untenable, because
the field actually *is* semantically significant for some VARIADIC ANY
functions.  This patch instead adopts the approach of redefining
funcvariadic (and aggvariadic, in HEAD) as meaning that the last argument
is a variadic array, whether it got that way by parser intervention or was
supplied explicitly by the user.  Therefore the value will always be true
for non-ANY variadic functions, restoring the principle of equivalence.
(However, the planner will continue to consider use of VARIADIC as a
meaningful difference for VARIADIC ANY functions, even though some such
functions might disregard it.)

In HEAD, this change lets us simplify the decompilation logic in
ruleutils.c, since the funcvariadic/aggvariadic flag tells directly whether
to print VARIADIC.  However, in 9.3 we have to continue to cope with
existing stored rules/views that might contain the previous definition.
Fortunately, this just means no change in ruleutils.c, since its existing
behavior effectively ignores funcvariadic for all cases other than VARIADIC
ANY functions.

In HEAD, bump catversion to reflect the fact that FuncExpr.funcvariadic
changed meanings; this is sort of pro forma, since I don't believe any
built-in views are affected.

Unfortunately, this patch doesn't magically fix everything for affected
9.3 users.  After installing 9.3.5, they might need to recreate their
rules/views/indexes containing variadic function calls in order to get
everything consistent with the new definition.  As in the cited bug,
the symptom of a problem would be failure to use a nominally matching
index that has a variadic function call in its definition.  We'll need
to mention this in the 9.3.5 release notes.

contrib/postgres_fdw/deparse.c
doc/src/sgml/xfunc.sgml
src/backend/parser/parse_func.c
src/backend/utils/adt/ruleutils.c
src/backend/utils/fmgr/fmgr.c
src/include/catalog/catversion.h
src/include/nodes/primnodes.h

index 32c0135071468d3b6a818886267d73eb6ed38cc5..e54d46d62b18441bb1dd4c92d3a5df08b2674391 100644 (file)
@@ -1548,15 +1548,7 @@ deparseFuncExpr(FuncExpr *node, deparse_expr_cxt *context)
        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;
+       use_variadic = node->funcvariadic;
 
        /* Print schema name only if it's not pg_catalog */
        if (procform->pronamespace != PG_CATALOG_NAMESPACE)
index 2b4ade0ffb315e72077bec3ff0e7a2195c57fb19..941b101f393fdd0a127303dc05f016f2faf5467d 100644 (file)
@@ -3152,9 +3152,10 @@ CREATE OR REPLACE FUNCTION retcomposite(IN integer, IN integer,
      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.
+     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>
index 5934ab029757f9d59fd49b5c8e966ec5ffcc43c2..cc4608417b2c5b5c0ad6574903be7fd6dca28962 100644 (file)
@@ -555,6 +555,17 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
        /* perform the necessary typecasting of arguments */
        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.
@@ -584,6 +595,11 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
                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;
        }
 
        /*
index 566b4c910b34a9a251c9f289115d1575faad1421..b1bac866aa16de30e2a3c73cb22c89bce5aea386 100644 (file)
@@ -401,7 +401,7 @@ static char *get_relation_name(Oid relid);
 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);
+                                          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);
@@ -8849,16 +8849,16 @@ generate_relation_name(Oid relid, List *namespaces)
  *
  * 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,
+ * 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, was_variadic should be FALSE and
+ * 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 was_variadic, bool *use_variadic_p)
+                                          bool has_variadic, bool *use_variadic_p)
 {
        char       *result;
        HeapTuple       proctup;
@@ -8884,32 +8884,27 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
         * 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.
+        * 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)
        {
-               if (OidIsValid(procform->provariadic))
-               {
-                       if (procform->provariadic != ANYOID)
-                               use_variadic = true;
-                       else
-                               use_variadic = was_variadic;
-               }
-               else
-                       use_variadic = false;
+               /* 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(!was_variadic);
+               Assert(!has_variadic);
                use_variadic = false;
        }
 
index c7024b2225778ff469ee2892203b43eccfe91c34..1fe45da50ce2ce14c67921676dc9806da57e2463 100644 (file)
@@ -2449,6 +2449,8 @@ get_call_expr_arg_stable(Node *expr, int argnum)
  * 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)
index 3c74fa0a5150bf9cc2c1bada1d696f8dad1ef344..e1a04c88b20bf2a61b5d5a43a47e4dd9ee10ae75 100644 (file)
@@ -53,6 +53,6 @@
  */
 
 /*                                                     yyyymmddN */
-#define CATALOG_VERSION_NO     201403261
+#define CATALOG_VERSION_NO     201404031
 
 #endif
index 4992bc083371ce2f53c83ef619dfd9a230f5fba4..9cce60b33be02e8a13bd2de94e5e16e88c106f11 100644 (file)
@@ -257,7 +257,8 @@ typedef struct Aggref
        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 */
+       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 */
@@ -358,7 +359,8 @@ typedef struct FuncExpr
        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 */
+       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 */