public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chengpeng Yan <[email protected]>
To: Tender Wang <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: BUG #19534: Qual pushdown across a window subquery is unsafe with nondeterministic partition collations
Date: Sun, 28 Jun 2026 13:43:22 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAHewXNk8xt1pNepcEiCx_e6_SW_=bm1GNnU7+bPczoZJuLC5qQ@mail.gmail.com>
References: <[email protected]>
<CAHewXNk8xt1pNepcEiCx_e6_SW_=bm1GNnU7+bPczoZJuLC5qQ@mail.gmail.com>
Hi
> On Jun 28, 2026, at 19:33, Tender Wang <[email protected]> wrote:
>
> Yes, I can reproduce this on HEAD.
> The whereClause was pushed down to the scan level of table t_window_ci.
> So the row with 'ABC' was ignored by the filter.
>
> In current logic, we have no logic to process a nondeterministic
> partition clause when considering whether the
> qual can be pushed down to the subquery.
>
> I try to fix this with the attached patch.
> I add a new flag UNSAFE_HAS_NONDETERMINITIC for
> pushdown_safety_info.unsafeFlags.
> If the var is in the partition clause and it is nondeterministic, we
> set this flag.
> In qual_is_pushdown_safe(), we check the qual's inputcollid; if it is
> deterministic, but the var in
> the partition clause is nondeterministic, it is PUSHDOWN_UNSAFE.
>
> Any thoughts?
> --
> Thanks,
> Tender Wang
> <0001-Fix-qual-pushdown-with-nondeterministic-partition-co.patch>
Thanks for working on this. I was looking into this issue too, and I
agree that this is a wrong-result bug in the window-function subquery
qual pushdown.
Your approach looks reasonable for the most direct collation-conflict
case: a qual can use equality semantics that distinguish values that the
window partition treats as equal. I think the underlying safety
condition is a bit broader, though. The pushed-down qual must return
the same result for all values in a window partition equality class.
The same risk is not limited to explicit collation conflicts. Using the
same kind of nondeterministic case-insensitive partition key as the
report, a non-collation-aware qual can still split a partition:
```
CREATE COLLATION case_insensitive
(provider = icu, locale = '@colStrength=secondary',
deterministic = false);
CREATE TABLE t (x text COLLATE case_insensitive);
INSERT INTO t VALUES ('abc'), ('ABC');
SELECT x, c
FROM (
SELECT x, count(*) OVER (PARTITION BY x) AS c
FROM t
) s
WHERE ascii(x) = 97;
```
Without the patch, the planner can push the `ascii(x) = 97` filter below
`WindowAgg`, so the result is:
```
x | c
-----+---
abc | 1
```
The correct result is:
```
x | c
-----+---
abc | 2
```
Postgres only knows at this point that the column appears in every
window `PARTITION BY` list; it does not know that an arbitrary qual on
that column is constant over the partition equality class. The attached
v2 patch therefore takes a conservative approach: if the matching
partition key uses a nondeterministic collation, ordinary qual pushdown
is disabled for that column.
The tradeoff is that we may miss some pushdown opportunities for quals
that really are constant over the partition. But for a wrong-result
case, that is safer than pushing a qual whose behavior over the
partition is not proven. Deterministic partition keys keep the existing
behavior.
I ran `make check`; all tests passed.
Any thoughts?
--
Best regards,
Chengpeng Yan
Attachments:
[application/octet-stream] v2-0001-Fix-window-qual-pushdown-with-nondeterministic-co.patch (11.2K, ../[email protected]/3-v2-0001-Fix-window-qual-pushdown-with-nondeterministic-co.patch)
download | inline diff:
From 8ad1ebc00d13c7716a578a06ec56a45c0bc246a6 Mon Sep 17 00:00:00 2001
From: Chengpeng Yan <[email protected]>
Date: Sun, 28 Jun 2026 18:11:15 +0800
Subject: [PATCH v2] Fix window qual pushdown with nondeterministic collations
Qual pushdown into a window-function subquery is safe only when it
keeps or rejects whole window partitions. That proof fails for a
nondeterministic partition collation, because an upper qual can
distinguish values that the partition equality treats as equal.
Mark such partition-key output columns unsafe for ordinary qual
pushdown while leaving deterministic partition keys unchanged.
Add ICU regression tests for a stricter-collation qual and for a
non-collation expression that distinguishes values in one partition.
---
src/backend/optimizer/path/allpaths.c | 68 +++++++++++--
.../regress/expected/collate.icu.utf8.out | 98 +++++++++++++++++++
src/test/regress/sql/collate.icu.utf8.sql | 57 +++++++++++
3 files changed, 214 insertions(+), 9 deletions(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index c134594a21a..a6fd3c6c484 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -57,6 +57,7 @@
#define UNSAFE_NOTIN_DISTINCTON_CLAUSE (1 << 2)
#define UNSAFE_NOTIN_PARTITIONBY_CLAUSE (1 << 3)
#define UNSAFE_TYPE_MISMATCH (1 << 4)
+#define UNSAFE_NONDETERMINISTIC_PARTITION (1 << 5)
/* results of subquery_is_pushdown_safe */
typedef struct pushdown_safety_info
@@ -158,7 +159,8 @@ static void check_output_expressions(Query *subquery,
pushdown_safety_info *safetyInfo);
static void compare_tlist_datatypes(List *tlist, List *colTypes,
pushdown_safety_info *safetyInfo);
-static bool targetIsInAllPartitionLists(TargetEntry *tle, Query *query);
+static bool targetIsInAllPartitionLists(TargetEntry *tle, Query *query,
+ bool *nondeterministic_partition);
static pushdown_safe_type qual_is_pushdown_safe(Query *subquery, Index rti,
RestrictInfo *rinfo,
pushdown_safety_info *safetyInfo);
@@ -4263,6 +4265,11 @@ recurse_pushdown_safe(Node *setOp, Query *topquery,
* subquery_is_pushdown_safe handles that.). Subquery columns marked as
* unsafe for this reason can still have WindowClause run conditions pushed
* down.
+ *
+ * The same reasoning does not hold if the partition key's collation is
+ * nondeterministic, since a qual might distinguish values that the
+ * partitioning equality operator sees as equal. In that case, mark the
+ * column unsafe for ordinary qual pushdown.
*/
static void
check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
@@ -4338,12 +4345,23 @@ check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
/* If subquery uses window functions, check point 4 */
if (subquery->hasWindowFuncs &&
(safetyInfo->unsafeFlags[tle->resno] &
- UNSAFE_NOTIN_PARTITIONBY_CLAUSE) == 0 &&
- !targetIsInAllPartitionLists(tle, subquery))
+ (UNSAFE_NOTIN_PARTITIONBY_CLAUSE |
+ UNSAFE_NONDETERMINISTIC_PARTITION)) == 0)
{
- /* not present in all PARTITION BY clauses, so mark it unsafe */
- safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
- continue;
+ bool nondeterministic_partition;
+
+ if (!targetIsInAllPartitionLists(tle, subquery,
+ &nondeterministic_partition))
+ {
+ /* not present in all PARTITION BY clauses, so mark it unsafe */
+ safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NOTIN_PARTITIONBY_CLAUSE;
+ continue;
+ }
+ if (nondeterministic_partition)
+ {
+ safetyInfo->unsafeFlags[tle->resno] |= UNSAFE_NONDETERMINISTIC_PARTITION;
+ continue;
+ }
}
}
}
@@ -4393,21 +4411,52 @@ compare_tlist_datatypes(List *tlist, List *colTypes,
* True if the TargetEntry is listed in the PARTITION BY clause
* of every window defined in the query.
*
+ * If so, set *nondeterministic_partition when any matching PARTITION BY
+ * expression has a nondeterministic collation.
+ *
* It would be safe to ignore windows not actually used by any window
* function, but it's not easy to get that info at this stage; and it's
* unlikely to be useful to spend any extra cycles getting it, since
* unreferenced window definitions are probably infrequent in practice.
*/
static bool
-targetIsInAllPartitionLists(TargetEntry *tle, Query *query)
+targetIsInAllPartitionLists(TargetEntry *tle, Query *query,
+ bool *nondeterministic_partition)
{
+ Index ref = tle->ressortgroupref;
ListCell *lc;
+ *nondeterministic_partition = false;
+
+ /* no need to scan lists if tle has no marker */
+ if (ref == 0)
+ return false;
+
foreach(lc, query->windowClause)
{
WindowClause *wc = (WindowClause *) lfirst(lc);
+ ListCell *lc2;
+ bool found = false;
+
+ foreach(lc2, wc->partitionClause)
+ {
+ SortGroupClause *scl = (SortGroupClause *) lfirst(lc2);
+
+ if (scl->tleSortGroupRef == ref)
+ {
+ TargetEntry *ptle = get_sortgroupclause_tle(scl,
+ query->targetList);
+ Oid collid = exprCollation((Node *) ptle->expr);
+
+ if (OidIsValid(collid) &&
+ !get_collation_isdeterministic(collid))
+ *nondeterministic_partition = true;
+ found = true;
+ break;
+ }
+ }
- if (!targetIsInSortList(tle, InvalidOid, wc->partitionClause))
+ if (!found)
return false;
}
return true;
@@ -4520,7 +4569,8 @@ qual_is_pushdown_safe(Query *subquery, Index rti, RestrictInfo *rinfo,
{
if (safetyInfo->unsafeFlags[var->varattno] &
(UNSAFE_HAS_VOLATILE_FUNC | UNSAFE_HAS_SET_FUNC |
- UNSAFE_NOTIN_DISTINCTON_CLAUSE | UNSAFE_TYPE_MISMATCH))
+ UNSAFE_NOTIN_DISTINCTON_CLAUSE | UNSAFE_TYPE_MISMATCH |
+ UNSAFE_NONDETERMINISTIC_PARTITION))
{
safe = PUSHDOWN_UNSAFE;
break;
diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out
index 04e2f6df037..f46217bfb12 100644
--- a/src/test/regress/expected/collate.icu.utf8.out
+++ b/src/test/regress/expected/collate.icu.utf8.out
@@ -2348,6 +2348,104 @@ SELECT x, count(*) FROM test3cs GROUP BY x HAVING x = 'abc' COLLATE case_insensi
ABC | 1
(2 rows)
+-- Test qual pushdown with window functions and nondeterministic collations.
+-- When the window partition key uses a nondeterministic collation, an upper
+-- qual that references the partition key must not be pushed below WindowAgg,
+-- otherwise it can remove some but not all rows from a window partition.
+-- Negative: conflicting collation, qual must stay above WindowAgg
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+ QUERY PLAN
+------------------------------------------------------------
+ Subquery Scan on s
+ Filter: (s.x = 'abc'::text COLLATE case_sensitive)
+ -> WindowAgg
+ Window: w1 AS (PARTITION BY test3ci.x)
+ -> Sort
+ Sort Key: test3ci.x COLLATE case_insensitive
+ -> Seq Scan on test3ci
+(7 rows)
+
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+ x | part_count
+-----+------------
+ abc | 2
+(1 row)
+
+-- Negative: non-collation-aware expression can still distinguish rows that
+-- the nondeterministic partition equality considers equal.
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE ascii(x) = 97
+ORDER BY x;
+ QUERY PLAN
+------------------------------------------------------------
+ Subquery Scan on s
+ Filter: (ascii(s.x) = 97)
+ -> WindowAgg
+ Window: w1 AS (PARTITION BY test3ci.x)
+ -> Sort
+ Sort Key: test3ci.x COLLATE case_insensitive
+ -> Seq Scan on test3ci
+(7 rows)
+
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE ascii(x) = 97
+ORDER BY x;
+ x | part_count
+-----+------------
+ abc | 2
+(1 row)
+
+-- Positive: deterministic partition key retains the existing pushdown path
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3cs
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+ QUERY PLAN
+----------------------------------------------------------
+ WindowAgg
+ Window: w1 AS ()
+ -> Seq Scan on test3cs
+ Filter: (x = 'abc'::text COLLATE case_sensitive)
+(4 rows)
+
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3cs
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+ x | part_count
+-----+------------
+ abc | 1
+(1 row)
+
-- bpchar
CREATE TABLE test1bpci (x char(3) COLLATE case_insensitive);
CREATE TABLE test2bpci (x char(3) COLLATE case_insensitive);
diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql
index 18c47e6e05a..abcb3014967 100644
--- a/src/test/regress/sql/collate.icu.utf8.sql
+++ b/src/test/regress/sql/collate.icu.utf8.sql
@@ -826,6 +826,63 @@ EXPLAIN (COSTS OFF)
SELECT x, count(*) FROM test3cs GROUP BY x HAVING x = 'abc' COLLATE case_insensitive ORDER BY 1;
SELECT x, count(*) FROM test3cs GROUP BY x HAVING x = 'abc' COLLATE case_insensitive ORDER BY 1;
+-- Test qual pushdown with window functions and nondeterministic collations.
+-- When the window partition key uses a nondeterministic collation, an upper
+-- qual that references the partition key must not be pushed below WindowAgg,
+-- otherwise it can remove some but not all rows from a window partition.
+
+-- Negative: conflicting collation, qual must stay above WindowAgg
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+
+-- Negative: non-collation-aware expression can still distinguish rows that
+-- the nondeterministic partition equality considers equal.
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE ascii(x) = 97
+ORDER BY x;
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3ci
+) s
+WHERE ascii(x) = 97
+ORDER BY x;
+
+-- Positive: deterministic partition key retains the existing pushdown path
+EXPLAIN (COSTS OFF)
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3cs
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+SELECT x, part_count
+FROM (
+ SELECT x, count(*) OVER (PARTITION BY x) AS part_count
+ FROM test3cs
+) s
+WHERE x = 'abc' COLLATE case_sensitive
+ORDER BY x;
+
-- bpchar
CREATE TABLE test1bpci (x char(3) COLLATE case_insensitive);
CREATE TABLE test2bpci (x char(3) COLLATE case_insensitive);
--
2.50.1 (Apple Git-155)
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19534: Qual pushdown across a window subquery is unsafe with nondeterministic partition collations
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox