Horton

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

Chao Li <li.evan.chao@gmail.com>

View thread
From: Chao Li <li.evan.chao@gmail.com>
To: Tatsuo Ishii <ishii@postgresql.org>
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
Date: 2025-10-16T11:50:06Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix coding style with "else".

  2. Fix multi WinGetFuncArgInFrame/Partition calls with IGNORE NULLS.

  3. Fix Coverity issue reported in commit 2273fa32bce.

  4. Use ereport rather than elog in WinCheckAndInitializeNullTreatment.

  5. Avoid uninitialized-variable warnings from older compilers.

  6. Fix Coverity issues reported in commit 25a30bbd423.

  7. Improve EXPLAIN's display of window functions.

  8. Automatically generate node support functions


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

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/