Re: get_fn_expr_variadic considered harmful

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