Getting rid of aggregate_dummy() - Mailing list pgsql-hackers

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