Re: WITHIN GROUP patch

From: Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Atri Sharma <atri(dot)jiit(at)gmail(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Vik Fearing <vik(dot)fearing(at)dalibo(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Subject: Re: WITHIN GROUP patch
Date: 2013-12-06 21:30:58
Message-ID: 87bo0tg03t.fsf@news-spur.riddles.org.uk
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

>>>>> "Tom" == Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

>> pg_get_function_arguments()'s interface assumes that the caller is
>> providing the enclosing parens. Changing it would have meant
>> returning a result like 'float8) WITHIN GROUP (float8' which I
>> reckoned would have too much chance of breaking existing callers.

Tom> Well, as it stands, existing callers are broken a fortiori;

Not in most cases, because using the output of
pg_get_function_arguments works in all contexts except for
constructing the CREATE AGGREGATE statement (for example, a function
that uses the pg_get_function_arguments output to construct GRANTs
or ALTER OWNERs would still work).

And since constructing a correct CREATE AGGREGATE statement for an
ordered set function requires that the caller know about the
additional options to supply in the body, this does not seem like a
restriction.

Tom> Of course, if we define the right output as being just the
Tom> arguments according to pg_proc, it's fine.

The patch accepts the 'just the arguments according to pg_proc' as
input everywhere except in CREATE AGGREGATE, in addition to the
syntax with explicit WITHIN GROUP.

(With the exception of GRANT, as it happens, where since there is no
existing GRANT ON AGGREGATE variant and the patch doesn't add one,
only the pg_proc arguments form is accepted.)

Tom> But your point about the parens is a good one anyway, because
Tom> now that I think about it, what \da has traditionally printed is

Tom> pg_catalog | sum | bigint | integer | sum as bigint acro
Tom> ss all integer input values

Tom> and I see the patch makes it do this

Tom> pg_catalog | sum | bigint | (integer) | sum as bigint acro

Tom> which I cannot find to be an improvement, especially since \df
Tom> does not look like that. (As patched, the output of "\df ran*"
Tom> is positively schizophrenic.)

Since I don't particularly trust my own judgement on aesthetics, I used
the feedback I got from others when deciding what to do. Frankly I think
this one needs wider input than just you and me arguing over it.

--
Andrew (irc:RhodiumToad)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2013-12-06 21:31:17 Re: pg_stat_statements: calls under-estimation propagation
Previous Message Tom Lane 2013-12-06 21:27:37 Re: WITHIN GROUP patch