Date: Tue, 07 Oct 2025 11:28:32 +0900 (JST)
Message-Id: <20251007.112832.740065769089328041.ishii@postgresql.org>
To: tgl@sss.pgh.pa.us
Cc: ojford@gmail.com, peter@eisentraut.org, li.evan.chao@gmail.com,
 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: <20251006.183439.876014721998277138.ishii@postgresql.org>
References: <1686755.1759679957@sss.pgh.pa.us>
	<20251006.085133.920304905412132984.ishii@postgresql.org>
	<20251006.183439.876014721998277138.ishii@postgresql.org>
Mime-Version: 1.0
Content-Type: Multipart/Mixed;
 boundary="--Next_Part(Tue_Oct__7_11_28_32_2025_209)--"
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20251007.112832.740065769089328041.ishii%40postgresql.org>
Precedence: bulk

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

>> Thank you for the report!
>> 
>>> Coverity is not very happy with this patch.
>>> It's complaining that the result of window_gettupleslot
>>> is not checked, which seems valid:
>>> 
>>> 1503     			{
>>> 1504     				if (fetch_tuple)
>>>>>>     CID 1666587:         Error handling issues  (CHECKED_RETURN)
>>>>>>     Calling "window_gettupleslot" without checking return value (as is done elsewhere 8 out of 9 times).
>>> 1505     					window_gettupleslot(winobj, pos, slot);
>>> 1506     				if (!are_peers(winstate, slot, winstate->ss.ss_ScanTupleSlot))
>>> 1507     					return -1;
>> 
>> Yes, I forgot to check the return value of window_gettupleslot.
>> 
>>> and also that WinGetFuncArgInPartition is dereferencing
>>> a possibly-null "isout" pointer at several places, including
>>> 
>>>>>>     Dereferencing null pointer "isout".
>>> 3806     				if (*isout)		/* out of partition? */
>>> 
>>>>>>     Dereferencing null pointer "isout".
>>> 3817     	if (!*isout && set_mark)
>>> 3818     		WinSetMarkPosition(winobj, abs_pos);
>>> 
>>>>>>     Dereferencing null pointer "isout".
>>> 3817     	if (!*isout && set_mark)
>>> 3818     		WinSetMarkPosition(winobj, abs_pos);
>>> 
>>> The latter complaints seem to be because some places in
>>> WinGetFuncArgInPartition check for nullness of that pointer
>>> and some do not.  That looks like at least a latent bug
>>> to me.
>> 
>> Agreed.
>> 
>> Attached is a patch to fix the issue.
> 
> Please disregard the v1 patch. It includes a bug: If
> WinGetFuncArgInPartition() is called with set_mark == true and isout
> == NULL, WinSetMarkPosition() is not called by
> WinGetFuncArgInPartition().
> 
> I will post v2 patch.

Attached is the v2 patch.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

----Next_Part(Tue_Oct__7_11_28_32_2025_209)--
Content-Type: Application/Octet-Stream
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="v2-0001-Fix-Coverity-issues-reported-in-commit-25a30bbd42.patch"

RnJvbSBkYWYwMTEzMTI4ZWFiN2E1NWJhZjUzZmM2NzZmODYzZmJhMzkxNGU4IE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBUYXRzdW8gSXNoaWkgPGlzaGlpQHBvc3RncmVzcWwub3JnPgpE
YXRlOiBUdWUsIDcgT2N0IDIwMjUgMTE6MjI6NTEgKzA5MDAKU3ViamVjdDogW1BBVENIIHYyXSBG
aXggQ292ZXJpdHkgaXNzdWVzIHJlcG9ydGVkIGluIGNvbW1pdCAyNWEzMGJiZDQyMy4KClRoaXMg
Y29tbWl0IGZpeGVzIHNldmVyYWwgaXNzdWVzIHBvaW50ZWQgb3V0IGJ5IENvdmVyaXR5LgoKLSBJ
biByb3dfaXNfaW5fZnJhbWUoKSwgcmV0dXJuIHZhbHVlIG9mIHdpbmRvd19nZXR0dXBsZXNsb3Qo
KSB3YXMgbm90CiAgY2hlY2tlZC4KCi0gV2luR2V0RnVuY0FyZ0luUGFydGl0aW9uKCkgdHJpZWQg
dG8gZGVyZWZmZXJlbmNlICJpc291dCIgcG9pbnRlcgogIGV2ZW4gaWYgaXQncyBOVUxMIGluIHNv
bWUgcGxhY2VzLgoKTW9yZW92ZXIsIGluIFdpbkdldEZ1bmNBcmdJblBhcnRpdGlvbiByZWZhY3Rv
ciB0aGUgZG8uLi53aGlsZSBsb29wIHNvCnRoYXQgdGhlIGNvZGVzIGluc2lkZSB0aGUgbG9vcCBz
aW1wbGVyLiBBbHNvIHNpbXBsaWZ5IHRoZSBjYXNlIHdoZW4KYWJzX3BvcyA8IDAuCgpEaXNjdXNz
aW9uOiBodHRwczovL3Bvc3Rnci5lcy9tLzIwMjUxMDA1MTYxMi5ndzY3amxjMmlxcHclNDBhbHZo
ZXJyZS5wZ3NxbAotLS0KIHNyYy9iYWNrZW5kL2V4ZWN1dG9yL25vZGVXaW5kb3dBZ2cuYyB8IDc3
ICsrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0KIDEgZmlsZSBjaGFuZ2VkLCAzOCBpbnNlcnRp
b25zKCspLCAzOSBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9zcmMvYmFja2VuZC9leGVjdXRv
ci9ub2RlV2luZG93QWdnLmMgYi9zcmMvYmFja2VuZC9leGVjdXRvci9ub2RlV2luZG93QWdnLmMK
aW5kZXggY2Y2NjdjODEyMTEuLjA2OThhYWUzN2E3IDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9l
eGVjdXRvci9ub2RlV2luZG93QWdnLmMKKysrIGIvc3JjL2JhY2tlbmQvZXhlY3V0b3Ivbm9kZVdp
bmRvd0FnZy5jCkBAIC0xNTAxLDggKzE1MDEsOSBAQCByb3dfaXNfaW5fZnJhbWUoV2luZG93T2Jq
ZWN0IHdpbm9iaiwgaW50NjQgcG9zLCBUdXBsZVRhYmxlU2xvdCAqc2xvdCwKIAkJCS8qIGZvbGxv
d2luZyByb3cgdGhhdCBpcyBub3QgcGVlciBpcyBvdXQgb2YgZnJhbWUgKi8KIAkJCWlmIChwb3Mg
PiB3aW5zdGF0ZS0+Y3VycmVudHBvcykKIAkJCXsKLQkJCQlpZiAoZmV0Y2hfdHVwbGUpCi0JCQkJ
CXdpbmRvd19nZXR0dXBsZXNsb3Qod2lub2JqLCBwb3MsIHNsb3QpOworCQkJCWlmIChmZXRjaF90
dXBsZSkJLyogbmVlZCB0byBmZXRjaCB0dXBsZT8gKi8KKwkJCQkJaWYgKCF3aW5kb3dfZ2V0dHVw
bGVzbG90KHdpbm9iaiwgcG9zLCBzbG90KSkKKwkJCQkJCXJldHVybiAtMTsKIAkJCQlpZiAoIWFy
ZV9wZWVycyh3aW5zdGF0ZSwgc2xvdCwgd2luc3RhdGUtPnNzLnNzX1NjYW5UdXBsZVNsb3QpKQog
CQkJCQlyZXR1cm4gLTE7CiAJCQl9CkBAIC0zNzIxLDYgKzM3MjIsNyBAQCBXaW5HZXRGdW5jQXJn
SW5QYXJ0aXRpb24oV2luZG93T2JqZWN0IHdpbm9iaiwgaW50IGFyZ25vLAogCWludAkJCW5vdG51
bGxfb2Zmc2V0OwogCWludAkJCW5vdG51bGxfcmVscG9zOwogCWludAkJCWZvcndhcmQ7CisJYm9v
bAkJbXlpc291dDsKIAogCUFzc2VydChXaW5kb3dPYmplY3RJc1ZhbGlkKHdpbm9iaikpOwogCXdp
bnN0YXRlID0gd2lub2JqLT53aW5zdGF0ZTsKQEAgLTM3NTksNjMgKzM3NjEsNjAgQEAgV2luR2V0
RnVuY0FyZ0luUGFydGl0aW9uKFdpbmRvd09iamVjdCB3aW5vYmosIGludCBhcmdubywKIAogCWlm
ICghbnVsbF90cmVhdG1lbnQpCQkvKiBJR05PUkUgTlVMTFMgaXMgbm90IHNwZWNpZmllZCAqLwog
CXsKKwkJLyogZ2V0IHR1cHBsZSBhbmQgZXZhbHVhdGUgaW4gYSBwYXJ0aXRpb24gKi8KIAkJZGF0
dW0gPSBnZXR0dXBsZV9ldmFsX3BhcnRpdGlvbih3aW5vYmosIGFyZ25vLAotCQkJCQkJCQkJCWFi
c19wb3MsIGlzbnVsbCwgaXNvdXQpOwotCQlpZiAoISppc291dCAmJiBzZXRfbWFyaykKKwkJCQkJ
CQkJCQlhYnNfcG9zLCBpc251bGwsICZteWlzb3V0KTsKKwkJaWYgKCFteWlzb3V0ICYmIHNldF9t
YXJrKQogCQkJV2luU2V0TWFya1Bvc2l0aW9uKHdpbm9iaiwgYWJzX3Bvcyk7CisJCWlmIChpc291
dCkKKwkJCSppc291dCA9IG15aXNvdXQ7CiAJCXJldHVybiBkYXR1bTsKIAl9CiAKKwlteWlzb3V0
ID0gZmFsc2U7CisJZGF0dW0gPSAwOworCiAJLyoKIAkgKiBHZXQgdGhlIG5leHQgbm9ubnVsbCB2
YWx1ZSBpbiB0aGUgcGFydGl0aW9uLCBtb3ZpbmcgZm9yd2FyZCBvciBiYWNrd2FyZAogCSAqIHVu
dGlsIHdlIGZpbmQgYSB2YWx1ZSBvciByZWFjaCB0aGUgcGFydGl0aW9uJ3MgZW5kLgogCSAqLwog
CWRvCiAJeworCQlpbnQJCQlubl9pbmZvOwkvKiBOT1QgTlVMTCBpbmZvICovCisKIAkJYWJzX3Bv
cyArPSBmb3J3YXJkOwotCQlpZiAoYWJzX3BvcyA8IDApCi0JCXsKLQkJCS8qIG91dCBvZiBwYXJ0
aXRpb24gKi8KLQkJCWlmIChpc291dCkKLQkJCQkqaXNvdXQgPSB0cnVlOwotCQkJKmlzbnVsbCA9
IHRydWU7Ci0JCQlkYXR1bSA9IDA7CisJCWlmIChhYnNfcG9zIDwgMCkJCS8qIGFwcGFyZW50bHkg
b3V0IG9mIHBhcnRpdGlvbiAqLwogCQkJYnJlYWs7Ci0JCX0KIAotCQlzd2l0Y2ggKGdldF9ub3Ru
dWxsX2luZm8od2lub2JqLCBhYnNfcG9zKSkKKwkJLyogY2hlY2sgTk9UIE5VTEwgY2FjaGVkIGlu
Zm8gKi8KKwkJbm5faW5mbyA9IGdldF9ub3RudWxsX2luZm8od2lub2JqLCBhYnNfcG9zKTsKKwkJ
aWYgKG5uX2luZm8gPT0gTk5fTk9UTlVMTCkJLyogdGhpcyByb3cgaXMga25vd24gdG8gYmUgTk9U
IE5VTEwgKi8KKwkJCW5vdG51bGxfb2Zmc2V0Kys7CisKKwkJZWxzZSBpZiAobm5faW5mbyA9PSBO
Tl9OVUxMKQkvKiB0aGlzIHJvdyBpcyBrbm93biB0byBiZSBOVUxMICovCisJCQljb250aW51ZTsJ
CQkvKiBrZWVwIG9uIG1vdmluZyBmb3J3YXJkIG9yIGJhY2t3YXJkICovCisKKwkJZWxzZQkJCQkJ
LyogbmVlZCB0byBjaGVjayBOVUxMIG9yIG5vdCAqLwogCQl7Ci0JCQljYXNlIE5OX05PVE5VTEw6
CS8qIHRoaXMgcm93IGlzIGtub3duIHRvIGJlIE5PVCBOVUxMICovCi0JCQkJbm90bnVsbF9vZmZz
ZXQrKzsKLQkJCQlpZiAobm90bnVsbF9vZmZzZXQgPj0gbm90bnVsbF9yZWxwb3MpCi0JCQkJewot
CQkJCQkvKiBwcmVwYXJlIHRvIGV4aXQgdGhpcyBsb29wICovCi0JCQkJCWRhdHVtID0gZ2V0dHVw
bGVfZXZhbF9wYXJ0aXRpb24od2lub2JqLCBhcmdubywKLQkJCQkJCQkJCQkJCQlhYnNfcG9zLCBp
c251bGwsIGlzb3V0KTsKLQkJCQl9Ci0JCQkJYnJlYWs7Ci0JCQljYXNlIE5OX05VTEw6CQkvKiB0
aGlzIHJvdyBpcyBrbm93biB0byBiZSBOVUxMICovCi0JCQkJaWYgKGlzb3V0KQotCQkJCQkqaXNv
dXQgPSBmYWxzZTsKLQkJCQkqaXNudWxsID0gdHJ1ZTsKLQkJCQlkYXR1bSA9IDA7Ci0JCQkJYnJl
YWs7Ci0JCQlkZWZhdWx0OgkJCS8qIG5lZWQgdG8gY2hlY2sgTlVMTCBvciBub3QgKi8KLQkJCQlk
YXR1bSA9IGdldHR1cGxlX2V2YWxfcGFydGl0aW9uKHdpbm9iaiwgYXJnbm8sCi0JCQkJCQkJCQkJ
CQlhYnNfcG9zLCBpc251bGwsIGlzb3V0KTsKLQkJCQlpZiAoKmlzb3V0KQkJLyogb3V0IG9mIHBh
cnRpdGlvbj8gKi8KLQkJCQkJcmV0dXJuIGRhdHVtOwotCi0JCQkJaWYgKCEqaXNudWxsKQotCQkJ
CQlub3RudWxsX29mZnNldCsrOwotCQkJCS8qIHJlY29yZCB0aGUgcm93IHN0YXR1cyAqLwotCQkJ
CXB1dF9ub3RudWxsX2luZm8od2lub2JqLCBhYnNfcG9zLCAqaXNudWxsKTsKKwkJCS8qIGdldCB0
dXBwbGUgYW5kIGV2YWx1YXRlIGluIGEgcGFydGl0aW9uICovCisJCQlkYXR1bSA9IGdldHR1cGxl
X2V2YWxfcGFydGl0aW9uKHdpbm9iaiwgYXJnbm8sCisJCQkJCQkJCQkJCWFic19wb3MsIGlzbnVs
bCwgJm15aXNvdXQpOworCQkJaWYgKG15aXNvdXQpCQkvKiBvdXQgb2YgcGFydGl0aW9uPyAqLwog
CQkJCWJyZWFrOworCQkJaWYgKCEqaXNudWxsKQorCQkJCW5vdG51bGxfb2Zmc2V0Kys7CisJCQkv
KiByZWNvcmQgdGhlIHJvdyBzdGF0dXMgKi8KKwkJCXB1dF9ub3RudWxsX2luZm8od2lub2JqLCBh
YnNfcG9zLCAqaXNudWxsKTsKIAkJfQogCX0gd2hpbGUgKG5vdG51bGxfb2Zmc2V0IDwgbm90bnVs
bF9yZWxwb3MpOwogCi0JaWYgKCEqaXNvdXQgJiYgc2V0X21hcmspCisJLyogZ2V0IHR1cHBsZSBh
bmQgZXZhbHVhdGUgaW4gYSBwYXJ0aXRpb24gKi8KKwlkYXR1bSA9IGdldHR1cGxlX2V2YWxfcGFy
dGl0aW9uKHdpbm9iaiwgYXJnbm8sCisJCQkJCQkJCQlhYnNfcG9zLCBpc251bGwsICZteWlzb3V0
KTsKKwlpZiAoIW15aXNvdXQgJiYgc2V0X21hcmspCiAJCVdpblNldE1hcmtQb3NpdGlvbih3aW5v
YmosIGFic19wb3MpOworCWlmIChpc291dCkKKwkJKmlzb3V0ID0gbXlpc291dDsKIAogCXJldHVy
biBkYXR1bTsKIH0KLS0gCjIuNDMuMAoK

----Next_Part(Tue_Oct__7_11_28_32_2025_209)----