Re: BUG #19752: GROUP BY on a constant and a cast to a length-limited array type crashes the server while planning

пт, 23 окт. 2026 г. в 20:34, Matheus Alcantara <matheusssilv97(at)gmail(dot)com>:

> On Thu Oct 8, 2026 at 6:21 PM -03, Andrey Rachitskiy wrote:
> > пт, 9 окт. 2026 г. в 01:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> >> > Skipping every GROUP BY item that contains no level-zero Var is too
> >> > broad. The item in that query is a Const, so it contains no Var, and
> >> > the commit exists to keep it a candidate. The constants that must not
> >> > be replaced are the other Const nodes that happen to compare equal.
> >>
> >> We might need to special-case TLEs that are identified this way.
> >> The basic problem is how do you tell which Consts are okay to replace,
> >> and what I'm saying is "none of them, except the identified TLE".
> >>
> >>
> > Agreed.
> >
> > None of the Consts should be replaced, except the targetlist
> > entry that GROUP BY identified.
> >
> > That entry already carries a ressortgroupref. When its expression is a
> > Const or Param, substitute_grouped_columns_mutator() replaces the entry
> > as a whole. The match is on ressortgroupref, because groupClauses may
> > be a copy after join alias flattening. These items are skipped in the
> > equal() loop, so the same value elsewhere stays a constant. Param
> > takes the same path as Const, since the existing code accepts the two
> > together.
> >
> > select 1 as one group by rollup(one) order by one nulls first still
> > returns a null row and then 1.
> > select 1 as one, 1+2 as three group by rollup(one) leaves three as 3.
> >
> > The reported query no longer crashes.
> >
> > GROUP BY true identified the boolean constant, and the true inside
> > ArrayCoerceExpr.elemexpr was a different Const.
> >
> > Patch attached.
>
> Hi,
>
> Thanks for working on this!
>
> I've reviewed the attached patch and I have a few comments:
>
> 1. It seems to me that the test group by cast(500 as int) exercises
> nothing. This test pass on master and on 18. I think that it's because
> the typmod for varchar(500) is 504, so changing to CAST(504 as int)
> trigger the issue.
>
> 2. Three of the new tests don't use grouping sets, so I'm wondering if
> aggregates.sql may be a better home for them.
>
> 3. I think that the check on substitute_grouped_columns_mutator can be
> written as:
>
> if (context->sublevels_up == 0 &&
> context->have_non_var_grouping &&
> IsA(node, TargetEntry) &&
> ((TargetEntry *) node)->ressortgroupref != 0)
> {
> TargetEntry *tle = (TargetEntry *) node;
> int attnum = 0;
>
> foreach(gl, context->groupClauses)
> {
> TargetEntry *grouptle = (TargetEntry *) lfirst(gl);
>
> attnum++;
> if (tle->ressortgroupref == grouptle->ressortgroupref &&
> (IsA(grouptle->expr, Const) || IsA(grouptle->expr, Param)))
> ...
>
> Because most targetlist entries aren't GROUP BY, ORDER BY or DISTINCT
> keys, so their ressortgroupref is 0. For those, the loop can never
> match, but it still walks the whole groupClauses list. The
> grouptle->ressortgroupref != 0 test inside the loop seems also redundant
> since an item in groupClauses always has a nonzero ressortgroupref,
> because that's how it was looked up. What it guards against is
> tle->ressortgroupref being 0, and that can be checked once, outside the
> loop.
>
> 4. I think that the patch only covers part of what Tom suggested. IIUC
> his first suggestion was that GROUP BY items should not be candidates
> for the search-and-replace unless they contain at least one level-zero
> Var, and then "none of them, except the identified TLE". The patch
> applies this only to plain Const and Param nodes, so other Var-free
> items like 1+2, now(), an uncorrelated sublink or an outer reference
> still go through the equal() matching. For example, with:
>
> select 1+2 as three, (1+2)*10 as x group by rollup(three);
>
> the 1+2 inside x is still replaced by a nullable grouped Var on the
> parse tree. The result is correct today only because the planner happens
> to not null it, which IIUC is the "it does not, right now" case that Tom
> mentioned.
>
> The crash itself can't come back through these items, since elemexpr
> only contains CaseTestExpr and Consts, but it seems to me that the rule
> should be based on what the item contains rather than on its node type.
>
> Based on this I'm attaching a v2 that implements Tom's suggestion. It's
> based on the same idea of matching the identified TLE by ressortgroupref
> from your patch. The main changes are:
>
> - parseCheckAggregates() records in a Bitmapset which GROUP BY items
> contain no Vars of the current query level, using
> contain_vars_of_level(expr, 0).
>
> - For those items, only the targetlist entry that GROUP BY identified
> is replaced as a whole, and they are skipped in the equal() loop at
> every sublevel.
>
> - Since a Const or Param can no longer match any grouping item, the
> early return for them is moved back to the top of the mutator, as it
> was before f5050f795ae.
>
> --
> Matheus Alcantara
> EDB: https://www.enterprisedb.com

Hi Matheus,

Thanks for the review and the v2. You understood Tom better than I did. v3
keeps the same code.
The queries that do not use grouping sets moved to aggregates.sql.

--
Regards,
Rachitskiy Andrey