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

From: Tatsuo Ishii <ishii(at)postgresql(dot)org>
To: michael(at)paquier(dot)xyz
Cc: li(dot)evan(dot)chao(at)gmail(dot)com, tgl(at)sss(dot)pgh(dot)pa(dot)us, pramsey(at)cleverelephant(dot)ca, ojford(at)gmail(dot)com, peter(at)eisentraut(dot)org, krasiyan(at)gmail(dot)com, vik(at)postgresfriends(dot)org, andrew(at)tao11(dot)riddles(dot)org(dot)uk, david(at)fetter(dot)org, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: 2025-10-19 00:38:31
Message-ID: 20251019.093831.1684444745027170849.ishii@postgresql.org
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tatsuo Ishii 2025-10-19 01:02:58 Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Previous Message Corey Huinker 2025-10-19 00:32:24 Re: Import Statistics in postgres_fdw before resorting to sampling.