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

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>
To: Tatsuo Ishii <ishii@postgresql.org>
Cc: li.evan.chao@gmail.com, 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-16T06:19:21Z
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 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