Thread

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

Tatsuo Ishii <ishii@postgresql.org> — 2025-10-16T10:17:06Z

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.
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp