Re: POC: GROUP BY optimization - Mailing list pgsql-hackers

From Pavel Borisov
Subject Re: POC: GROUP BY optimization
Date
Msg-id CALT9ZEGp_9V+QwejwTjDKu8gTkCasCXzdqHg5m6VUbFd+DpWaw@mail.gmail.com
Whole thread Raw
In response to Re: POC: GROUP BY optimization  (Alexander Korotkov <aekorotkov@gmail.com>)
Responses Re: POC: GROUP BY optimization
List pgsql-hackers
Hi, Alexander!

On Sun, 2 Jun 2024 at 17:18, Alexander Korotkov <aekorotkov@gmail.com> wrote:
Hi!

On Sun, Jun 2, 2024 at 10:55 AM jian he <jian.universality@gmail.com> wrote:
>
> On Fri, May 31, 2024 at 8:12 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > I've revised some grammar including the sentence you've proposed.
> >
>
> -static List *groupclause_apply_groupingset(PlannerInfo *root, List *force);
> +static List *preprocess_groupclause(PlannerInfo *root, List *force);
>
> changing preprocess_groupclause the second argument
> from "force" to "gset" would be more intuitive, I think.

Probably, but my intention is to restore preprocess_groupclause() as
it was before 0452b461bc with minimal edits to support incremental
sort.  I'd rather avoid refactoring if this area for now.

> `elog(ERROR, "Order of group-by clauses doesn't correspond incoming
> sort order");`
>
> I think this error message makes people wonder what "incoming sort order" is.
> BTW, "correspond", generally people use  "correspond to".

Thank you.  On the second thought, I think it would be better to turn
this into an assertion like the checks before.

> I did some minor cosmetic changes, mainly changing foreach to foreach_node.
> Please check the attachment.

I would avoid refactoring of preprocess_groupclause() for the reason
described above.  But I picked the grammar fix for PlannerInfo's
comment.

Thank you for working on this patchset.

0001: Patch looks right

Comments:
 
Covert -> Convert
sets the uninitialized value of ec_sortref of the pathkey's EquivalenceClass -> sets the value of the pathkey's EquivalenceClass unless it's already initialized
wasn't set yet -> hasn't been set yet

0002: additional assert checking only. Looks right.

0003: pure renaming, looks good.

0004: Restores pre 0452b461bc state to preprocess_groupclause with removed partial_match fallback. Looks right. I haven't checked the comments provided they are restored from pre 0452b461bc state.

0005: Looks right.

Regards,
Pavel Borisov
Supabase

pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: Volatile write caches on macOS and Windows, redux
Next
From: Erik Wienhold
Date:
Subject: Re: pltcl crashes due to a syntax error