Thread: get_fn_expr_variadic considered harmful
In bug #9817 there's a complaint that the planner fails to consider
these expressions equivalent: foo('a'::text, 'b'::text) foo(variadic array['a'::text, 'b'::text])
when foo() is declared as taking variadic text[].
Such cases worked okay before 9.3, the reason being that the use of
VARIADIC disappeared after parsing, so that the two calls were in fact
identical. However, once we added FuncExpr.funcvariadic, they're not
identical anymore.
I thought for a bit that we could fix this easily by having equal()
disregard funcvariadic; there is precedent for that, since it ignores
other fields that are just for display purposes and have no semantic
impact, such as CoercionForm and location.
Unfortunately, funcvariadic *does* have semantic impact on a few
functions that use get_fn_expr_variadic, such as format(). Since
the planner has no good way to know which ones those are, it cannot
safely ignore funcvariadic while matching expressions.
In short, commit 75b39e790 broke this rather badly, and I don't see
any easy way out.
We could possibly salvage something by redefining funcvariadic as only
being true if VARIADIC was used *and* the function is VARIADIC ANY,
so that it returns to not being different for semantically-equivalent
cases. This would be a bit messy, since it would not un-break the
behavior for any already stored rules or indexes in 9.3 databases.
But I'm not sure there is any good way to make the problem magically
go away in 9.3 databases.
Thoughts?
regards, tom lane
On Tue, Apr 1, 2014 at 12:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In bug #9817 there's a complaint that the planner fails to consider
> these expressions equivalent:
> foo('a'::text, 'b'::text)
> foo(variadic array['a'::text, 'b'::text])
> when foo() is declared as taking variadic text[].
My first reaction to this was "who cares? after all, the user should
just write the expression the same way both times and then they won't
have this problem". But after going and looking at the bug report I
see that the user wrote it the first way consistently, but pg_dump
blithely rewrote it to the second way. I'm disinclined to view that
as a planner problem; it seems to me to be a pg_dump or ruleutils bug.If those two things don't have the same parse
representation,then
pg_dump has no business treating them as equivalent - even if we were
to put enough smarts into the planner to paper over that
non-equivalence.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 1, 2014 at 12:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In bug #9817 there's a complaint that the planner fails to consider
>> these expressions equivalent:
>> foo('a'::text, 'b'::text)
>> foo(variadic array['a'::text, 'b'::text])
>> when foo() is declared as taking variadic text[].
> My first reaction to this was "who cares? after all, the user should
> just write the expression the same way both times and then they won't
> have this problem". But after going and looking at the bug report I
> see that the user wrote it the first way consistently, but pg_dump
> blithely rewrote it to the second way. I'm disinclined to view that
> as a planner problem; it seems to me to be a pg_dump or ruleutils bug.
> If those two things don't have the same parse representation, then
> pg_dump has no business treating them as equivalent - even if we were
> to put enough smarts into the planner to paper over that
> non-equivalence.
The point is that they *were* equivalent before 9.3, and so ruleutils
was entirely within its rights to not worry about which way it dumped
the expression; indeed, it couldn't, because the information was not
there as to which way the call had been written originally. I do not
think it's appropriate to blame ruleutils for taking advantage of this
equivalence, because more than likely user applications have too.
Or in other words, what I wrote above is a more general statement of the
problem than what was complained of in bug #9817 ... but if we just hack
ruleutils to dump the cases differently, we will fail to fix the more
general problem. So we can still expect future bug reports about that,
because it worked as-expected for years before 9.3.
There's also the point that even if we changed ruleutils' behavior
now, this would not fix existing dump files that have considered the
two forms interchangeable ever since VARIADIC existed. And we
generally try hard to not break existing dump files. To be even
more to the point: what you propose is incapable of fixing the precise
problem stated in the bug report, because it's complaining about a
dump taken from 9.1, and there is *no* way to make 9.1 produce a
dump that only uses VARIADIC if the original call did. It hasn't
got the information. Even using a newer version of pg_dump wouldn't
help that.
regards, tom lane
On 2014-04-01 12:52:54 -0400, Tom Lane wrote: > We could possibly salvage something by redefining funcvariadic as only > being true if VARIADIC was used *and* the function is VARIADIC ANY, > so that it returns to not being different for semantically-equivalent > cases. This would be a bit messy, since it would not un-break the > behavior for any already stored rules or indexes in 9.3 databases. > But I'm not sure there is any good way to make the problem magically > go away in 9.3 databases. It's pretty damn ugly, but if we're going for magic in around those edges, we could just force the new behaviour in readfuncs.c. IIUC all the neccessary data for it is there. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-04-01 12:52:54 -0400, Tom Lane wrote:
>> We could possibly salvage something by redefining funcvariadic as only
>> being true if VARIADIC was used *and* the function is VARIADIC ANY,
>> so that it returns to not being different for semantically-equivalent
>> cases. This would be a bit messy, since it would not un-break the
>> behavior for any already stored rules or indexes in 9.3 databases.
>> But I'm not sure there is any good way to make the problem magically
>> go away in 9.3 databases.
> It's pretty damn ugly, but if we're going for magic in around those
> edges, we could just force the new behaviour in readfuncs.c. IIUC all
> the neccessary data for it is there.
I don't want either readfuncs or equalfuncs going in for catalog lookups,
which is what they'd have to do to fix it at that level (the key point
being they'd have to find out whether the called function is declared as
VARIADIC ANY). Too much risk of unpleasant side effects if we do that.
The parser, on the other hand, has ready access to the function's
parameter list when building a FuncExpr.
regards, tom lane
On Tue, Apr 1, 2014 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Tue, Apr 1, 2014 at 12:52 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> In bug #9817 there's a complaint that the planner fails to consider
>>> these expressions equivalent:
>>> foo('a'::text, 'b'::text)
>>> foo(variadic array['a'::text, 'b'::text])
>>> when foo() is declared as taking variadic text[].
>
>> My first reaction to this was "who cares? after all, the user should
>> just write the expression the same way both times and then they won't
>> have this problem". But after going and looking at the bug report I
>> see that the user wrote it the first way consistently, but pg_dump
>> blithely rewrote it to the second way. I'm disinclined to view that
>> as a planner problem; it seems to me to be a pg_dump or ruleutils bug.
>> If those two things don't have the same parse representation, then
>> pg_dump has no business treating them as equivalent - even if we were
>> to put enough smarts into the planner to paper over that
>> non-equivalence.
>
> The point is that they *were* equivalent before 9.3, and so ruleutils
> was entirely within its rights to not worry about which way it dumped
> the expression; indeed, it couldn't, because the information was not
> there as to which way the call had been written originally. I do not
> think it's appropriate to blame ruleutils for taking advantage of this
> equivalence, because more than likely user applications have too.
Sure, it was reasonable at the time, certainly. But they're not
equivalent any more. Either they've got to become equivalent again,
or you can't dump one as the other. I'm happy with either one, but
the first rule of correct dumping is that what you dump out must, on
reload, produce an equivalent database.
> Or in other words, what I wrote above is a more general statement of the
> problem than what was complained of in bug #9817 ... but if we just hack
> ruleutils to dump the cases differently, we will fail to fix the more
> general problem. So we can still expect future bug reports about that,
> because it worked as-expected for years before 9.3.
>
> There's also the point that even if we changed ruleutils' behavior
> now, this would not fix existing dump files that have considered the
> two forms interchangeable ever since VARIADIC existed. And we
> generally try hard to not break existing dump files. To be even
> more to the point: what you propose is incapable of fixing the precise
> problem stated in the bug report, because it's complaining about a
> dump taken from 9.1, and there is *no* way to make 9.1 produce a
> dump that only uses VARIADIC if the original call did. It hasn't
> got the information. Even using a newer version of pg_dump wouldn't
> help that.
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Apr 1, 2014 at 2:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> There's also the point that even if we changed ruleutils' behavior
>> now, this would not fix existing dump files that have considered the
>> two forms interchangeable ever since VARIADIC existed. And we
>> generally try hard to not break existing dump files. To be even
>> more to the point: what you propose is incapable of fixing the precise
>> problem stated in the bug report, because it's complaining about a
>> dump taken from 9.1, and there is *no* way to make 9.1 produce a
>> dump that only uses VARIADIC if the original call did. It hasn't
>> got the information. Even using a newer version of pg_dump wouldn't
>> help that.
> 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.
regards, tom lane
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 */