Обсуждение: Fix HAVING-to-WHERE pushdown with nondeterministic collations

As briefly discussed on Discord, when a GROUP BY clause uses a
nondeterministic collation, the planner's optimization of moving
HAVING clauses to WHERE can produce incorrect results if the HAVING
clause applies a stricter collation.

CREATE TABLE t (x TEXT COLLATE case_insensitive);
INSERT INTO t VALUES ('a'), ('A');

SELECT x, count(*) FROM t GROUP BY x HAVING x = 'a' COLLATE "C";

This returns count=1, but should return count=2.

The attached draft patch fixes this for HEAD by leveraging GROUP Vars
(Vars referencing RTE_GROUP) to detect collation conflicts on a
per-clause basis, so only unsafe clauses are kept in HAVING while safe
ones are still pushed.  Please see the commit message for more
details.

For versions prior to v18, we do not have GROUP Vars.  I wonder if we
can take a conservative approach: skipping the HAVING-to-WHERE
pushdown optimization entirely if any GROUP BY expression uses a
nondeterministic collation.

Thoughts and reviews are welcome.

- Richard

On Wed, Apr 1, 2026 at 11:19 AM wenhui qiu <qiuwenhuifx@gmail.com> wrote:
> > + vars = pull_var_clause(node, PVC_RECURSE_PLACEHOLDERS);
> > +
> > + foreach_node(Var, var, vars)
> > + {
> > + if (var->varno == *group_rtindex &&
> > + OidIsValid(var->varcollid) &&
> > + var->varcollid != inputcollid &&
> > + !get_collation_isdeterministic(var->varcollid))
> > + {
> > + list_free(vars);
> > + return true;
> > + }
> > + }
> > +
> > + list_free(vars);

> This might be overthinking, but I wonder if calling pull_var_clause() at each walker step could introduce some
overheaddue to repeated subtree scans 

That's a good point, but I doubt that it'd be an issue in practice.
HAVING clauses are typically very small expressions.  Even in unusual
queries, the clause size is bounded by what a human writes, which is
negligible compared to the work the planner does elsewhere.

Maybe we can avoid this by calling pull_var_clause once at the top of
each clause and reusing that var list at every node.  But that can
introduce false positives.  The pre-pulled list contains all GROUP
Vars from the entire clause, but a given operator node only acts on
the vars in its own subtree.

- Richard

On Tue, Mar 31, 2026 at 12:41 PM Richard Guo <guofenglinux@gmail.com> wrote:
> The attached draft patch fixes this for HEAD by leveraging GROUP Vars
> (Vars referencing RTE_GROUP) to detect collation conflicts on a
> per-clause basis, so only unsafe clauses are kept in HAVING while safe
> ones are still pushed.  Please see the commit message for more
> details.

I noticed a bug in this patch.  The pull_var_clause call in
having_collation_conflict_walker needs to recurse through Aggrefs,
since they can still be present in havingQual at this point and we
need to look through them to reach any GROUP Vars in their direct
arguments.  v2 attached fixes this.

> For versions prior to v18, we do not have GROUP Vars.  I wonder if we
> can take a conservative approach: skipping the HAVING-to-WHERE
> pushdown optimization entirely if any GROUP BY expression uses a
> nondeterministic collation.

I'm afraid this approach would regress performance for queries that
currently benefit from the optimization.  But a proper pre-v18 fix
would require a different approach from the v18+ one, since GROUP Vars
don't exist on earlier branches.  Given the absence of field reports,
I don't think the risk of carrying a different fix on stable branches
is justified.  So I'm inclined to back-patch this fix to v18 only.

Any thoughts?

- Richard

On Thu, Apr 2, 2026 at 7:11 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Apr 1, 2026 at 11:19 AM wenhui qiu <qiuwenhuifx@gmail.com> wrote:
> > > + vars = pull_var_clause(node, PVC_RECURSE_PLACEHOLDERS);
> > > +
> > > + foreach_node(Var, var, vars)
> > > + {
> > > + if (var->varno == *group_rtindex &&
> > > + OidIsValid(var->varcollid) &&
> > > + var->varcollid != inputcollid &&
> > > + !get_collation_isdeterministic(var->varcollid))
> > > + {
> > > + list_free(vars);
> > > + return true;
> > > + }
> > > + }
> > > +
> > > + list_free(vars);
>
> > This might be overthinking, but I wonder if calling pull_var_clause() at each walker step could introduce some
overheaddue to repeated subtree scans 

> That's a good point, but I doubt that it'd be an issue in practice.
> HAVING clauses are typically very small expressions.  Even in unusual
> queries, the clause size is bounded by what a human writes, which is
> negligible compared to the work the planner does elsewhere.

I was about to push the v2 patch, but I just can't shake off the
concern Wenhui Qiu raised about the repeated subtree scan.  I still
don't have a concrete real-world case where a query has a large enough
HAVING clause for it to matter, but let's just be paranoid.

I think we can fix it easily.  The current walker calls
pull_var_clause() at every collation-aware node, which re-walks the
subtree.  The fix is to flip it inside out: walk top-down, push
inputcollids onto a LIFO stack, and at each GROUP Var check against
the stack.  This way, we only need to walk the expression tree once.
Attached v3 does this.

v3 also fixes the RowCompareExpr case.  Unlike the node types covered
by exprInputCollation(), RowCompareExpr carries per-column
inputcollids[] rather than a single inputcollid, so we need to descend
into each (largs[i], rargs[i]) pair with the matching collation pushed
onto the stack.  Without this, a HAVING clause like:

  HAVING ROW(x, 1) < ROW('ABC' COLLATE case_sensitive, 1)

over a case_insensitive group would give wrong results.

- Richard

On Thu, Apr 30, 2026 at 12:08 PM Richard Guo <guofenglinux@gmail.com> wrote:
> I was about to push the v2 patch, but I just can't shake off the
> concern Wenhui Qiu raised about the repeated subtree scan.  I still
> don't have a concrete real-world case where a query has a large enough
> HAVING clause for it to matter, but let's just be paranoid.
>
> I think we can fix it easily.  The current walker calls
> pull_var_clause() at every collation-aware node, which re-walks the
> subtree.  The fix is to flip it inside out: walk top-down, push
> inputcollids onto a LIFO stack, and at each GROUP Var check against
> the stack.  This way, we only need to walk the expression tree once.
> Attached v3 does this.
>
> v3 also fixes the RowCompareExpr case.  Unlike the node types covered
> by exprInputCollation(), RowCompareExpr carries per-column
> inputcollids[] rather than a single inputcollid, so we need to descend
> into each (largs[i], rargs[i]) pair with the matching collation pushed
> onto the stack.  Without this, a HAVING clause like:
>
>   HAVING ROW(x, 1) < ROW('ABC' COLLATE case_sensitive, 1)
>
> over a case_insensitive group would give wrong results.

I've committed this and back-patched it to v18.  I was not
back-patching further because pre-v18 branches would need a very
different and more complex fix due to the lack of the RTE_GROUP
mechanism.  I think it's too risky, and doesn't seem justified given
the absence of field reports.

- Richard

On Wed, May 6, 2026 at 9:58 AM SATYANARAYANA NARLAPURAM
<satyanarlapuram@gmail.com> wrote:
> It appears HAVING-to-WHERE pushdown is still wrong with CASE and nondeterministic
> collations. The shorthand CASE expression bypasses the new collation-conflict detector,
> so the HAVING clause gets pushed to WHERE, filtering rows before
> grouping and silently changing aggregate results.

Right.  For simple CASE (CaseExpr with a non-NULL arg), parse analysis
builds each WHEN as OpExpr(CaseTestExpr op val), where CaseTestExpr is
a placeholder for the arg, while the actual arg sits at cexpr->arg,
outside the OpExpr that carries the comparison's inputcollid.  A GROUP
Var at cexpr->arg is therefore visited without the WHEN's inputcollid
on the stack.  So the conflict fails to be detected, and the HAVING
clause is incorrectly pushed to WHERE.

> My understanding is shallow here, attached a draft patch that adds a CaseExpr branch to
> having_collation_conflict_walker() mirroring the existing RowCompareExpr
> special case. Patch includes the tests. Please take a look.

This patch is on the right track.  I didn't like how the stack was
restored after walking cexpr->arg; list_truncate fits better there.
The comments also needed some tightening.  I've made those
adjustments, pushed, and back-patched.

- Richard