Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix HAVING-to-WHERE pushdown for simple-CASE form

  2. Fix HAVING-to-WHERE pushdown with nondeterministic collations

  1. Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-03-31T03:41:09Z

    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

  2. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    wenhui qiu <qiuwenhuifx@gmail.com> — 2026-04-01T02:19:09Z

    Hi Richard
> + if (OidIsValid(inputcollid))
> + {
> + List   *vars;
> +
> + /*
> + * We use PVC_RECURSE_PLACEHOLDERS because PlaceHolderVars may have
> + * been introduced by pull_up_subqueries, and we need to look through
> + * them to find the underlying Vars.  We don't need to consider
> + * Aggrefs because clauses containing aggregates are already excluded
> + * from HAVING-to-WHERE pushdown by the contain_agg_clause check.
> + * Likewise, WindowFuncs are ignored since they cannot appear in a
> + * HAVING clause.
> + */
> + 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);
> + }
> +
> + return expression_tree_walker(node, having_collation_conflict_walker,
> +  group_rtindex);
> +}
This might be overthinking, but I wonder if calling pull_var_clause() at
each walker step could introduce some overhead due to repeated subtree scans
,Should we add a test （SELECT x, count(*) FROM test3ci GROUP BY x HAVING
max(x) = 'abc' COLLATE case_sensitive;）

Thanks

On Tue, Mar 31, 2026 at 11:41 AM Richard Guo <guofenglinux@gmail.com> wrote:

> 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
>

  3. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-04-02T10:11:07Z

    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 overhead due 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

  4. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-04-22T06:36:23Z

    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

  5. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-04-30T03:08:46Z

    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 overhead due 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

  6. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-05-01T02:47:20Z

    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

  7. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    SATYANARAYANA NARLAPURAM <satyanarlapuram@gmail.com> — 2026-05-06T00:58:02Z

    Hi Richard,

On Thu, Apr 30, 2026 at 7:47 PM Richard Guo <guofenglinux@gmail.com> wrote:

> 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.
>

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.

Repro:

CREATE COLLATION ci (provider=icu, locale='und-u-ks-level2',
                     deterministic=false);
CREATE COLLATION cs (provider=icu, locale='und',
                     deterministic=true);
CREATE TABLE t (x text COLLATE ci);
INSERT INTO t VALUES ('abc'),('ABC'),('def'),('DEF'),('xyz');
CREATE COLLATION
CREATE COLLATION
CREATE TABLE
INSERT 0 5

-- This works correctly as fixed in the patch
srcdb=# EXPLAIN (COSTS OFF)
SELECT x, count(*) FROM t GROUP BY x
  HAVING x = 'abc' COLLATE cs;
               QUERY PLAN
----------------------------------------
 HashAggregate
   Group Key: x
   Filter: (x = 'abc'::text COLLATE cs)
   ->  Seq Scan on t
(4 rows)

srcdb=# SELECT x, count(*) FROM t GROUP BY x
  HAVING x = 'abc' COLLATE cs;
  x  | count
-----+-------
 abc |     2
(1 row)


-- CASE from incorrectly pushed to WHERE
EXPLAIN (COSTS OFF)
SELECT x, count(*) FROM t GROUP BY x
  HAVING CASE x WHEN 'abc' COLLATE cs THEN true ELSE false END;
                                 QUERY PLAN

-----------------------------------------------------------------------------
 HashAggregate
   Group Key: x
   ->  Seq Scan on t
         Filter: CASE x WHEN 'abc'::text COLLATE cs THEN true ELSE false END
(4 rows)

 SELECT x, count(*) FROM t GROUP BY x
  HAVING CASE x WHEN 'abc' COLLATE cs THEN true ELSE false END;
  x  | count
-----+-------
 abc |     1
(1 row)

Under the case-insensitive GROUP BY collation 'ci', 'abc' and 'ABC'
belong to the same group with count=2.  The case-sensitive filter must
run after grouping, not before.  But when hidden inside CASE, it runs
as a Seq Scan filter and eliminates 'ABC' before it can be counted.

having_collation_conflict_walker() walks the HAVING clause top-down,
maintaining a stack of ancestor inputcollids.  When it reaches a GROUP
Var with a nondeterministic varcollid, it reports a conflict if any
ancestor pushed a different collation.  The ancestor collations are
gathered via exprInputCollation(), which doesn't cover CaseExpr.

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.

Thanks,
Satya

  8. Re: Fix HAVING-to-WHERE pushdown with nondeterministic collations

    Richard Guo <guofenglinux@gmail.com> — 2026-05-08T08:07:07Z

    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