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
Description: Binary data
