Date: Sun, 19 Oct 2025 10:02:58 +0900 (JST)
Message-Id: <20251019.100258.1857152982068276203.ishii@postgresql.org>
To: li.evan.chao@gmail.com
Cc: michael@paquier.xyz, 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: <24DE0F3D-369B-4421-9178-FCF9A8D4B4E7@gmail.com>
References: <aPCOabSE4VfJLaky@paquier.xyz>
	<20251016.191706.731898842046838424.ishii@postgresql.org>
	<24DE0F3D-369B-4421-9178-FCF9A8D4B4E7@gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=iso-2022-jp
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20251019.100258.1857152982068276203.ishii%40postgresql.org>
Precedence: bulk

>> On Oct 16, 2025, at 18:17, Tatsuo Ishii <ishii@postgresql.org> 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