Getting rid of aggregate_dummy()
While fooling with Gen_fmgrtab.pl for a nearby patch [1]/messages/by-id/472274.1604258384@sss.pgh.pa.us, I noticed
that fmgrtab.c had a lot of entries pointing at aggregate_dummy,
which seemed rather useless. So I experimented with removing them.
It turns out that nodeWindowAgg.c is carelessly expecting them to be
there, because it does fmgr_info_cxt() on the target window function
even if it will never call it because it's a plain aggregate.
But that's pretty trivial to fix, just need to relocate that call.
With that, we don't actually need aggregate_dummy() to exist at
all, because it's never referenced. Having "aggregate_dummy"
as the prosrc value for an aggregate function is now just a
random convention; any other string would do as well. (We could
save a few bytes in pg_proc by choosing a shorter string, but
probably it's better to stick to the existing convention.)
Anyway, this saves about 3KB in fmgrtab.o, without any downside
that I can see. If someone accidentally called an aggregate as
a normal function, they'd now get a different error message,
namely "internal function "aggregate_dummy" is not in internal lookup
table" instead of "aggregate function NNN called as normal function".
That doesn't really seem like a problem.
The attached patch is a delta over the one in [1]/messages/by-id/472274.1604258384@sss.pgh.pa.us.
regards, tom lane
Attachments:
remove-aggregate_dummy.patchtext/x-diff; charset=us-ascii; name=remove-aggregate_dummy.patchDownload+13-26
On 01/11/2020 22:47, Tom Lane wrote:
With that, we don't actually need aggregate_dummy() to exist at
all, because it's never referenced. Having "aggregate_dummy"
as the prosrc value for an aggregate function is now just a
random convention; any other string would do as well. (We could
save a few bytes in pg_proc by choosing a shorter string, but
probably it's better to stick to the existing convention.)
NULL would seem like the natural value for that.
- Heikki
On Sun, 1 Nov 2020 at 15:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Anyway, this saves about 3KB in fmgrtab.o, without any downside
that I can see. If someone accidentally called an aggregate as
a normal function, they'd now get a different error message,
namely "internal function "aggregate_dummy" is not in internal lookup
table" instead of "aggregate function NNN called as normal function".
That doesn't really seem like a problem.
Speaking as somebody who sometimes does really dumb things, I don’t like
this change in error message. The current message clearly identifies the
problem; the new message makes it look like there is a bug in Postgres.
Isaac Morland <isaac.morland@gmail.com> writes:
On Sun, 1 Nov 2020 at 15:47, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Anyway, this saves about 3KB in fmgrtab.o, without any downside
that I can see. If someone accidentally called an aggregate as
a normal function, they'd now get a different error message,
namely "internal function "aggregate_dummy" is not in internal lookup
table" instead of "aggregate function NNN called as normal function".
That doesn't really seem like a problem.
Speaking as somebody who sometimes does really dumb things, I don’t like
this change in error message. The current message clearly identifies the
problem; the new message makes it look like there is a bug in Postgres.
Neither message would be reachable without (erroneous) C hacking,
so I don't quite buy that there's a problem.
regards, tom lane
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 01/11/2020 22:47, Tom Lane wrote:
With that, we don't actually need aggregate_dummy() to exist at
all, because it's never referenced. Having "aggregate_dummy"
as the prosrc value for an aggregate function is now just a
random convention; any other string would do as well. (We could
save a few bytes in pg_proc by choosing a shorter string, but
probably it's better to stick to the existing convention.)
NULL would seem like the natural value for that.
I wouldn't be in favor of that unless we changed the prolang value
as well. Which could certainly be considered, but it makes the
patch rather more invasive, and I'm not sure it's worth it.
regards, tom lane
On Mon, 2 Nov 2020 at 09:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Isaac Morland <isaac.morland@gmail.com> writes:
Speaking as somebody who sometimes does really dumb things, I don’t like
this change in error message. The current message clearly identifies the
problem; the new message makes it look like there is a bug in Postgres.Neither message would be reachable without (erroneous) C hacking,
so I don't quite buy that there's a problem.
OK, I must have misunderstood. I had the impression that we were talking
about just writing a query which used an aggregate function where a normal
function was needed, but on closer consideration I see I had it wrong. For
example:
odyssey=> select * from uw_term where count(*) = 1;
ERROR: aggregate functions are not allowed in WHERE
LINE 1: select * from uw_term where count(*) = 1;
^
odyssey=>
But this is a different error message, and thinking about it putting an
aggregate in the SELECT will end up using it as an aggregate (e.g. SELECT
count(*) FROM ...).
I agree that C hackers need to know what they’re doing ;-)
I wrote:
Heikki Linnakangas <hlinnaka@iki.fi> writes:
On 01/11/2020 22:47, Tom Lane wrote:
With that, we don't actually need aggregate_dummy() to exist at
all, because it's never referenced. Having "aggregate_dummy"
as the prosrc value for an aggregate function is now just a
random convention; any other string would do as well. (We could
save a few bytes in pg_proc by choosing a shorter string, but
probably it's better to stick to the existing convention.)
NULL would seem like the natural value for that.
I wouldn't be in favor of that unless we changed the prolang value
as well. Which could certainly be considered, but it makes the
patch rather more invasive, and I'm not sure it's worth it.
Looking closer, I see that pg_proc.prosrc is marked NOT NULL,
so this couldn't work anyway unless we wish to remove that marking.
Which doesn't seem particularly wise. I pushed this without any
change in the catalog contents.
regards, tom lane