Inconsistent Behavior of GROUP BY ROLLUP in v17 vs master

On Tue, 23 Sept 2025 at 15:49, 邱宇航 <iamqyh@gmail.com> wrote:

I've noticed that two GROUP BY ROLLUP queries behave differently in v17
compared to master and REL_18_STABLE. The issue can be reproduced by
following SQL:

After some git bisect work, I traced the root cause:
- The first issue was introduced by commit f5050f79 (Mark expressions
nullable by grouping sets).
- The second issue stems from commit 67a54b9e (Allow pushdown of HAVING
clauses with grouping sets).

If you check the release notes and the commit message for f5050f795
you'll see that it does mention that wrong results could be returned.

What wasn't mentioned was that this wasn't fixed in prior versions.
The reason being is that the fix required changing the query tree
representation, which we can't change in the back branches due to
incompatibility with stored rules in existing databases. So, a change
in query results for certain queries here is expected.

David

On Tue, Sep 23, 2025 at 07:38:14PM +1200, David Rowley wrote:

On Tue, 23 Sept 2025 at 15:49, 邱宇航 <iamqyh@gmail.com> wrote:

I've noticed that two GROUP BY ROLLUP queries behave differently in v17
compared to master and REL_18_STABLE. The issue can be reproduced by
following SQL:

After some git bisect work, I traced the root cause:
- The first issue was introduced by commit f5050f79 (Mark expressions
nullable by grouping sets).
- The second issue stems from commit 67a54b9e (Allow pushdown of HAVING
clauses with grouping sets).

If you check the release notes and the commit message for f5050f795
you'll see that it does mention that wrong results could be returned.

What wasn't mentioned was that this wasn't fixed in prior versions.
The reason being is that the fix required changing the query tree
representation, which we can't change in the back branches due to
incompatibility with stored rules in existing databases. So, a change
in query results for certain queries here is expected.

Uh, by design, items mentioned in the major release notes have _not_
been fixed in previous minor versions. Not sure if we can make that
clearer to users. I did write a blog about this:

https://momjian.us/main/blogs/pgblog/2022.html#June_13_2022

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Do not let urgent matters crowd out time for investment in the future.

2025年9月23日 15:38，David Rowley <dgrowleyml@gmail.com> 写道：

If you check the release notes and the commit message for f5050f795
you'll see that it does mention that wrong results could be returned.

What wasn't mentioned was that this wasn't fixed in prior versions.
The reason being is that the fix required changing the query tree
representation, which we can't change in the back branches due to
incompatibility with stored rules in existing databases. So, a change
in query results for certain queries here is expected.

Yeah, I got it. Thanks David and Bruce.

And what about the query 2. This is caused by another commit, and
it's not mentioned in the commit message or the mailing discussion.

Best regards,
Yuhang Qiu

=?utf-8?B?6YKx5a6H6Iiq?= <iamqyh@gmail.com> writes:

And what about the query 2. This is caused by another commit, and
it's not mentioned in the commit message or the mailing discussion.

That one indeed seems quite broken. EXPLAIN confirms that it's
pushing the HAVING below the aggregation, which is simply wrong
because it fails to filter the all-null row(s) that the aggregation
node will create out of thin air.

Is there anything we can salvage from 67a54b9e, or should
we just revert it?

regards, tom lane

On Wed, 24 Sept 2025 at 02:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Is there anything we can salvage from 67a54b9e, or should
we just revert it?

It doesn't seem great that we need to reconsider the safety of that
optimisation post-release. It's not as if 67a54b9e added several cases
to test for and got one of them wrong. It's a case of the 1 case it
did add wasn't fully thought through.

As for fixing it; testing for a Const-false havingClause can't be done
as that only works for this case which const-folding happens to figure
out during planning. You could equally have something Var-less like:

create or replace function one() returns int as $$ begin return 1;
end; $$ stable language plpgsql;
SELECT 'XXX',count(*) FROM t GROUP BY ROLLUP(id) HAVING NOT one()=1;

which isn't known at plan-time. For me, I can't see how to make this
safe and I suspect due to your above question that you're in a similar
situation. Reverting and reconsidering for v19 seems like the safest
option.

Let's see what Richard thinks.

David

David Rowley <dgrowleyml@gmail.com> writes:

As for fixing it; testing for a Const-false havingClause can't be done
as that only works for this case which const-folding happens to figure
out during planning. You could equally have something Var-less like:

Yeah, I don't think the havingClause being constant-false is the
key point here. I've not looked closely at 67a54b9e, but I suspect
that anything Var-free is potentially an issue. Or it might even
fail for something that has Vars but is non-strict.

The core of the problem though is that the aggregation node will
issue an all-nulls row that did not come from its input, and we
have to be sure that the HAVING clause is properly evaluated
for that row.

regards, tom lane

On Wed, Sep 24, 2025 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah, I don't think the havingClause being constant-false is the
key point here. I've not looked closely at 67a54b9e, but I suspect
that anything Var-free is potentially an issue. Or it might even
fail for something that has Vars but is non-strict.

The core of the problem though is that the aggregation node will
issue an all-nulls row that did not come from its input, and we
have to be sure that the HAVING clause is properly evaluated
for that row.

I think the issue is that an empty grouping set can produce a row out
of nowhere, and pushing down HAVING clauses in this case may cause us
to fail to filter out that row.

Currently, non-variable-free HAVING clauses aren't pushed down when
there is an empty grouping set, because the empty grouping set would
nullify the vars referenced in those clauses, and we have logic in
place to prevent their pushdown.

However, we (I) overlooked variable-free HAVING clauses. If there are
only empty grouping sets, this is not a problem since a copy of the
HAVING clauses is retained in HAVING. The issue arises when there are
both nonempty and empty grouping sets.

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.

- Richard

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

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 5:18 PM Richard Guo <guofenglinux@gmail.com> wrote:

Here is the patch.

I plan to push this patch soon, unless there are any objections.

- Richard

On Thu, 25 Sept 2025 at 13:01, Richard Guo <guofenglinux@gmail.com> wrote:

I plan to push this patch soon, unless there are any objections.

What's your confidence levels on the logic now being correct? 100%?
90%? Hopeful?

I've not personally had the time to process the patch and figure out
that this is now safe. It sounds like you have, but (with respect) I
expect you also thought that before the initial commit too. I realise
that you now have more of the picture, but how do we know the picture
is now complete? I think we all know this stuff is hard and we also
know that even the most seasoned of planner hackers don't always get
it right first time.

What I'm now wondering is the risk to reward ratio of fixing this in
18.1. If it happens that it's still not right for 18.1, then we need
to wait until 18.2, which is currently due Feb-26. I don't quite have
the same confidence levels as you do, but maybe I would if I took the
time to more carefully think about this. For now, my thoughts are
that it's safer to revert and try again for v19. Probably it would
make more sense to try harder for an 18.1 fix if this optimisation was
a more critical and people had been waiting on it and there was no
other way to obtain the benefits of it. But that's not quite the case
here as, in theory, someone could rewrite their query if it's safe for
their use case and end up with the same performance benefits.

Just so it's clear, I'm not objecting to fixing for 18.1. I just want
to ensure your judgment is for the project and not for
self-preservation. I think most people will respect the decision if
you decide that you need more time to consider this and opt to revert
in v18 and fix only in master. Based on my current understanding of
the optimisation, I'd certainly be more at ease with that.

On the other hand, if you're 100% confident, I'll step aside.

David

David Rowley <dgrowleyml@gmail.com> writes:

On Thu, 25 Sept 2025 at 13:01, Richard Guo <guofenglinux@gmail.com> wrote:

I plan to push this patch soon, unless there are any objections.

What's your confidence levels on the logic now being correct? 100%?
90%? Hopeful?

FWIW, my confidence in it is rather low. I've not had time to think
this through carefully, but it seems to me that the test ought to
involve whether there is an empty grouping set, yet the proposed
patch does no such thing --- or at least, if it manages to achieve
that effect, it's not obvious how.

18.1 will not be coming out till November, so I feel no need to
rush to judgment on what to do here.

regards, tom lane

On Thu, Sep 25, 2025 at 11:05 AM David Rowley <dgrowleyml@gmail.com> wrote:

What's your confidence levels on the logic now being correct? 100%?
90%? Hopeful?

I've not personally had the time to process the patch and figure out
that this is now safe. It sounds like you have, but (with respect) I
expect you also thought that before the initial commit too. I realise
that you now have more of the picture, but how do we know the picture
is now complete? I think we all know this stuff is hard and we also
know that even the most seasoned of planner hackers don't always get
it right first time.

What I'm now wondering is the risk to reward ratio of fixing this in
18.1. If it happens that it's still not right for 18.1, then we need
to wait until 18.2, which is currently due Feb-26. I don't quite have
the same confidence levels as you do, but maybe I would if I took the
time to more carefully think about this. For now, my thoughts are
that it's safer to revert and try again for v19. Probably it would
make more sense to try harder for an 18.1 fix if this optimisation was
a more critical and people had been waiting on it and there was no
other way to obtain the benefits of it. But that's not quite the case
here as, in theory, someone could rewrite their query if it's safe for
their use case and end up with the same performance benefits.

Just so it's clear, I'm not objecting to fixing for 18.1. I just want
to ensure your judgment is for the project and not for
self-preservation. I think most people will respect the decision if
you decide that you need more time to consider this and opt to revert
in v18 and fix only in master. Based on my current understanding of
the optimisation, I'd certainly be more at ease with that.

On the other hand, if you're 100% confident, I'll step aside.

Thanks for the input; that's a reasonable concern. Although I've
reviewed my analysis again and didn't find any new issues, I don't
think I would claim to be 100% confident that the current logic is
bug-free. Realistically, I doubt anyone would make such a claim.

I'm completely open to reverting this and revisiting it for v19.
However, I'd really appreciate any reviews that point out specific
issues in the current logic. I've shared my analysis in my initial
email as well as in the commit message of the proposed patch. If
anyone can highlight parts that don't make sense, we can discuss them
and update the patch accordingly. Without that kind of feedback, I'm
concerned that we may just end up repeating the same code in v19.

- Richard

On Thu, Sep 25, 2025 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, my confidence in it is rather low. I've not had time to think
this through carefully, but it seems to me that the test ought to
involve whether there is an empty grouping set, yet the proposed
patch does no such thing --- or at least, if it manages to achieve
that effect, it's not obvious how.

I explained the test for empty grouping sets in my initial email as
well as in the commit message of the proposed patch. Apparently, I
failed to make myself clear, so here's another attempt.

To be clear, we are only discussing HAVING clauses that do not contain
aggregates, since clauses with aggregates wouldn't be pushed down
anyway.

First, let's consider the presence of empty grouping sets. There are
two cases: either the query contains only empty grouping sets, or it
contains both non-empty and empty grouping sets.

In the former case (only empty grouping sets), the HAVING clauses must
be degenerate (variable-free). These are placed in the WHERE clause
and also retained in HAVING. This follows the same logic as prior to
commit 67a54b9e8.

In the latter case (both non-empty and empty grouping sets), for
non-degenerate HAVING clauses, the empty grouping sets would nullify
the vars referenced in the clauses, so they would not be pushed
down. This is ensured by the check bms_is_member(root->group_rtindex,
having_relids). For degenerate HAVING clauses, they are also
not pushed down, guaranteed by the new check added in the proposed
patch: having_relids == NULL.

Next, suppose there are no empty grouping sets in the query. For
non-degenerate HAVING clauses, only those clauses that do not
reference columns nullable by the grouping sets are pushed down, which
is guaranteed by the same check: bms_is_member(root->group_rtindex,
having_relids). For degenerate HAVING clauses, the check
having_relids == NULL prevents pushing them down, though it could be
argued that in this case, they could be pushed down. This is what I
meant by (quoting from the commit message):

"
This could be done more precisely, for example by restricting the
prevention only to cases where empty grouping sets are also present,
but the added complexity does not seem justified.
"

In summary, under the current logic, HAVING clauses in queries with
grouping sets are pushed down only in the following two cases:

1) There are only empty grouping sets.

In this case, the HAVING clauses are pushed down, but they are also
retained in HAVING.

2) There are non-empty grouping sets and the HAVING clauses are
non-degenerate and they do not reference any columns nullable by the
grouping sets.

I believe both of the above cases are safe for pushing down HAVING
clauses. It could be argued that there may be more cases where we can
push down HAVING clauses with grouping sets, but for now, I'd prefer
to focusing on ensuring the current logic is safe.

Does this analysis make sense to you? Am I missing anything?

18.1 will not be coming out till November, so I feel no need to
rush to judgment on what to do here.

Thanks. I'll wait for any feedback on the patch itself before
deciding how to proceed. I'd appreciate it if a review could be done
before November.

- Richard

On Thu, Sep 25, 2025 at 5:27 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Thu, Sep 25, 2025 at 12:24 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

18.1 will not be coming out till November, so I feel no need to
rush to judgment on what to do here.

Thanks. I'll wait for any feedback on the patch itself before
deciding how to proceed. I'd appreciate it if a review could be done
before November.

Having heard nothing but crickets and not wanting to leave this until
the 11th hour before November, I'll plan to push the v1 patch next
week, unless there are any objections.

- Richard

Richard Guo <guofenglinux@gmail.com> writes:

Having heard nothing but crickets and not wanting to leave this until
the 11th hour before November, I'll plan to push the v1 patch next
week, unless there are any objections.

I started to look at this again, and now I'm thinking that there is
indeed an issue related to "Query 1". Recall that the test setup is

CREATE TABLE t(id int);
INSERT INTO t SELECT generate_series(1, 3);

If we do

regression=# SELECT id, 'XXX' FROM t GROUP BY ROLLUP (id, 1);

we get this, which seems correct:

id | ?column?
----+----------
| XXX
3 | XXX
2 | XXX
1 | XXX
3 | XXX
2 | XXX
1 | XXX
(7 rows)

But leave out the "id" output, and look what happens:

regression=# SELECT 'XXX' FROM t GROUP BY ROLLUP (id, 1);
?column?
----------

XXX
XXX
XXX

(7 rows)

How can that be correct?? Simplifying further to

regression=# SELECT 'XXX' FROM t GROUP BY ROLLUP (id);
?column?
----------
XXX
XXX
XXX
XXX
(4 rows)

restores sanity. I've not dug into the code, but these two examples
make it look like we think that 'XXX' is dependent on '1', which
surely it is not, most especially since it shouldn't vary depending
on whether "id" is included as an output column.

This behavior is the same as in v17, but that doesn't make it not
broken.

regards, tom lane

I wrote:

I started to look at this again, and now I'm thinking that there is
indeed an issue related to "Query 1".

Oh, scratch that, I see my mistake: I was thinking of "1" as a
constant, but actually we must be interpreting it per SQL92 rules
as a reference to output column 1. So that's why adding or dropping
the "id" output column changes the results of the grouping.

-ENOCAFFEINE ... sorry for the noise. I'll go back to studying
the HAVING issue.

regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:

Having heard nothing but crickets and not wanting to leave this until
the 11th hour before November, I'll plan to push the v1 patch next
week, unless there are any objections.

OK, I finally made some time to think this through, and here's
my thoughts:

The definition of HAVING is that it excludes grouped rows that don't
satisfy the HAVING condition. The idea behind the push-down-HAVING
optimization is that if we exclude all source rows that don't satisfy
the condition, then no groups that don't satisfy it will be formed, so
we don't have to re-check the HAVING condition after forming groups.

Grouping sets break this concept, which is why the pre-67a54b9e
planner code just punted on the optimization if there were grouping
sets. There are two reasons why they cause trouble:

1. If a variable doesn't appear in every grouping set, then it will
sometimes go to NULL during grouping, meaning that the HAVING clause
needs to test a different value than what is available at the scan
level. We did not have a good way to deal with that pre-v18, but now
we can check to see whether the clause contains any variables marked
as nullable by group_rtindex; if not, it should be safe to push down.

2. If there is an empty grouping set, it will give rise to a grouped
row whether there is any input or not. In this case we *must* keep
the HAVING condition at the top level, else it fails to filter that
row. (We might still choose to copy it to WHERE, however.) Note
that whether the HAVING condition is degenerate (variable-free) or
not does not enter into this directly; although if it has any Vars
they are certainly all nullable by group_rtindex, meaning that
checking for #1 accidentally keeps us out of trouble unless the
condition is degenerate.

67a54b9e accounted for #1 but not #2, which is why we have a bug.

The proposed patch tries to close the hole by checking whether
the condition is degenerate, but that feels subtly wrong to me:
what we ought to check is whether there is any empty grouping set.
As proposed, I think we miss optimization opportunities for
degenerate HAVING because we will not try the trick of copying
it to WHERE.

Also, I suspect that this change in 67a54b9e was wrong in detail:

            newHaving = lappend(newHaving, havingclause);
        }
-       else if (parse->groupClause && !parse->groupingSets)
+       else if (parse->groupClause)
        {
            Node       *whereclause;

because it allows us to apply "move to WHERE" rather than "copy to
WHERE" in cases with grouping sets, and that can't always be right.
I think that if that change hadn't been made, we would not have
had a bug. So the fact that the proposed fix doesn't touch this
bit does not seem right.

regards, tom lane

I wrote:

The proposed patch tries to close the hole by checking whether
the condition is degenerate, but that feels subtly wrong to me:
what we ought to check is whether there is any empty grouping set.
As proposed, I think we miss optimization opportunities for
degenerate HAVING because we will not try the trick of copying
it to WHERE.

Concretely, I think we could do the attached. This has the same
test query as in v1, but the generated plan is better because it
realizes it can copy the constant-false HAVING clause into WHERE,
resulting in a dummy scan of the table.

I'm not sure if planner.c is the best place to put
has_empty_grouping_set(). I couldn't find any existing code doing the
same thing, but maybe someday we'd want the functionality elsewhere.

regards, tom lane