Horton

Re: Eager aggregation, take 3

David Rowley <dgrowleyml@gmail.com>

View thread View on postgr.es
From: David Rowley <dgrowleyml@gmail.com>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Tender Wang <tndrwang@gmail.com>, Paul George <p.a.george19@gmail.com>, Andy Fan <zhihuifan1213@163.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, pgsql-hackers@lists.postgresql.org, Matheus Alcantara <matheusssilv97@gmail.com>
Date: 2025-10-08T11:14:39Z
Lists: pgsql-hackers 
On Tue, 7 Oct 2025 at 23:57, Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Mon, Oct 6, 2025 at 10:59 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > 6. Shouldn't this be using lappend()?
> >
> >  agg_clause_list = list_append_unique(agg_clause_list, ac_info);
> >
> > I don't understand why ac_info could already be in the list. You've
> > just done: ac_info = makeNode(AggClauseInfo);
>
> A query can specify the same Aggref expressions multiple times in the
> target list.  Using lappend here can lead to duplicate partial Aggref
> nodes in the targetlist of a grouped path, which is what I want to
> avoid.

I was getting that mixed up with list_append_unique_ptr().

> > 9. In get_expression_sortgroupref(), a comment claims "We ignore child
> > members here.". I think that's outdated since ec_members no longer has
> > child members.
>
> I think that comment is used to explain why we only scan ec_members
> here.  Similar comments can be found in many other places, such as in
> equivclass.c:
>
>   /*
>    * Found our match.  Scan the other EC members and attempt to generate
>    * joinclauses.  Ignore children here.
>    */
>   foreach(lc2, cur_ec->ec_members)
>   {

I'd say that's also wrong. "Ignore" means not to pay attention to
something that's there. The child members are not there.

> > 11. The way you've written the header comments for typedef struct
> > RelAggInfo seems weird.  I've only ever seen extra details in the
> > header comment when the inline comments have been kept to a single
> > line. You're spanning multiple lines, so why have the out of line
> > comments in the header at all?

> I've also updated the comments within RelAggInfo to use one-line
> style.

The style I'd thought of had the comments on the same line as the
field. Something like struct EquivalenceClass.

>I wrapped the long queries in v24.

+-- Enable eager aggregation, which by default is disabled.
+SET enable_eager_aggregate TO on;

The above comment and command mismatch to my understanding from
looking at postgresql.conf.sample and guc_parameters.dat.

David

Commits

  1. Fix eager aggregation for semi/antijoin inner rels

  2. Cover additional errors and corner conditions in repack.c

  3. Fix volatile function evaluation in eager aggregation

  4. Fix collation handling for grouping keys in eager aggregation

  5. Rename apply_at to apply_agg_at for clarity

  6. Fix comment in eager_aggregate.sql

  7. Remove unnecessary include of "utils/fmgroids.h"

  8. Implement Eager Aggregation

  9. Allow negative aggtransspace to indicate unbounded state size

  10. Add macros for looping through a List without a ListCell.

  11. Account for the effect of lossy pages when costing bitmap scans.

  12. Fix a thinko in join_is_legal: when we decide we can implement a semijoin