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

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