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)
{
