| From: | Haibo Yan <tristan(dot)yim(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Cc: | Richard Guo <guofenglinux(at)gmail(dot)com>, shihao zhong <zhong950419(at)gmail(dot)com> |
| Subject: | Re: [PATCH] Fix grouping-set nulling of variable-free grouping expressions |
| Date: | 2026-10-10 17:03:48 |
| Message-ID: | CABXr29EQMOv1nhyLpkR_kPKcJJn4U29aC-NwDP5=tRRtMjbFxQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Oct 9, 2026 at 11:10 PM Haibo Yan <tristan(dot)yim(at)gmail(dot)com> wrote:
>
> Hi Hackers,
>
> I found a wrong-result issue with grouping sets when a grouping
> expression is reduced to a constant during planning.
>
> For example:
>
> CREATE TABLE t (a int NOT NULL);
> INSERT INTO t VALUES (1);
>
> SELECT CASE WHEN a IS NOT NULL THEN 'yes' ELSE 'no' END,
> GROUPING(a IS NOT NULL), count(*)
> FROM t
> GROUP BY ROLLUP (a IS NOT NULL)
> ORDER BY 2;
>
> PostgreSQL 18 returns:
>
> yes | 0 | 1
> no | 1 | 1
>
> PostgreSQL 19 instead returns:
>
> yes | 0 | 1
> yes | 1 | 1
>
> The grand-total row should contain 'no', since the grouping expression
> is NULL in that grouping set.
>
> This regression was exposed by commit e2debb64380, which allows
> "a IS NOT NULL" to be folded to true based on the column's NOT NULL
> constraint. The folding itself is valid for the grouping input.
>
> The underlying issue predates this change. For example:
>
> SELECT (1 + 1) * 10
> FROM generate_series(1, 10)
> GROUP BY ROLLUP (1 + 1);
>
> PostgreSQL 18 already returns 20 for the grand-total row, where the
> result should be NULL.
>
> The problem is that flatten_group_exprs() substitutes the preprocessed
> grouping expression for an RTE_GROUP reference. For variable-free
> expressions, mark_nullable_by_grouping() wraps the expression in a
> PlaceHolderVar, but creates a new PHV ID for each reference to the
> same grouping output.
>
> Consequently, setrefs may fail to match an upper reference to the
> actual grouping column. The expression is then evaluated separately,
> bypassing the NULL inserted for the corresponding grouping set.
>
> The attached patch caches one PHV ID per RTE_GROUP attribute in
> PlannerInfo, so references to the same grouping output share an ID.
> Different grouping attributes retain distinct IDs, even when their
> expressions are folded to the same constant.
>
> This fixes both the PostgreSQL 19 regression and the pre-existing
> variable-free grouping-expression issue, without restricting constant
> folding or changing the aggregate executor.
>
> The patch has been tested on master and REL_19_STABLE with assertions
> enabled. Core regression, isolation, and relevant TAP tests pass.
> Regression tests cover ROLLUP, CUBE, GROUPING SETS, HAVING, LATERAL,
> and distinct grouping expressions folded to identical constants.
>
> Given the wrong-result regression in PostgreSQL 19, I'd appreciate
> review of whether this fix is appropriate for REL_19_STABLE.
>
> The underlying issue also affects PostgreSQL 18. Backpatching there
> would require some branch-specific adaptation and testing.
>
> Comments and review are welcome.
>
> Thanks,
> Haibo
Hi,
Attached is v2, updated to current master.
There are no changes to the implementation or regression tests
from v1. The patch applies cleanly to both master and
REL_19_STABLE.
I also added the following contributor credit:
Reported-by: Shihao Zhong <zhong950419(at)gmail(dot)com>
The patch has been independently tested on both branches with
assertions enabled. Core regression (239/239), isolation
(134/134), and the relevant postgres_fdw tests all pass.
Thanks,
Haibo
| Attachment | Content-Type | Size |
|---|---|---|
| 0002-Fix-preexisting-grouping-expression-bug.patch | application/octet-stream | 8.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2026-10-10 17:15:27 | Re: Policy for Abandoned Extensions |
| Previous Message | Fujii Masao | 2026-10-10 17:02:14 | Re: REPACK: warn about skipping foreign partitions |