Thread: Getting rid of aggregate_dummy()
While fooling with Gen_fmgrtab.pl for a nearby patch [1], 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].
regards, tom lane
[1] https://www.postgresql.org/message-id/472274.1604258384%40sss.pgh.pa.us
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 0cf1da6ebb..7664bb6285 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -620,7 +620,7 @@ AggregateCreate(const char *aggName,
GetUserId(), /* proowner */
INTERNALlanguageId, /* languageObjectId */
InvalidOid, /* no validator */
- "aggregate_dummy", /* placeholder proc */
+ "aggregate_dummy", /* placeholder (no such proc) */
NULL, /* probin */
PROKIND_AGGREGATE,
false, /* security invoker (currently not
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 75e5bbf209..d87677d659 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -4935,24 +4935,6 @@ AggRegisterCallback(FunctionCallInfo fcinfo,
}
-/*
- * aggregate_dummy - dummy execution routine for aggregate functions
- *
- * This function is listed as the implementation (prosrc field) of pg_proc
- * entries for aggregate functions. Its only purpose is to throw an error
- * if someone mistakenly executes such a function in the normal way.
- *
- * Perhaps someday we could assign real meaning to the prosrc field of
- * an aggregate?
- */
-Datum
-aggregate_dummy(PG_FUNCTION_ARGS)
-{
- elog(ERROR, "aggregate function %u called as normal function",
- fcinfo->flinfo->fn_oid);
- return (Datum) 0; /* keep compiler quiet */
-}
-
/* ----------------------------------------------------------------
* Parallel Query Support
* ----------------------------------------------------------------
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 4cc7da268d..de58df3d3f 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2446,11 +2446,6 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
perfuncstate->wfuncstate = wfuncstate;
perfuncstate->wfunc = wfunc;
perfuncstate->numArguments = list_length(wfuncstate->args);
-
- fmgr_info_cxt(wfunc->winfnoid, &perfuncstate->flinfo,
- econtext->ecxt_per_query_memory);
- fmgr_info_set_expr((Node *) wfunc, &perfuncstate->flinfo);
-
perfuncstate->winCollation = wfunc->inputcollid;
get_typlenbyval(wfunc->wintype,
@@ -2479,6 +2474,11 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winobj->argstates = wfuncstate->args;
winobj->localmem = NULL;
perfuncstate->winobj = winobj;
+
+ /* It's a real window function, so set up to call it. */
+ fmgr_info_cxt(wfunc->winfnoid, &perfuncstate->flinfo,
+ econtext->ecxt_per_query_memory);
+ fmgr_info_set_expr((Node *) wfunc, &perfuncstate->flinfo);
}
}
diff --git a/src/backend/utils/Gen_fmgrtab.pl b/src/backend/utils/Gen_fmgrtab.pl
index 1edd5195d7..34e27950e7 100644
--- a/src/backend/utils/Gen_fmgrtab.pl
+++ b/src/backend/utils/Gen_fmgrtab.pl
@@ -75,6 +75,7 @@ foreach my $row (@{ $catalog_data{pg_proc} })
oid => $bki_values{oid},
name => $bki_values{proname},
lang => $bki_values{prolang},
+ kind => $bki_values{prokind},
strict => $bki_values{proisstrict},
retset => $bki_values{proretset},
nargs => $bki_values{pronargs},
@@ -195,8 +196,10 @@ foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
$sqlname .= "_" . $s->{args} if ($proname_counts{ $s->{name} } > 1);
$sqlname =~ tr/ /_/;
print $ofh "#define F_" . uc $sqlname . " $s->{oid}\n";
- # We want only one extern per internal-language function
- if ($s->{lang} eq 'internal' && !$seenit{ $s->{prosrc} })
+ # We want only one extern per internal-language, non-aggregate function
+ if ( $s->{lang} eq 'internal'
+ && $s->{kind} ne 'a'
+ && !$seenit{ $s->{prosrc} })
{
$seenit{ $s->{prosrc} } = 1;
print $pfh "extern Datum $s->{prosrc}(PG_FUNCTION_ARGS);\n";
@@ -214,6 +217,8 @@ my $fmgr_count = 0;
foreach my $s (sort { $a->{oid} <=> $b->{oid} } @fmgr)
{
next if $s->{lang} ne 'internal';
+ # We do not need entries for aggregate functions
+ next if $s->{kind} eq 'a';
print $tfh ",\n" if ($fmgr_count > 0);
print $tfh
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