Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit d359f71

Browse files
committed
Fix non-equivalence of VARIADIC and non-VARIADIC function call formats.
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 75b39e7 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.
1 parent 2186533 commit d359f71

File tree

5 files changed

+24
-13
lines changed

5 files changed

+24
-13
lines changed

doc/src/sgml/xfunc.sgml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3154,9 +3154,10 @@ CREATE OR REPLACE FUNCTION retcomposite(IN integer, IN integer,
31543154
is zero based. <function>get_call_result_type</> can also be used
31553155
as an alternative to <function>get_fn_expr_rettype</>.
31563156
There is also <function>get_fn_expr_variadic</>, which can be used to
3157-
find out whether the call contained an explicit <literal>VARIADIC</>
3158-
keyword. This is primarily useful for <literal>VARIADIC "any"</>
3159-
functions, as described below.
3157+
find out whether variadic arguments have been merged into an array.
3158+
This is primarily useful for <literal>VARIADIC "any"</> functions,
3159+
since such merging will always have occurred for variadic functions
3160+
taking ordinary array types.
31603161
</para>
31613162

31623163
<para>

src/backend/parser/parse_func.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -374,6 +374,11 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
374374
newa->location = exprLocation((Node *) vargs);
375375

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

379384
/* build the appropriate output structure */

src/backend/utils/adt/ruleutils.c

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8604,7 +8604,7 @@ generate_relation_name(Oid relid, List *namespaces)
86048604
*
86058605
* If we're dealing with a potentially variadic function (in practice, this
86068606
* means a FuncExpr and not some other way of calling the function), then
8607-
* was_variadic must specify whether VARIADIC appeared in the original call,
8607+
* was_variadic should specify whether variadic arguments have been merged,
86088608
* and *use_variadic_p will be set to indicate whether to print VARIADIC in
86098609
* the output. For non-FuncExpr cases, was_variadic should be FALSE and
86108610
* use_variadic_p can be NULL.
@@ -8639,14 +8639,16 @@ generate_function_name(Oid funcid, int nargs, List *argnames, Oid *argtypes,
86398639
* since it affects the lookup rules in func_get_detail().
86408640
*
86418641
* Currently, we always print VARIADIC if the function is variadic and
8642-
* takes a variadic type other than ANY. (In principle, if VARIADIC
8643-
* wasn't originally specified and the array actual argument is
8644-
* deconstructable, we could print the array elements separately and not
8645-
* print VARIADIC, thus more nearly reproducing the original input. For
8646-
* the moment that seems like too much complication for the benefit.)
8647-
* However, if the function takes VARIADIC ANY, then the parser didn't
8648-
* fold the arguments together into an array, so we must print VARIADIC if
8649-
* and only if it was used originally.
8642+
* takes a variadic type other than ANY. However, if the function takes
8643+
* VARIADIC ANY, then the parser didn't fold the arguments together into
8644+
* an array, so we must print VARIADIC if and only if it was used
8645+
* originally.
8646+
*
8647+
* Note: with the current definition of funcvariadic, we could just set
8648+
* use_variadic = was_variadic, which indeed is the solution adopted in
8649+
* 9.4. However, in rules/views stored before 9.3.5, funcvariadic will
8650+
* reflect the previous definition (was VARIADIC written in the call?).
8651+
* So in 9.3 we cannot trust it unless the function is VARIADIC ANY.
86508652
*/
86518653
if (use_variadic_p)
86528654
{

src/backend/utils/fmgr/fmgr.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2452,6 +2452,8 @@ get_call_expr_arg_stable(Node *expr, int argnum)
24522452
* Get the VARIADIC flag from the function invocation
24532453
*
24542454
* Returns false (the default assumption) if information is not available
2455+
*
2456+
* Note this is generally only of interest to VARIADIC ANY functions
24552457
*/
24562458
bool
24572459
get_fn_expr_variadic(FmgrInfo *flinfo)

src/include/nodes/primnodes.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,8 @@ typedef struct FuncExpr
346346
Oid funcid; /* PG_PROC OID of the function */
347347
Oid funcresulttype; /* PG_TYPE OID of result value */
348348
bool funcretset; /* true if function returns set */
349-
bool funcvariadic; /* true if VARIADIC was used in call */
349+
bool funcvariadic; /* true if variadic arguments have been
350+
* combined into an array last argument */
350351
CoercionForm funcformat; /* how to display this function call */
351352
Oid funccollid; /* OID of collation of result */
352353
Oid inputcollid; /* OID of collation that function should use */

0 commit comments

Comments
 (0)