Re: Eager aggregation, take 3

On Tue, 7 Oct 2025 at 23:57, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Mon, Oct 6, 2025 at 10:59 PM David Rowley <dgrowleyml(at)gmail(dot)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