Horton

Re: POC: GROUP BY optimization

Pavel Borisov <pashkin.elfe@gmail.com>

View thread
From: Pavel Borisov <pashkin.elfe@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: jian he <jian.universality@gmail.com>, Andrei Lepikhov <a.lepikhov@postgrespro.ru>, Tom Lane <tgl@sss.pgh.pa.us>, Richard Guo <guofenglinux@gmail.com>, vignesh C <vignesh21@gmail.com>, PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas.vondra@enterprisedb.com>, Teodor Sigaev <teodor@sigaev.ru>, David Rowley <dgrowleyml@gmail.com>, "a.rybakina" <a.rybakina@postgrespro.ru>
Date: 2024-06-03T15:54:53Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Restore preprocess_groupclause()

  2. Rename PathKeyInfo to GroupByOrdering

  3. Add invariants check to get_useful_group_keys_orderings()

  4. Fix asymmetry in setting EquivalenceClass.ec_sortref

  5. Multiple revisions to the GROUP BY reordering tests

  6. Get rid of pg_class usage in SJE regression tests

  7. Rename index "abc" in aggregates.sql

  8. Explore alternative orderings of group-by pathkeys during optimization.

  9. Generalize the common code of adding sort before processing of grouping

  10. Fix out-dated comment in preprocess_groupclause()

  11. Force parallelism in partition_aggregate

  12. Optimize order of GROUP BY keys

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