Thread: Small clean up in nodeAgg.c

Much of nodeAgg.c does not really know the difference between the
aggregate's combine function and the aggregate's transition function.
This was done on purpose so that we can keep as much code the same as
possible between partial aggregate and finalize aggregate.

We can take this a bit further with the attached patch which managed a
net reduction of about 3 dozen lines of code.

3 files changed, 118 insertions(+), 155 deletions(-)

I also did some renaming to try and make it more clear about when
we're talking about aggtransfn and when we're just talking about the
transition function that's being used, which in the finalize aggregate
case will be the combine function.

I proposed this a few years ago in [1], but at the time we went with a
more minimal patch to fix the bug being reported there with plans to
come back and do a bit more once we branched.

I've rebased this and I'd like to propose this small cleanup for pg15.

The patch is basically making build_pertrans_for_aggref() oblivious to
if it's working with the aggtransfn or the aggcombinefn and all the
code that needs to treat them differently is moved up into
ExecInitAgg().  This allows us to just completely get rid of
build_aggregate_combinefn_expr() and just use
build_aggregate_transfn_expr() instead.

I feel this is worth doing as nodeAgg.c has grown quite a bit over the
years. Shrinking it down a bit and maybe making it a bit more readable
seems like a worthy goal.  Heikki took a big step forward towards that
goal in 0a2bc5d61e. This, arguably, helps a little more.

I've included Andres and Horiguchi-san because they were part of the
discussion on the original thread.

David

[1] https://www.postgresql.org/message-id/CAKJS1f-Qu2Q9g6Xfcf5dctg99oDkbV9LyW4Lym9C4L1vEHTN%3Dg%40mail.gmail.com

On Sat, 12 Jun 2021 at 23:03, David Rowley <dgrowleyml@gmail.com> wrote:
> I've rebased this and I'd like to propose this small cleanup for pg15.

Now that the pg15 branch is open, does anyone have any objections to this patch?

David

David Rowley <dgrowleyml@gmail.com> writes:
> Now that the pg15 branch is open, does anyone have any objections to this patch?

Just reading it over quickly, I noticed a comment mentioning
"aggcombinedfn" which I suppose should be "aggcombinefn".
No particular opinion on whether this is a net reduction
of logical complexity.

            regards, tom lane

On Thu, 1 Jul 2021 at 11:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Just reading it over quickly, I noticed a comment mentioning
> "aggcombinedfn" which I suppose should be "aggcombinefn".

Thanks. I've fixed that locally.

> No particular opinion on whether this is a net reduction
> of logical complexity.

I had another look over it and I think we do need to be more clear
about when we're talking about aggtransfn and aggcombinefn.  The
existing code uses variables name aggtransfn when the value stored
could be the value for the aggcombinefn.  Additionally, the other
change to remove the special case build_aggregate_combinefn_expr()
function seems good in a sense of reusing more code and reducing the
amount of code in that file.

Unless anyone thinks differently about this, I plan on pushing the
patch in the next day or so.

David

On Fri, 2 Jul 2021 at 22:30, David Rowley <dgrowleyml@gmail.com> wrote:
> Unless anyone thinks differently about this, I plan on pushing the
> patch in the next day or so.

Pushed (63b1af943)

David