> On Jul 15, 2026, at 14:27, Ewan Young <[email protected]> wrote: > > On Wed, Jul 15, 2026 at 12:49 AM Chao Li <[email protected]> wrote: >> >> >> >>> 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. > > 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/
v3-0001-Remove-redundant-null-treatment-check-in-window-f.patch
Description: Binary data
