Horton

Re: Inconsistent Behavior of GROUP BY ROLLUP in v17 vs master

wenhui qiu <qiuwenhuifx@gmail.com>

View thread
From: wenhui qiu <qiuwenhuifx@gmail.com>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, David Rowley <dgrowleyml@gmail.com>, 邱宇航 <iamqyh@gmail.com>, Bruce Momjian <bruce@momjian.us>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2025-09-24T09:19:41Z
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. Fix test case from 40c242830

  2. Fix pushdown of degenerate HAVING clauses

  3. Allow pushdown of HAVING clauses with grouping sets

  4. Mark expressions nullable by grouping sets

Hi Richard
 > I think the following change fixes this problem.
>
>     foreach(l, (List *) parse->havingQual)
>     {
>         Node       *havingclause = (Node *) lfirst(l);
> +       Relids     having_relids;
>
>         if (contain_agg_clause(havingclause) ||
>             contain_volatile_functions(havingclause) ||
>             contain_subplans(havingclause) ||
>             (parse->groupClause && parse->groupingSets &&
> -            bms_is_member(root->group_rtindex, pull_varnos(root,
> havingclause))))
> +            ((having_relids = pull_varnos(root, havingclause)) == NULL ||
> +            bms_is_member(root->group_rtindex, having_relids))))
>         {
>             /* keep it in HAVING */
>             newHaving = lappend(newHaving, havingclause);
>
> This change essentially prevents variable-free HAVING clauses from
> being pushed down when there are any nonempty grouping sets.  Of
> course, this could be done more precisely -- for example, we could
> restrict the prevention only to cases where empty grouping sets are
> also present, but I'm not sure it's worth the effort.
I think it makes sense. However, it is now too close to the release date of
v18.Awaiting Tom's feedback？


Thanks

On Wed, Sep 24, 2025 at 4:18 PM Richard Guo <guofenglinux@gmail.com> wrote:

> On Wed, Sep 24, 2025 at 12:10 PM Richard Guo <guofenglinux@gmail.com>
> wrote:
> > In summary, the problem occurs when both of the following conditions
> > are met: 1) there are both nonempty and empty grouping sets, 2) there
> > are variable-free HAVING clauses.
> >
> > I think the following change fixes this problem.
> >
> >     foreach(l, (List *) parse->havingQual)
> >     {
> >         Node       *havingclause = (Node *) lfirst(l);
> > +       Relids     having_relids;
> >
> >         if (contain_agg_clause(havingclause) ||
> >             contain_volatile_functions(havingclause) ||
> >             contain_subplans(havingclause) ||
> >             (parse->groupClause && parse->groupingSets &&
> > -            bms_is_member(root->group_rtindex, pull_varnos(root,
> > havingclause))))
> > +            ((having_relids = pull_varnos(root, havingclause)) == NULL
> ||
> > +            bms_is_member(root->group_rtindex, having_relids))))
> >         {
> >             /* keep it in HAVING */
> >             newHaving = lappend(newHaving, havingclause);
> >
> > This change essentially prevents variable-free HAVING clauses from
> > being pushed down when there are any nonempty grouping sets.  Of
> > course, this could be done more precisely -- for example, we could
> > restrict the prevention only to cases where empty grouping sets are
> > also present, but I'm not sure it's worth the effort.
>
> Here is the patch.
>
> - Richard
>