Fix duplicate detection for null-treatment window functions

On Jul 7, 2026, at 08:14, Chao Li <li.evan.chao@gmail.com> 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/

Hi Chao,

On Thu, Jul 9, 2026 at 8:35 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 7, 2026, at 08:14, Chao Li <li.evan.chao@gmail.com> 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.

Nice catch, and v2 does restore the intended de-duplication.

While reviewing it, though, I think the check the patch repairs is actually
redundant, and the real issue is that the code (both the original commit and
the patch) keeps a shadow copy of ignore_nulls that has to be maintained by
hand.

WindowFunc.ignore_nulls is a plain scalar field with no pg_node_attr, so
equal() already compares it -- the generated _equalWindowFunc() has:

COMPARE_SCALAR_FIELD(winagg);
COMPARE_SCALAR_FIELD(ignore_nulls);
COMPARE_LOCATION_FIELD(location);

That means the equal() call in the dedup loop already distinguishes two
WindowFuncs that differ only in null treatment:

for (i = 0; i <= wfuncno; i++)
{
if (equal(wfunc, perfunc[i].wfunc) &&
!contain_volatile_functions((Node *) wfunc))
break;
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)

If ignore_nulls differs, equal() returns false, the loop never breaks on
that entry, and we never reach the extra term. If equal() matches, then
ignore_nulls necessarily matched too. So "&& wfunc->ignore_nulls ==
perfunc[i].ignore_nulls" can never change the outcome, and
WindowStatePerFuncData.ignore_nulls exists only to feed it -- it is read
only there and, before this patch, written nowhere (which is exactly why it
was always 0).

So rather than populating the field, I'd suggest dropping the redundant term
and the field, and letting equal() do the work:

and the field, and letting equal() do the work:

-   if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
+   if (i <= wfuncno)
    {
        /* Found a match to an existing entry, so just mark it */
        wfuncstate->wfuncno = i;
        continue;
    }

plus removing the ignore_nulls member from WindowStatePerFuncData and
trimming the now-stale "which needs the same ignore_nulls value" comment.
That fixes the same bug while removing the duplicated state that caused it,
so it can't silently drift again.

The one argument for an explicit check is defensiveness: if someone later
tags ignore_nulls with a pg_node_attr that excludes it from equal(), the
loop would start collapsing functions with different null treatment. If
that's a worry, the robust form compares the stored node directly instead of
a shadow copy, and still needs no separate field:

if (i <= wfuncno &&
wfunc->ignore_nulls == perfunc[i].wfunc->ignore_nulls)

Given ignore_nulls has to stay significant to equal() anyway (two calls with
different null treatment really are different functions), I'd lean toward
just removing the check.

Happy to send a patch along these lines if you agree.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

--
Regards,
Ewan Young

On Jul 10, 2026, at 11:44, Ewan Young <kdbase.hack@gmail.com> wrote:

Hi Chao,

On Thu, Jul 9, 2026 at 8:35 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 7, 2026, at 08:14, Chao Li <li.evan.chao@gmail.com> 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.

Nice catch, and v2 does restore the intended de-duplication.

While reviewing it, though, I think the check the patch repairs is actually
redundant, and the real issue is that the code (both the original commit and
the patch) keeps a shadow copy of ignore_nulls that has to be maintained by
hand.

WindowFunc.ignore_nulls is a plain scalar field with no pg_node_attr, so
equal() already compares it -- the generated _equalWindowFunc() has:

COMPARE_SCALAR_FIELD(winagg);
COMPARE_SCALAR_FIELD(ignore_nulls);
COMPARE_LOCATION_FIELD(location);

That means the equal() call in the dedup loop already distinguishes two
WindowFuncs that differ only in null treatment:

for (i = 0; i <= wfuncno; i++)
{
if (equal(wfunc, perfunc[i].wfunc) &&
!contain_volatile_functions((Node *) wfunc))
break;
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)

If ignore_nulls differs, equal() returns false, the loop never breaks on
that entry, and we never reach the extra term. If equal() matches, then
ignore_nulls necessarily matched too. So "&& wfunc->ignore_nulls ==
perfunc[i].ignore_nulls" can never change the outcome, and
WindowStatePerFuncData.ignore_nulls exists only to feed it -- it is read
only there and, before this patch, written nowhere (which is exactly why it
was always 0).

So rather than populating the field, I'd suggest dropping the redundant term
and the field, and letting equal() do the work:

and the field, and letting equal() do the work:

-   if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
+   if (i <= wfuncno)
{
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}

plus removing the ignore_nulls member from WindowStatePerFuncData and
trimming the now-stale "which needs the same ignore_nulls value" comment.
That fixes the same bug while removing the duplicated state that caused it,
so it can't silently drift again.

The one argument for an explicit check is defensiveness: if someone later
tags ignore_nulls with a pg_node_attr that excludes it from equal(), the
loop would start collapsing functions with different null treatment. If
that's a worry, the robust form compares the stored node directly instead of
a shadow copy, and still needs no separate field:

if (i <= wfuncno &&
wfunc->ignore_nulls == perfunc[i].wfunc->ignore_nulls)

Given ignore_nulls has to stay significant to equal() anyway (two calls with
different null treatment really are different functions), I'd lean toward
just removing the check.

Happy to send a patch along these lines if you agree.

I think your analysis is correct, please feel free to post your version.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Wed, Jul 15, 2026 at 12:49 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 10, 2026, at 11:44, Ewan Young <kdbase.hack@gmail.com> wrote:

Hi Chao,

On Thu, Jul 9, 2026 at 8:35 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 7, 2026, at 08:14, Chao Li <li.evan.chao@gmail.com> 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.

Nice catch, and v2 does restore the intended de-duplication.

While reviewing it, though, I think the check the patch repairs is actually
redundant, and the real issue is that the code (both the original commit and
the patch) keeps a shadow copy of ignore_nulls that has to be maintained by
hand.

WindowFunc.ignore_nulls is a plain scalar field with no pg_node_attr, so
equal() already compares it -- the generated _equalWindowFunc() has:

COMPARE_SCALAR_FIELD(winagg);
COMPARE_SCALAR_FIELD(ignore_nulls);
COMPARE_LOCATION_FIELD(location);

That means the equal() call in the dedup loop already distinguishes two
WindowFuncs that differ only in null treatment:

for (i = 0; i <= wfuncno; i++)
{
if (equal(wfunc, perfunc[i].wfunc) &&
!contain_volatile_functions((Node *) wfunc))
break;
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)

If ignore_nulls differs, equal() returns false, the loop never breaks on
that entry, and we never reach the extra term. If equal() matches, then
ignore_nulls necessarily matched too. So "&& wfunc->ignore_nulls ==
perfunc[i].ignore_nulls" can never change the outcome, and
WindowStatePerFuncData.ignore_nulls exists only to feed it -- it is read
only there and, before this patch, written nowhere (which is exactly why it
was always 0).

So rather than populating the field, I'd suggest dropping the redundant term
and the field, and letting equal() do the work:

and the field, and letting equal() do the work:

-   if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
+   if (i <= wfuncno)
{
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}

plus removing the ignore_nulls member from WindowStatePerFuncData and
trimming the now-stale "which needs the same ignore_nulls value" comment.
That fixes the same bug while removing the duplicated state that caused it,
so it can't silently drift again.

The one argument for an explicit check is defensiveness: if someone later
tags ignore_nulls with a pg_node_attr that excludes it from equal(), the
loop would start collapsing functions with different null treatment. If
that's a worry, the robust form compares the stored node directly instead of
a shadow copy, and still needs no separate field:

if (i <= wfuncno &&
wfunc->ignore_nulls == perfunc[i].wfunc->ignore_nulls)

Given ignore_nulls has to stay significant to equal() anyway (two calls with
different null treatment really are different functions), I'd lean toward
just removing the check.

Happy to send a patch along these lines if you agree.

I think your analysis is correct, please feel free to post your version.

Thanks, Chao. Patch attached.

It drops the redundant "&& wfunc->ignore_nulls == perfunc[i].ignore_nulls"
term together with WindowStatePerFuncData.ignore_nulls, and rewords the
now-stale comment, leaving equal() to distinguish calls that differ only
in null treatment -- which it already does, since WindowFunc.ignore_nulls
is a plain scalar field that _equalWindowFunc() compares. This fixes the
same bug your v2 fixed, but removes the hand-maintained shadow copy that
was the root cause, so it can't silently drift out of sync again.

Testing:
- Since the patch changes the layout of WindowStatePerFuncData, I also
built the whole tree with -fsanitize=alignment,undefined
-fno-sanitize-recover=alignment and re-ran make check under it: 245/245,
no runtime errors. The removed byte sat right before an 8-byte-aligned
pointer (winobj), so it was absorbed by padding and no following field
offset changes.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

--
Regards,
Ewan Young

Ewan, Chao,

Thank you for the finding and patches.
I will take care of this.

Thanks, Chao. Patch attached.

It drops the redundant "&& wfunc->ignore_nulls == perfunc[i].ignore_nulls"
term together with WindowStatePerFuncData.ignore_nulls, and rewords the
now-stale comment, leaving equal() to distinguish calls that differ only
in null treatment -- which it already does, since WindowFunc.ignore_nulls
is a plain scalar field that _equalWindowFunc() compares. This fixes the
same bug your v2 fixed, but removes the hand-maintained shadow copy that
was the root cause, so it can't silently drift out of sync again.

Testing:
- Since the patch changes the layout of WindowStatePerFuncData, I also
built the whole tree with -fsanitize=alignment,undefined
-fno-sanitize-recover=alignment and re-ran make check under it: 245/245,
no runtime errors. The removed byte sat right before an 8-byte-aligned
pointer (winobj), so it was absorbed by padding and no following field
offset changes.

--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On Jul 15, 2026, at 14:27, Ewan Young <kdbase.hack@gmail.com> wrote:

On Wed, Jul 15, 2026 at 12:49 AM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 10, 2026, at 11:44, Ewan Young <kdbase.hack@gmail.com> wrote:

Hi Chao,

On Thu, Jul 9, 2026 at 8:35 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 7, 2026, at 08:14, Chao Li <li.evan.chao@gmail.com> 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.

Nice catch, and v2 does restore the intended de-duplication.

While reviewing it, though, I think the check the patch repairs is actually
redundant, and the real issue is that the code (both the original commit and
the patch) keeps a shadow copy of ignore_nulls that has to be maintained by
hand.

WindowFunc.ignore_nulls is a plain scalar field with no pg_node_attr, so
equal() already compares it -- the generated _equalWindowFunc() has:

COMPARE_SCALAR_FIELD(winagg);
COMPARE_SCALAR_FIELD(ignore_nulls);
COMPARE_LOCATION_FIELD(location);

That means the equal() call in the dedup loop already distinguishes two
WindowFuncs that differ only in null treatment:

for (i = 0; i <= wfuncno; i++)
{
if (equal(wfunc, perfunc[i].wfunc) &&
!contain_volatile_functions((Node *) wfunc))
break;
}
if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)

If ignore_nulls differs, equal() returns false, the loop never breaks on
that entry, and we never reach the extra term. If equal() matches, then
ignore_nulls necessarily matched too. So "&& wfunc->ignore_nulls ==
perfunc[i].ignore_nulls" can never change the outcome, and
WindowStatePerFuncData.ignore_nulls exists only to feed it -- it is read
only there and, before this patch, written nowhere (which is exactly why it
was always 0).

So rather than populating the field, I'd suggest dropping the redundant term
and the field, and letting equal() do the work:

and the field, and letting equal() do the work:

-   if (i <= wfuncno && wfunc->ignore_nulls == perfunc[i].ignore_nulls)
+   if (i <= wfuncno)
{
/* Found a match to an existing entry, so just mark it */
wfuncstate->wfuncno = i;
continue;
}

plus removing the ignore_nulls member from WindowStatePerFuncData and
trimming the now-stale "which needs the same ignore_nulls value" comment.
That fixes the same bug while removing the duplicated state that caused it,
so it can't silently drift again.

The one argument for an explicit check is defensiveness: if someone later
tags ignore_nulls with a pg_node_attr that excludes it from equal(), the
loop would start collapsing functions with different null treatment. If
that's a worry, the robust form compares the stored node directly instead of
a shadow copy, and still needs no separate field:

if (i <= wfuncno &&
wfunc->ignore_nulls == perfunc[i].wfunc->ignore_nulls)

Given ignore_nulls has to stay significant to equal() anyway (two calls with
different null treatment really are different functions), I'd lean toward
just removing the check.

Happy to send a patch along these lines if you agree.

I think your analysis is correct, please feel free to post your version.

Thanks, Chao. Patch attached.

It drops the redundant "&& wfunc->ignore_nulls == perfunc[i].ignore_nulls"
term together with WindowStatePerFuncData.ignore_nulls, and rewords the
now-stale comment, leaving equal() to distinguish calls that differ only
in null treatment -- which it already does, since WindowFunc.ignore_nulls
is a plain scalar field that _equalWindowFunc() compares. This fixes the
same bug your v2 fixed, but removes the hand-maintained shadow copy that
was the root cause, so it can't silently drift out of sync again.

Testing:
- Since the patch changes the layout of WindowStatePerFuncData, I also
built the whole tree with -fsanitize=alignment,undefined
-fno-sanitize-recover=alignment and re-ran make check under it: 245/245,
no runtime errors. The removed byte sat right before an 8-byte-aligned
pointer (winobj), so it was absorbed by padding and no following field
offset changes.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

--
Regards,
Ewan Young
<v2-0001-Remove-redundant-null-treatment-check-window-dedup.patch>

V2 overall looks good. One comment: I don’t think we need to add a comment explaining the equal() behavior for ignore_nulls.

The duplicate detection logic is legacy. Commit 25a30bbd4 intended to handle ignore_nulls specially, so it added “which needs the same ignore_nulls value” to the comment. Since equal() already handles ignore_nulls natively, the code change that added special handling for ignore_nulls has been reverted, so I think the comment should just be reverted as well.

PFA v3, reverted the comment.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Happy to send a patch along these lines if you agree.

I think your analysis is correct, please feel free to post your version.

Thanks, Chao. Patch attached.

It drops the redundant "&& wfunc->ignore_nulls == perfunc[i].ignore_nulls"
term together with WindowStatePerFuncData.ignore_nulls, and rewords the
now-stale comment, leaving equal() to distinguish calls that differ only
in null treatment -- which it already does, since WindowFunc.ignore_nulls
is a plain scalar field that _equalWindowFunc() compares. This fixes the
same bug your v2 fixed, but removes the hand-maintained shadow copy that
was the root cause, so it can't silently drift out of sync again.

Yeah, in nodeWindowAgg.c, actually ignore_nulls checking is performed
using WindowObject.ignore_nulls. I agree with your analysis.

Testing:
- Since the patch changes the layout of WindowStatePerFuncData, I also
built the whole tree with -fsanitize=alignment,undefined
-fno-sanitize-recover=alignment and re-ran make check under it: 245/245,
no runtime errors. The removed byte sat right before an 8-byte-aligned
pointer (winobj), so it was absorbed by padding and no following field
offset changes.

Thanks for the checking.

V2 overall looks good. One comment: I don’t think we need to add a comment explaining the equal() behavior for ignore_nulls.

The duplicate detection logic is legacy. Commit 25a30bbd4 intended to handle ignore_nulls specially, so it added “which needs the same ignore_nulls value” to the comment. Since equal() already handles ignore_nulls natively, the code change that added special handling for ignore_nulls has been reverted, so I think the comment should just be reverted as well.

Fair enough.

PFA v3, reverted the comment.

Looks good to me except the comment line needs indentation fixing. v4
patch attached. If there's no objection, I will commit it.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

On Jul 18, 2026, at 10:19, Tatsuo Ishii <ishii@postgresql.org> wrote:

Happy to send a patch along these lines if you agree.

I think your analysis is correct, please feel free to post your version.

Thanks, Chao. Patch attached.

It drops the redundant "&& wfunc->ignore_nulls == perfunc[i].ignore_nulls"
term together with WindowStatePerFuncData.ignore_nulls, and rewords the
now-stale comment, leaving equal() to distinguish calls that differ only
in null treatment -- which it already does, since WindowFunc.ignore_nulls
is a plain scalar field that _equalWindowFunc() compares. This fixes the
same bug your v2 fixed, but removes the hand-maintained shadow copy that
was the root cause, so it can't silently drift out of sync again.

Yeah, in nodeWindowAgg.c, actually ignore_nulls checking is performed
using WindowObject.ignore_nulls. I agree with your analysis.

Testing:
- Since the patch changes the layout of WindowStatePerFuncData, I also
built the whole tree with -fsanitize=alignment,undefined
-fno-sanitize-recover=alignment and re-ran make check under it: 245/245,
no runtime errors. The removed byte sat right before an 8-byte-aligned
pointer (winobj), so it was absorbed by padding and no following field
offset changes.

Thanks for the checking.

V2 overall looks good. One comment: I don’t think we need to add a comment explaining the equal() behavior for ignore_nulls.

The duplicate detection logic is legacy. Commit 25a30bbd4 intended to handle ignore_nulls specially, so it added “which needs the same ignore_nulls value” to the comment. Since equal() already handles ignore_nulls natively, the code change that added special handling for ignore_nulls has been reverted, so I think the comment should just be reverted as well.

Fair enough.

PFA v3, reverted the comment.

Looks good to me except the comment line needs indentation fixing. v4
patch attached. If there's no objection, I will commit it.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
<v4-0001-Remove-redundant-null-treatment-check-in-window-f.patch>

V4 looks good to me.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

V4 looks good to me.

Fix pushed to master and REL_19_STABLE branches.
Thank you!
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp