public inbox for [email protected]
help / color / mirror / Atom feedFix duplicate detection for null-treatment window functions
2+ messages / 1 participants
[nested] [flat]
* Fix duplicate detection for null-treatment window functions
@ 2026-07-07 15:14 Chao Li <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Chao Li @ 2026-07-07 15:14 UTC (permalink / raw)
To: PostgreSQL-development <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>
Hi,
I just spotted an oversight from “[25a30bbd4] Add IGNORE NULLS/RESPECT NULLS option to Window functions”.
In ExecInitWindowAgg(), there is logic to detect duplicate functions:
```
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
{
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}
```
However, when appending function info to perfunc, ignore_nulls is not copied:
```
/* Fill in the perfuncstate data */
perfuncstate->wfuncstate = wfuncstate;
perfuncstate->wfunc = wfunc;
perfuncstate->numArguments = list_length(wfuncstate->args);
perfuncstate->winCollation = wfunc->inputcollid;
```
As a result, wfunc->ignore_nulls == perfunc[i].ignore_nulls can never be true for duplicate IGNORE NULLS or explicit RESPECT NULLS calls. This means duplicate detection doesn't work for those calls. This bug is easy to prove by adding temporary logs, and the fix is straightforward: copy ignore_nulls when filling in the perfuncstate data.
This is a simple repro:
Without the fix:
```
evantest=# WITH t(x) AS (VALUES (NULL::int), (1), (2))
evantest-# SELECT first_value(x) IGNORE NULLS OVER w AS a,
evantest-# first_value(x) IGNORE NULLS OVER w AS b
evantest-# FROM t
evantest-# WINDOW w AS (ORDER BY x NULLS FIRST
evantest(# ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING);
INFO: WindowAgg duplicate cache miss: no previous match, ignore_nulls 1
INFO: WindowAgg duplicate cache miss: matched wfuncno 0, ignore_nulls 1, cached ignore_nulls 0
a | b
---+---
1 | 1
1 | 1
1 | 1
(3 rows)
```
With the fix:
```
evantest=# WITH t(x) AS (VALUES (NULL::int), (1), (2))
evantest-# SELECT first_value(x) IGNORE NULLS OVER w AS a,
evantest-# first_value(x) IGNORE NULLS OVER w AS b
evantest-# FROM t
evantest-# WINDOW w AS (ORDER BY x NULLS FIRST
evantest(# ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING);
INFO: WindowAgg duplicate cache miss: no previous match, ignore_nulls 1
INFO: WindowAgg duplicate cache hit: existing wfuncno 0, ignore_nulls 1
a | b
---+---
1 | 1
1 | 1
1 | 1
(3 rows)
```
See the attached patch for details. The actual fix is only one line. The INFO logs above were produced with temporary debug logs added around the duplicate-function lookup, those logs are not part of the proposed fix. I left those logs in the patch with TODO comments only so reviewers can see the behavior before and after the fix. They should be removed before pushing.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Fix-duplicate-detection-for-null-treatment-window.patch (2.5K, ../../[email protected]/2-v1-0001-Fix-duplicate-detection-for-null-treatment-window.patch)
download | inline diff:
From 252b1e9b361feb82a12931787bb6108b11063598 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Tue, 7 Jul 2026 08:10:58 -0700
Subject: [PATCH v1] Fix duplicate detection for null-treatment window
functions
`ExecInitWindowAgg()` checks `perfunc[i].ignore_nulls` when deciding
whether a window function call can reuse an existing `WindowStatePerFunc`
entry. However, that field was never initialized when filling the
`perfuncstate` data, so it remained zero from `palloc0_array()`.
As a result, duplicate calls using `IGNORE NULLS` or explicit
`RESPECT NULLS` failed to reuse the existing entry, because their
`wfunc->ignore_nulls` value did not match the zeroed per-function state.
Plain calls were unaffected because their null-treatment value is also zero.
Copy `wfunc->ignore_nulls` into `perfuncstate->ignore_nulls` during
initialization so duplicate null-treatment window function calls are
deduplicated as intended.
Oversight in 25a30bbd423.
Author: Chao Li <[email protected]>
---
src/backend/executor/nodeWindowAgg.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index f1c524d00df..676fe024965 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2749,11 +2749,23 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
{
+ /* TODO: remove this log before pushing */
+ elog(INFO, "WindowAgg duplicate cache hit: existing wfuncno %d, ignore_nulls %d",
+ i, wfunc->ignore_nulls);
+
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}
+ /* TODO: remove this log before pushing */
+ if (i <= wfuncno)
+ elog(INFO, "WindowAgg duplicate cache miss: matched wfuncno %d, ignore_nulls %d, cached ignore_nulls %d",
+ i, wfunc->ignore_nulls, perfunc[i].ignore_nulls);
+ else
+ elog(INFO, "WindowAgg duplicate cache miss: no previous match, ignore_nulls %d",
+ wfunc->ignore_nulls);
+
/* Nope, so assign a new PerAgg record */
perfuncstate = &perfunc[++wfuncno];
@@ -2773,6 +2785,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
perfuncstate->wfunc = wfunc;
perfuncstate->numArguments = list_length(wfuncstate->args);
perfuncstate->winCollation = wfunc->inputcollid;
+ perfuncstate->ignore_nulls = wfunc->ignore_nulls;
get_typlenbyval(wfunc->wintype,
&perfuncstate->resulttypeLen,
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: Fix duplicate detection for null-treatment window functions
@ 2026-07-09 12:32 Chao Li <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Chao Li @ 2026-07-09 12:32 UTC (permalink / raw)
To: PostgreSQL-development <[email protected]>; +Cc: Tatsuo Ishii <[email protected]>
> On Jul 7, 2026, at 08:14, Chao Li <[email protected]> wrote:
>
> Hi,
>
> I just spotted an oversight from “[25a30bbd4] Add IGNORE NULLS/RESPECT NULLS option to Window functions”.
>
> In ExecInitWindowAgg(), there is logic to detect duplicate functions:
> ```
> if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
> {
> /* Found a match to an existing entry, so just mark it */
> wfuncstate->wfuncno = i;
> continue;
> }
> ```
>
> However, when appending function info to perfunc, ignore_nulls is not copied:
> ```
> /* Fill in the perfuncstate data */
> perfuncstate->wfuncstate = wfuncstate;
> perfuncstate->wfunc = wfunc;
> perfuncstate->numArguments = list_length(wfuncstate->args);
> perfuncstate->winCollation = wfunc->inputcollid;
> ```
>
> As a result, wfunc->ignore_nulls == perfunc[i].ignore_nulls can never be true for duplicate IGNORE NULLS or explicit RESPECT NULLS calls. This means duplicate detection doesn't work for those calls. This bug is easy to prove by adding temporary logs, and the fix is straightforward: copy ignore_nulls when filling in the perfuncstate data.
>
> This is a simple repro:
>
> Without the fix:
> ```
> evantest=# WITH t(x) AS (VALUES (NULL::int), (1), (2))
> evantest-# SELECT first_value(x) IGNORE NULLS OVER w AS a,
> evantest-# first_value(x) IGNORE NULLS OVER w AS b
> evantest-# FROM t
> evantest-# WINDOW w AS (ORDER BY x NULLS FIRST
> evantest(# ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING);
> INFO: WindowAgg duplicate cache miss: no previous match, ignore_nulls 1
> INFO: WindowAgg duplicate cache miss: matched wfuncno 0, ignore_nulls 1, cached ignore_nulls 0
> a | b
> ---+---
> 1 | 1
> 1 | 1
> 1 | 1
> (3 rows)
> ```
>
> With the fix:
> ```
> evantest=# WITH t(x) AS (VALUES (NULL::int), (1), (2))
> evantest-# SELECT first_value(x) IGNORE NULLS OVER w AS a,
> evantest-# first_value(x) IGNORE NULLS OVER w AS b
> evantest-# FROM t
> evantest-# WINDOW w AS (ORDER BY x NULLS FIRST
> evantest(# ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING);
> INFO: WindowAgg duplicate cache miss: no previous match, ignore_nulls 1
> INFO: WindowAgg duplicate cache hit: existing wfuncno 0, ignore_nulls 1
> a | b
> ---+---
> 1 | 1
> 1 | 1
> 1 | 1
> (3 rows)
> ```
>
> See the attached patch for details. The actual fix is only one line. The INFO logs above were produced with temporary debug logs added around the duplicate-function lookup, those logs are not part of the proposed fix. I left those logs in the patch with TODO comments only so reviewers can see the behavior before and after the fix. They should be removed before pushing.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
> <v1-0001-Fix-duplicate-detection-for-null-treatment-window.patch>
I realized that leaving the temp log code in the patch is not friendly to the CF test. So, splitting the temp log part into a diff file.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] nocfbot.windowagg-null-treatment-debug-logs.diff (1.1K, ../../[email protected]/2-nocfbot.windowagg-null-treatment-debug-logs.diff)
download | inline diff:
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 676fe024965..00000000000 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2749,11 +2749,23 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
{
+ /* TODO: remove this log before pushing */
+ elog(INFO, "WindowAgg duplicate cache hit: existing wfuncno %d, ignore_nulls %d",
+ i, wfunc->ignore_nulls);
+
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}
+ /* TODO: remove this log before pushing */
+ if (i <= wfuncno)
+ elog(INFO, "WindowAgg duplicate cache miss: matched wfuncno %d, ignore_nulls %d, cached ignore_nulls %d",
+ i, wfunc->ignore_nulls, perfunc[i].ignore_nulls);
+ else
+ elog(INFO, "WindowAgg duplicate cache miss: no previous match, ignore_nulls %d",
+ wfunc->ignore_nulls);
+
/* Nope, so assign a new PerAgg record */
perfuncstate = &perfunc[++wfuncno];
[application/octet-stream] v2-0001-Fix-duplicate-detection-for-null-treatment-window.patch (1.6K, ../../[email protected]/3-v2-0001-Fix-duplicate-detection-for-null-treatment-window.patch)
download | inline diff:
From b87100e21c618107c1f17d64c9efcd6d2e32a428 Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Tue, 7 Jul 2026 08:10:58 -0700
Subject: [PATCH v2] Fix duplicate detection for null-treatment window
functions
`ExecInitWindowAgg()` checks `perfunc[i].ignore_nulls` when deciding
whether a window function call can reuse an existing `WindowStatePerFunc`
entry. However, that field was never initialized when filling the
`perfuncstate` data, so it remained zero from `palloc0_array()`.
As a result, duplicate calls using `IGNORE NULLS` or explicit
`RESPECT NULLS` failed to reuse the existing entry, because their
`wfunc->ignore_nulls` value did not match the zeroed per-function state.
Plain calls were unaffected because their null-treatment value is also zero.
Copy `wfunc->ignore_nulls` into `perfuncstate->ignore_nulls` during
initialization so duplicate null-treatment window function calls are
deduplicated as intended.
Oversight in 25a30bbd423.
Author: Chao Li <[email protected]>
---
src/backend/executor/nodeWindowAgg.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index f1c524d00df..b8b0d853dec 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2773,6 +2773,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
perfuncstate->wfunc = wfunc;
perfuncstate->numArguments = list_length(wfuncstate->args);
perfuncstate->winCollation = wfunc->inputcollid;
+ perfuncstate->ignore_nulls = wfunc->ignore_nulls;
get_typlenbyval(wfunc->wintype,
&perfuncstate->resulttypeLen,
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-07-09 12:32 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-07 15:14 Fix duplicate detection for null-treatment window functions Chao Li <[email protected]>
2026-07-09 12:32 ` Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox