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)
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 |