On Tue, Oct 14, 2025 at 07:21:13PM +0900, Tatsuo Ishii wrote:
> V2 patch pushed. Thanks.
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.
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.
--
Michael
