Re: Eager aggregation, take 3

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