Re: Variadic aggregates vs. project policy

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> On 2013-08-29 18:29:34 -0400, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>> I'd say we let the check in there but have a list of exceptions in it so
>>> that one has to explicitly think about the issue before adding the
>>> function.

>> That's pretty much how the tests in opr_sanity work now.

> I basically mean that we should adapt the paragraph you quoted upthread
> to roughly say something like:

Attached is a complete draft patch for this. I modified the text about
the opr_sanity test a bit (though not exactly as you suggest) and also
added some text in xaggr.sgml pointing out the hazard, so that users can
make informed decisions about whether they want to use the feature.

My basic approach was to share the existing grammar and catalog support
for declaring variadic function arguments. That made it practically free
to add support for storing argument names for aggregates, so I did that
too. However, it was and remains true that aggregates don't support
default values for arguments, nor do we support writing calls in named or
mixed parameter style for them. The same is true of window functions.
I felt that fixing those things was out of scope for this patch, but it
might be something somebody else wants to work on. (I think that fixing
this might be relatively easy for window functions --- just need to get
the planner to apply its existing code for fixing up regular FuncExprs.
But it's less than trivial for aggregates because of our use of
TargetEntry list representation for aggregate parameter lists.)
Addition of default values for aggregates would run up against the same
number-of-arguments ambiguity we were discussing for VARIADIC, but I don't
see why we shouldn't adopt the same we-won't-do-this-but-users-can stance
as for VARIADIC.

Another point is that variadic window functions could stand some love.
I think it partially works, but ruleutils.c at least doesn't know how to
reverse-list such calls correctly. Again, that seemed out of scope for
this patch.

Comments?

regards, tom lane