> On Jul 10, 2026, at 11:44, Ewan Young <[email protected]> wrote:
>
> Hi Chao,
>
>
> On Thu, Jul 9, 2026 at 8:35 PM Chao Li <[email protected]> wrote:
>>
>>
>>
>>> 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.
>
> 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/
