Date: Thu, 16 Oct 2025 19:17:06 +0900 (JST)
Message-Id: <20251016.191706.731898842046838424.ishii@postgresql.org>
To: michael@paquier.xyz
Cc: li.evan.chao@gmail.com, tgl@sss.pgh.pa.us, pramsey@cleverelephant.ca,
 ojford@gmail.com, peter@eisentraut.org, krasiyan@gmail.com,
 vik@postgresfriends.org, andrew@tao11.riddles.org.uk, david@fetter.org,
 pgsql-hackers@lists.postgresql.org
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: <aPCOabSE4VfJLaky@paquier.xyz>
References: <20251013.143941.1447453701514209859.ishii@postgresql.org>
	<20251014.192113.1554227757626092006.ishii@postgresql.org>
	<aPCOabSE4VfJLaky@paquier.xyz>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Thu_Oct_16_19_17_06_2025_168)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20251016.191706.731898842046838424.ishii%40postgresql.org>
Precedence: bulk

----Next_Part(Thu_Oct_16_19_17_06_2025_168)--
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit

Thanks for the report.

> Coverity thinks that this code has still some incorrect bits, and I
> think that it is right to think so even on today's HEAD at
> 02c171f63fca.
> 
> In WinGetFuncArgInPartition()@nodeWindowAgg.c, we have the following
> loop (keeping only the relevant parts:
>     do
>     {
>         [...]
>         else                    /* need to check NULL or not */
>         {
>             /* get tuple and evaluate in partition */
>             datum = gettuple_eval_partition(winobj, argno,
>                                             abs_pos, isnull, &myisout);
>             if (myisout)        /* out of partition? */
>                 break;
>             if (!*isnull)
>                 notnull_offset++;
>             /* record the row status */
>             put_notnull_info(winobj, abs_pos, *isnull);
>         }
>     } while (notnull_offset < notnull_relpos);
> 
>     /* get tuple and evaluate in partition */
>     datum = gettuple_eval_partition(winobj, argno,
>                                     abs_pos, isnull, &myisout);
> 
> And Coverity is telling that there is no point in setting a datum in
> this else condition to just override its value when we exit the while
> loop.  To me, it's a sigh that this code's logic could be simplified.

To fix the issue, I think we can change:

>     datum = gettuple_eval_partition(winobj, argno,
>                                     abs_pos, isnull, &myisout);

to:

     (void) gettuple_eval_partition(winobj, argno,
                                           abs_pos, isnull, &myisout);

This explicitely stats that we ignore the return value from
gettuple_eval_partition. I hope coverity understands this.

> In passing, gettuple_eval_partition() is under-documented for me.  Its
> name refers to the fact that it gets a tuple and evaluates a
> partition.  Its top comment tells the same thing as the name of the
> function, so it's a bit hard to say why it is useful with the code
> written this way, and how others many benefit when attempting to reuse
> it, or if it even makes sense to reuse it for other purposes.

What about changing the comment this way?

/* gettuple_eval_partition
 * get tuple in a patition and evaluate the window function's argument
 * expression on it.
 */

Attached is the patch for above.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

----Next_Part(Thu_Oct_16_19_17_06_2025_168)--
Content-Type: Application/Octet-Stream
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="v1-0001-Fix-coverity-complaint-about-WinGetFuncArgInParti.patch"

RnJvbSBjMThkY2JjY2ZmN2JkN2M5NTI5NWJlZWRhYmIyMTE2Y2JiM2IzYmUyIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBUYXRzdW8gSXNoaWkgPGlzaGlpQHBvc3RncmVzcWwub3JnPgpE
YXRlOiBUaHUsIDE2IE9jdCAyMDI1IDE5OjAwOjU2ICswOTAwClN1YmplY3Q6IFtQQVRDSCB2MV0g
Rml4IGNvdmVyaXR5IGNvbXBsYWludCBhYm91dCBXaW5HZXRGdW5jQXJnSW5QYXJ0aXRpb24uCgpD
b3Zlcml0eSBjb21wbGFpbnMgdGhhdCB0aGUgcmV0dXJuIHZhbHVlIGZyb20gZ2V0dHVwbGVfZXZh
bF9wYXJ0aXRpb24KaW4gYSBkby4ud2hpbGUgbG9vcCBpbiBXaW5HZXRGdW5jQXJnSW5QYXJ0aXRp
b24gaXMgb3ZlcndyaXR0ZW4gd2hlbgpleGl0aW5nIHRoZSB3aGlsZSBsb29wLiBUaGlzIGNvbW1p
dCB0cmllcyB0byBmaXggdGhlIGlzc3VlIGJ5IGNoYW5naW5nCmdldHR1cGxlX2V2YWxfcGFydGl0
aW9uIGNhbGwgdG86Cgoodm9pZCkgZ2V0dHVwbGVfZXZhbF9wYXJ0aXRpb24oKQoKZXhwbGljaXRs
eSBzdGF0aW5nIHRoYXQgd2UgZGlzY2FyZCB0aGUgcmV0dXJuIHZhbHVlLgoKQWxzbyBlbmhhbmNl
IHNvbWUgY29tbWVudHMgZm9yIGVhc2llciBjb2RlIHJlYWRpbmcuCgpEaXNjdXNzaW9uOiBodHRw
czovL3Bvc3Rnci5lcy9tL2FQQ09hYlNFNFZmSkxha3klNDBwYXF1aWVyLnh5egotLS0KIHNyYy9i
YWNrZW5kL2V4ZWN1dG9yL25vZGVXaW5kb3dBZ2cuYyB8IDE5ICsrKysrKysrKysrKystLS0tLS0K
IDEgZmlsZSBjaGFuZ2VkLCAxMyBpbnNlcnRpb25zKCspLCA2IGRlbGV0aW9ucygtKQoKZGlmZiAt
LWdpdCBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVXaW5kb3dBZ2cuYyBiL3NyYy9iYWNrZW5k
L2V4ZWN1dG9yL25vZGVXaW5kb3dBZ2cuYwppbmRleCA0N2UwMGJlN2I0OS4uYWExNDVkNGUxYTkg
MTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVXaW5kb3dBZ2cuYworKysgYi9z
cmMvYmFja2VuZC9leGVjdXRvci9ub2RlV2luZG93QWdnLmMKQEAgLTMyNzAsOCArMzI3MCw5IEBA
IHdpbmRvd19nZXR0dXBsZXNsb3QoV2luZG93T2JqZWN0IHdpbm9iaiwgaW50NjQgcG9zLCBUdXBs
ZVRhYmxlU2xvdCAqc2xvdCkKIAlyZXR1cm4gdHJ1ZTsKIH0KIAotLyoKLSAqIGdldCB0dXBsZSBh
bmQgZXZhbHVhdGUgaW4gcGFydGl0aW9uCisvKiBnZXR0dXBsZV9ldmFsX3BhcnRpdGlvbgorICog
Z2V0IHR1cGxlIGluIGEgcGF0aXRpb24gYW5kIGV2YWx1YXRlIHRoZSB3aW5kb3cgZnVuY3Rpb24n
cyBhcmd1bWVudAorICogZXhwcmVzc2lvbiBvbiBpdC4KICAqLwogc3RhdGljIERhdHVtCiBnZXR0
dXBsZV9ldmFsX3BhcnRpdGlvbihXaW5kb3dPYmplY3Qgd2lub2JqLCBpbnQgYXJnbm8sCkBAIC0z
NzkwLDkgKzM3OTEsMTUgQEAgV2luR2V0RnVuY0FyZ0luUGFydGl0aW9uKFdpbmRvd09iamVjdCB3
aW5vYmosIGludCBhcmdubywKIAkJCWNvbnRpbnVlOwkJCS8qIGtlZXAgb24gbW92aW5nIGZvcndh
cmQgb3IgYmFja3dhcmQgKi8KIAkJZWxzZQkJCQkJLyogbmVlZCB0byBjaGVjayBOVUxMIG9yIG5v
dCAqLwogCQl7Ci0JCQkvKiBnZXQgdHVwbGUgYW5kIGV2YWx1YXRlIGluIHBhcnRpdGlvbiAqLwot
CQkJZGF0dW0gPSBnZXR0dXBsZV9ldmFsX3BhcnRpdGlvbih3aW5vYmosIGFyZ25vLAotCQkJCQkJ
CQkJCQlhYnNfcG9zLCBpc251bGwsICZteWlzb3V0KTsKKwkJCS8qCisJCQkgKiBOT1QgTlVMTCBp
bmZvIGRvZXMgbm90IGV4aXN0IHlldC4gIEdldCB0dXBsZSBhbmQgZXZhbHVhdGUgZnVuYworCQkJ
ICogYXJnIGluIHBhcnRpdGlvbi4gV2UgaWdub3JlIHRoZSByZXR1cm4gdmFsdWUgZnJvbQorCQkJ
ICogZ2V0dHVwbGVfZXZhbF9wYXJ0aXRpb24gYmVjYXVzZSB3ZSBhcmUganVzdCBpbnRlcmVzdGVk
IGluCisJCQkgKiB3aGV0aGVyIHdlIGFyZSBpbnNpZGUgb3Igb3V0c2lkZSBvZiBwYXJ0aXRpb24s
IE5VTEwgb3IgTk9UCisJCQkgKiBOVUxMLgorCQkJICovCisJCQkodm9pZCkgZ2V0dHVwbGVfZXZh
bF9wYXJ0aXRpb24od2lub2JqLCBhcmdubywKKwkJCQkJCQkJCQkgICBhYnNfcG9zLCBpc251bGws
ICZteWlzb3V0KTsKIAkJCWlmIChteWlzb3V0KQkJLyogb3V0IG9mIHBhcnRpdGlvbj8gKi8KIAkJ
CQlicmVhazsKIAkJCWlmICghKmlzbnVsbCkKQEAgLTM4MDIsNyArMzgwOSw3IEBAIFdpbkdldEZ1
bmNBcmdJblBhcnRpdGlvbihXaW5kb3dPYmplY3Qgd2lub2JqLCBpbnQgYXJnbm8sCiAJCX0KIAl9
IHdoaWxlIChub3RudWxsX29mZnNldCA8IG5vdG51bGxfcmVscG9zKTsKIAotCS8qIGdldCB0dXBs
ZSBhbmQgZXZhbHVhdGUgaW4gcGFydGl0aW9uICovCisJLyogZ2V0IHR1cGxlIGFuZCBldmFsdWF0
ZSBmdW5jIGFyZyBpbiBwYXJ0aXRpb24gKi8KIAlkYXR1bSA9IGdldHR1cGxlX2V2YWxfcGFydGl0
aW9uKHdpbm9iaiwgYXJnbm8sCiAJCQkJCQkJCQlhYnNfcG9zLCBpc251bGwsICZteWlzb3V0KTsK
IAlpZiAoIW15aXNvdXQgJiYgc2V0X21hcmspCi0tIAoyLjQzLjAKCg==

----Next_Part(Thu_Oct_16_19_17_06_2025_168)----