On ons, 2012-04-04 at 18:59 -0400, Tom Lane wrote:
>
> >> Why not call it string_agg?
>
> > Here is a patch to do the renaming. As it stands, it fails the
> > opr_sanity regression test, because that complains that there are now
> > two aggregate functions string_agg with different number of arguments.
> > It seems to me that that test should really only complain if the common
> > argument types of the two aggregates are the same, correct?
>
> Uh, no. That test is there for good and sufficient reasons, as per its
> comment:
>
> -- Check that there are not aggregates with the same name and different
> -- numbers of arguments. While not technically wrong, we have a project policy
> -- to avoid this because it opens the door for confusion in connection with
> -- ORDER BY: novices frequently put the ORDER BY in the wrong place.
> -- See the fate of the single-argument form of string_agg() for history.
>
> The renaming you propose would only be acceptable to those who have
> forgotten that history. I haven't.
I had reviewed that thread very carefully, but I'm not sure it applies.
The issue was that we don't want aggregates with optional second
argument, because "novices" could confuse
agg(a, b ORDER BY c)
with
agg(a ORDER BY b, c) -- wrong
without the error being flagged.
But that doesn't apply if the first argument has different types.
Erroneously calling agg(textdatum ORDER BY textdatum, sthelse) will not
result in a call to agg(bytea). (Unless the textdatum is really an
unknown literal, but that would be silly.)
Nevertheless, the problem would now be that adding string_agg(bytea)
would effectively forbid adding string_agg(bytea, delim) in the future.
So making a two-argument string_agg(bytea, bytea) now seems like the
best solution anyway. (This applies independently of the function
renaming, actually.)
