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]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: Sun, 19 Oct 2025 10:02:58 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
>> On Oct 16, 2025, at 18:17, Tatsuo Ishii <[email protected]> wrote:
>>
>> 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.
>>
>>>
>
> I think Coverity is complaining about the redundant call to gettuple_eval_partition().
>
> In the “else” clause, the function is called, then when “if (myisout)” is satisfied, it will break out the while loop. After that, the function is immediately called again, so “datum” is overwritten. But I haven’t spent time thinking about how to fix.
Yes, the function is called again. But I think the cost is cheap in
this case. Inside the function window_gettupleslot() is called. It
could be costly if it spools tuples. But as tuple is already spooled
by the former call of gettuple_eval_partition(), almost no cost is
needed. We could avoid the redundant call by putting more code after
the former function call to return immediately, or introduce a goto
statement or a flag. But I think they will make the code harder to
read and do not worth the trouble. Others may think differently
though.
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
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], [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