Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options - Mailing list pgsql-hackers

Thanks for updating the patch.

>> It seems you allow to use IGNORE NULLS for all window functions. If
>> the case, you should explicitely stat that in the docs. Otherwise
>> users will be confused because;
> 
> The latest version restricts it to lag, lead, first_value, last_value,
> and nth_value. We can extend it in a subsequent patch if there's
> demand?

The restriction is required by the SQL standard. So I don't think we
need to extend to other window functions.

>> I take a look at the patch and noticed that following functions have
>> no comments on what they are doing and what are the arguments. Please
>> look into other functions in nodeWindowAgg.c and add appropriate
>> comments to those functions.
> 
> Latest version has more comments and should be in the standard coding style.

Still I see non standard coding stiles and indentations. See attached
patch for nodeWindowAgg.c, which is fixed by pgindent, for
example. (Other files may need fixing too).

>> I also notice that you have an array in memory which records non-null
>> row positions in a partition. The position is represented in int64,
>> which means 1 entry consumes 8 bytes. If my understanding is correct,
>> the array continues to grow up to the partition size. Also the array
>> is created for each window function (is it really necessary?). I worry
>> about this because it might consume excessive memory for big
>> partitions.
> 
> It's an int64 because it stores the abs_pos/mark_pos which are int64.
> Keeping an array for each function is needed for the mark optimization
> to work correctly.

Ok.

Here are some comments regarding the patch:

(1) I noticed that ignorenulls_getfuncarginframe() does not take
account EXCLUSION frame options. The code path is in
WinGetFuncArgInFrame():

            /*
             * Account for exclusion option if one is active, but advance only
             * abs_pos not mark_pos.  This prevents changes of the current
             * row's peer group from resulting in trying to fetch a row before
             * some previous mark position.
:
:

I guess ignorenulls_getfuncarginframe() was created by modifying
WinGetFuncArgInFrame() so I don't see the reason why
ignorenulls_getfuncarginframe() does not take account EXCLUSION frame
options.

(2) New member ignore_nulls are added to some structs. Its value is 0,
1 or -1. It's better to use a DEFINE for the value of ignore_nulls,
rather than 0, 1, or -1.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index e1117857dc0..520e7e1bfcb 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -2624,7 +2624,10 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
             elog(ERROR, "WindowFunc with winref %u assigned to WindowAgg with winref %u",
                  wfunc->winref, node->winref);
 
-        /* Look for a previous duplicate window function, which needs the same ignore_nulls value */
+        /*
+         * Look for a previous duplicate window function, which needs the same
+         * ignore_nulls value
+         */
         for (i = 0; i <= wfuncno; i++)
         {
             if (equal(wfunc, perfunc[i].wfunc) &&
@@ -3379,8 +3382,8 @@ increment_nonnulls(WindowObject winobj, int64 pos)
         winobj->nonnulls_size *= 2;
         winobj->win_nonnulls =
             repalloc_array(winobj->win_nonnulls,
-                            int64,
-                            winobj->nonnulls_size);
+                           int64,
+                           winobj->nonnulls_size);
     }
     winobj->win_nonnulls[winobj->nonnulls_len] = pos;
     winobj->nonnulls_len++;
@@ -3394,7 +3397,8 @@ increment_nonnulls(WindowObject winobj, int64 pos)
 static Datum
 ignorenulls_getfuncarginpartition(WindowObject winobj, int argno,
                                   int relpos, int seektype, bool set_mark,
-                                  bool *isnull, bool *isout) {
+                                  bool *isnull, bool *isout)
+{
     WindowAggState *winstate;
     ExprContext *econtext;
     TupleTableSlot *slot;
@@ -3416,27 +3420,27 @@ ignorenulls_getfuncarginpartition(WindowObject winobj, int argno,
 
     switch (seektype)
     {
-    case WINDOW_SEEK_CURRENT:
-        abs_pos = winstate->currentpos;
-        break;
-    case WINDOW_SEEK_HEAD:
-        abs_pos = 0;
-        break;
-    case WINDOW_SEEK_TAIL:
-        spool_tuples(winstate, -1);
-        abs_pos = winstate->spooled_rows - 1;
-        break;
-    default:
-        elog(ERROR, "unrecognized window seek type: %d", seektype);
-        abs_pos = 0; /* keep compiler quiet */
-        break;
+        case WINDOW_SEEK_CURRENT:
+            abs_pos = winstate->currentpos;
+            break;
+        case WINDOW_SEEK_HEAD:
+            abs_pos = 0;
+            break;
+        case WINDOW_SEEK_TAIL:
+            spool_tuples(winstate, -1);
+            abs_pos = winstate->spooled_rows - 1;
+            break;
+        default:
+            elog(ERROR, "unrecognized window seek type: %d", seektype);
+            abs_pos = 0;        /* keep compiler quiet */
+            break;
     }
 
     if (forward == -1)
         goto check_partition;
 
     /* if we're moving forward, store previous rows */
-    for (i=0; i < winobj->nonnulls_len; ++i)
+    for (i = 0; i < winobj->nonnulls_len; ++i)
     {
         if (winobj->win_nonnulls[i] > abs_pos)
         {
@@ -3448,7 +3452,7 @@ ignorenulls_getfuncarginpartition(WindowObject winobj, int argno,
                     *isout = false;
                 window_gettupleslot(winobj, abs_pos, slot);
                 econtext->ecxt_outertuple = slot;
-                return ExecEvalExpr((ExprState *)list_nth(winobj->argstates, argno),
+                return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
                                     econtext, isnull);
             }
         }
@@ -3465,13 +3469,13 @@ check_partition:
             if (isout)
                 *isout = true;
             *isnull = true;
-            return (Datum)0;
+            return (Datum) 0;
         }
 
         if (isout)
             *isout = false;
         econtext->ecxt_outertuple = slot;
-        datum = ExecEvalExpr((ExprState *)list_nth(winobj->argstates, argno),
+        datum = ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
                              econtext, isnull);
 
         if (!*isnull)
@@ -3494,7 +3498,8 @@ check_partition:
 static Datum
 ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
                               int relpos, int seektype, bool set_mark,
-                              bool *isnull, bool *isout) {
+                              bool *isnull, bool *isout)
+{
     WindowAggState *winstate;
     ExprContext *econtext;
     TupleTableSlot *slot;
@@ -3511,7 +3516,7 @@ ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
     winstate = winobj->winstate;
     econtext = winstate->ss.ps.ps_ExprContext;
     slot = winstate->temp_slot_1;
-    datum = (Datum)0;
+    datum = (Datum) 0;
     notnull_offset = 0;
     notnull_relpos = abs(relpos);
 
@@ -3549,70 +3554,72 @@ ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
      */
     for (i = 0; i < winobj->nonnulls_len; ++i)
     {
-            int inframe;
-            if (winobj->win_nonnulls[i] < winobj->markpos)
-                continue;
-            if (!window_gettupleslot(winobj, winobj->win_nonnulls[i], slot))
-                continue;
+        int            inframe;
 
-            inframe = row_is_in_frame(winstate, winobj->win_nonnulls[i], slot);
-            if (inframe <= 0)
-            {
-                if (inframe == -1 && set_mark)
-                    WinSetMarkPosition(winobj, winobj->win_nonnulls[i]);
-                continue;
-            }
+        if (winobj->win_nonnulls[i] < winobj->markpos)
+            continue;
+        if (!window_gettupleslot(winobj, winobj->win_nonnulls[i], slot))
+            continue;
 
-            abs_pos = winobj->win_nonnulls[i] + 1;
-            ++notnull_offset;
+        inframe = row_is_in_frame(winstate, winobj->win_nonnulls[i], slot);
+        if (inframe <= 0)
+        {
+            if (inframe == -1 && set_mark)
+                WinSetMarkPosition(winobj, winobj->win_nonnulls[i]);
+            continue;
+        }
 
-            if (notnull_offset > notnull_relpos)
-            {
-                if (isout)
+        abs_pos = winobj->win_nonnulls[i] + 1;
+        ++notnull_offset;
+
+        if (notnull_offset > notnull_relpos)
+        {
+            if (isout)
                 *isout = false;
-                econtext->ecxt_outertuple = slot;
-                return ExecEvalExpr((ExprState *)list_nth(winobj->argstates, argno),
-                                    econtext, isnull);
-            }
+            econtext->ecxt_outertuple = slot;
+            return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
+                                econtext, isnull);
+        }
     }
 
 check_frame:
     do
     {
-            int inframe;
-            if (!window_gettupleslot(winobj, abs_pos, slot))
-                goto out_of_frame;
+        int            inframe;
 
-            inframe = row_is_in_frame(winstate, abs_pos, slot);
-            if (inframe == -1)
-                goto out_of_frame;
-            else if (inframe == 0)
-                goto advance;
+        if (!window_gettupleslot(winobj, abs_pos, slot))
+            goto out_of_frame;
 
-            gottuple = window_gettupleslot(winobj, abs_pos, slot);
+        inframe = row_is_in_frame(winstate, abs_pos, slot);
+        if (inframe == -1)
+            goto out_of_frame;
+        else if (inframe == 0)
+            goto advance;
 
-            if (!gottuple)
-            {
-                if (isout)
-                    *isout = true;
-                *isnull = true;
-                return (Datum)0;
-            }
+        gottuple = window_gettupleslot(winobj, abs_pos, slot);
 
+        if (!gottuple)
+        {
             if (isout)
-                *isout = false;
-            econtext->ecxt_outertuple = slot;
-            datum = ExecEvalExpr((ExprState *)list_nth(winobj->argstates, argno),
-                                 econtext, isnull);
+                *isout = true;
+            *isnull = true;
+            return (Datum) 0;
+        }
 
-            if (!*isnull)
-            {
-                ++notnull_offset;
-                increment_nonnulls(winobj, abs_pos);
-            }
+        if (isout)
+            *isout = false;
+        econtext->ecxt_outertuple = slot;
+        datum = ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
+                             econtext, isnull);
+
+        if (!*isnull)
+        {
+            ++notnull_offset;
+            increment_nonnulls(winobj, abs_pos);
+        }
 
 advance:
-            abs_pos += forward;
+        abs_pos += forward;
     } while (notnull_offset <= notnull_relpos);
 
     return datum;
@@ -3660,7 +3667,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
 
     if (winobj->ignore_nulls == 1 && relpos != 0)
         return ignorenulls_getfuncarginpartition(winobj, argno, relpos, seektype,
-                                                    set_mark, isnull, isout);
+                                                 set_mark, isnull, isout);
 
     switch (seektype)
     {
@@ -3752,7 +3759,7 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
 
     if (winobj->ignore_nulls == 1)
         return ignorenulls_getfuncarginframe(winobj, argno, relpos, seektype,
-                                                set_mark, isnull, isout);
+                                             set_mark, isnull, isout);
 
     switch (seektype)
     {