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