public inbox for [email protected]  

help / color / mirror / Atom feed

From: 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 09:38:31 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>

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

Patch pushed with minor comment tweaks.
Thanks.
--
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