public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: Tue, 07 Oct 2025 11:28:32 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
>> 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
Attachments:
[application/octet-stream] v2-0001-Fix-Coverity-issues-reported-in-commit-25a30bbd42.patch (4.4K, ../[email protected]/2-v2-0001-Fix-Coverity-issues-reported-in-commit-25a30bbd42.patch)
download | inline diff:
From daf0113128eab7a55baf53fc676f863fba3914e8 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Tue, 7 Oct 2025 11:22:51 +0900
Subject: [PATCH v2] Fix Coverity issues reported in commit 25a30bbd423.
This commit fixes several issues pointed out by Coverity.
- In row_is_in_frame(), return value of window_gettupleslot() was not
checked.
- WinGetFuncArgInPartition() tried to derefference "isout" pointer
even if it's NULL in some places.
Moreover, in WinGetFuncArgInPartition refactor the do...while loop so
that the codes inside the loop simpler. Also simplify the case when
abs_pos < 0.
Discussion: https://postgr.es/m/202510051612.gw67jlc2iqpw%40alvherre.pgsql
---
src/backend/executor/nodeWindowAgg.c | 77 ++++++++++++++--------------
1 file changed, 38 insertions(+), 39 deletions(-)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index cf667c81211..0698aae37a7 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -1501,8 +1501,9 @@ row_is_in_frame(WindowObject winobj, int64 pos, TupleTableSlot *slot,
/* following row that is not peer is out of frame */
if (pos > winstate->currentpos)
{
- if (fetch_tuple)
- window_gettupleslot(winobj, pos, slot);
+ if (fetch_tuple) /* need to fetch tuple? */
+ if (!window_gettupleslot(winobj, pos, slot))
+ return -1;
if (!are_peers(winstate, slot, winstate->ss.ss_ScanTupleSlot))
return -1;
}
@@ -3721,6 +3722,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
int notnull_offset;
int notnull_relpos;
int forward;
+ bool myisout;
Assert(WindowObjectIsValid(winobj));
winstate = winobj->winstate;
@@ -3759,63 +3761,60 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
if (!null_treatment) /* IGNORE NULLS is not specified */
{
+ /* get tupple and evaluate in a partition */
datum = gettuple_eval_partition(winobj, argno,
- abs_pos, isnull, isout);
- if (!*isout && set_mark)
+ abs_pos, isnull, &myisout);
+ if (!myisout && set_mark)
WinSetMarkPosition(winobj, abs_pos);
+ if (isout)
+ *isout = myisout;
return datum;
}
+ myisout = false;
+ datum = 0;
+
/*
* Get the next nonnull value in the partition, moving forward or backward
* until we find a value or reach the partition's end.
*/
do
{
+ int nn_info; /* NOT NULL info */
+
abs_pos += forward;
- if (abs_pos < 0)
- {
- /* out of partition */
- if (isout)
- *isout = true;
- *isnull = true;
- datum = 0;
+ if (abs_pos < 0) /* apparently out of partition */
break;
- }
- switch (get_notnull_info(winobj, abs_pos))
+ /* check NOT NULL cached info */
+ nn_info = get_notnull_info(winobj, abs_pos);
+ if (nn_info == NN_NOTNULL) /* this row is known to be NOT NULL */
+ notnull_offset++;
+
+ else if (nn_info == NN_NULL) /* this row is known to be NULL */
+ continue; /* keep on moving forward or backward */
+
+ else /* need to check NULL or not */
{
- case NN_NOTNULL: /* this row is known to be NOT NULL */
- notnull_offset++;
- if (notnull_offset >= notnull_relpos)
- {
- /* prepare to exit this loop */
- datum = gettuple_eval_partition(winobj, argno,
- abs_pos, isnull, isout);
- }
- break;
- case NN_NULL: /* this row is known to be NULL */
- if (isout)
- *isout = false;
- *isnull = true;
- datum = 0;
- break;
- default: /* need to check NULL or not */
- datum = gettuple_eval_partition(winobj, argno,
- abs_pos, isnull, isout);
- if (*isout) /* out of partition? */
- return datum;
-
- if (!*isnull)
- notnull_offset++;
- /* record the row status */
- put_notnull_info(winobj, abs_pos, *isnull);
+ /* get tupple and evaluate in a 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);
- if (!*isout && set_mark)
+ /* get tupple and evaluate in a partition */
+ datum = gettuple_eval_partition(winobj, argno,
+ abs_pos, isnull, &myisout);
+ if (!myisout && set_mark)
WinSetMarkPosition(winobj, abs_pos);
+ if (isout)
+ *isout = myisout;
return datum;
}
--
2.43.0
view thread (91+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox