Re: Fix duplicate detection for null-treatment window functions

>>>> 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