public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tatsuo Ishii <[email protected]>
To: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
Date: Mon, 13 Oct 2025 13:43:32 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
>> 2. AFAICS there is only one notnull_info array, which amounts to
>> assuming that the window function will have only one argument position
>> that it calls WinGetFuncArgInFrame or WinGetFuncArgInPartition for.
>> That may be true for the built-in functions but it seems mighty
>> restrictive for extensions. Worse yet, there's no check, so that
>> you'd just get silently wrong answers if two or more arguments are
>> evaluated. I think there ought to be a separate array for each argno;
>> of course only created if the window function actually asks for
>> evaluations of a particular argno.
>
> I missed that. Thank you for pointed it out. I agree it would be
> better allow to use multiple argument positions that calls
> WinGetFuncArgInFrame or WinGetFuncArgInPartition in
> extensions. Attached is a PoC patch for that.
>
> Currently there's an issue with the patch, however.
>
> SELECT x, y, mywindowfunc2(x, y, 2) IGNORE NULLS OVER w FROM g
> WINDOW w AS (ORDER BY y);
> psql:test2.sql:9: ERROR: cannot fetch row before WindowObject's mark position
>
> mywindowfunc2 is a user defined window function, taking 3 arguments. x
> and y are expected to be evaluated to integer. The third argument is
> relative offset to current row. In the query above x and y are
> retrieved using two WinGetFuncArgInPartition() calls. The data set
> (table "g") looks like below.
>
> x | y
> ----+---
> | 1
> | 2
> 10 | 3
> 20 | 4
> (4 rows)
>
> I think the cause of the error is:
>
> (1) WinGetFuncArgInPartition keep on fetching column x until it's
> evalued to not null and placed in the second row (in this case that's
> x==20). In WinGetFuncArgInPartition WinSetMarkPosition is called at
> abs_pos==3.
>
> (2) WinGetFuncArgInPartition tries to fetch column y at row 0. Since
> the mark was set to at row 3, the error occurred.
>
> To avoid the error, we could call WinGetFuncArgInPartition with
> set_mark = false (and call WinSetMarkPosition separately) but I am not
> sure if it's an acceptable solution.
Attached is a v2 patch to fix the "cannot fetch row before
WindowObject's mark position" error, by tweaking the logic to
calculate the set mark position in WinGetFuncArgInPartition.
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Attachments:
[application/octet-stream] v2-0001-Allow-multiple-WinGetFuncArgInPartition-Frame-cal.patch (8.9K, ../[email protected]/2-v2-0001-Allow-multiple-WinGetFuncArgInPartition-Frame-cal.patch)
download | inline diff:
From 2913e8189adf9d75bdefef3c62ad83dabf2fa5cb Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Mon, 13 Oct 2025 13:39:04 +0900
Subject: [PATCH v2] Allow multiple WinGetFuncArgInPartition/Frame calls with
IGNORE NULLS option.
Previously it was assumed that there's only one call to
WinGetFuncArgInPartition/Frame in a window function when IGNORE NULLS
option is specified. To allow multiple calls to them,
winobj->notnull_info is modified from "uint8 *" to "uint8 **" so that
winobj->notnull_info could store pointers to not null info that
correspond to each function argument.
Also fix the set mark position logic in WinGetFuncArgInPartition to
not raise a "cannot fetch row before WindowObject's mark position"
error in IGNORE NULLS case.
---
src/backend/executor/nodeWindowAgg.c | 87 +++++++++++++++++++---------
1 file changed, 61 insertions(+), 26 deletions(-)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index e6a53f95391..d25ab51ca82 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -69,7 +69,7 @@ typedef struct WindowObjectData
int readptr; /* tuplestore read pointer for this fn */
int64 markpos; /* row that markptr is positioned on */
int64 seekpos; /* row that readptr is positioned on */
- uint8 *notnull_info; /* not null info */
+ uint8 **notnull_info; /* not null info for each func args */
int num_notnull_info; /* track size of the notnull_info array */
/*
@@ -214,10 +214,10 @@ static Datum ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
static Datum gettuple_eval_partition(WindowObject winobj, int argno,
int64 abs_pos, bool *isnull,
bool *isout);
-static void init_notnull_info(WindowObject winobj);
-static void grow_notnull_info(WindowObject winobj, int64 pos);
-static uint8 get_notnull_info(WindowObject winobj, int64 pos);
-static void put_notnull_info(WindowObject winobj, int64 pos, bool isnull);
+static void init_notnull_info(WindowObject winobj, WindowStatePerFunc perfuncstate);
+static void grow_notnull_info(WindowObject winobj, int64 pos, int argno);
+static uint8 get_notnull_info(WindowObject winobj, int64 pos, int argno);
+static void put_notnull_info(WindowObject winobj, int64 pos, int argno, bool isnull);
/*
* Not null info bit array consists of 2-bit items
@@ -1304,9 +1304,14 @@ begin_partition(WindowAggState *winstate)
/* reset null map */
if (winobj->ignore_nulls == IGNORE_NULLS)
- memset(winobj->notnull_info, 0,
- NN_POS_TO_BYTES(
- perfuncstate->winobj->num_notnull_info));
+ {
+ int numargs = perfuncstate->numArguments;
+
+ for (int j = 0; j < numargs; j++)
+ memset(winobj->notnull_info[j], 0,
+ NN_POS_TO_BYTES(
+ perfuncstate->winobj->num_notnull_info));
+ }
}
}
@@ -2734,7 +2739,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winobj->localmem = NULL;
perfuncstate->winobj = winobj;
winobj->ignore_nulls = wfunc->ignore_nulls;
- init_notnull_info(winobj);
+ init_notnull_info(winobj, perfuncstate);
/* It's a real window function, so set up to call it. */
fmgr_info_cxt(wfunc->winfnoid, &perfuncstate->flinfo,
@@ -3386,7 +3391,7 @@ ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
if (isout)
*isout = false;
- v = get_notnull_info(winobj, abs_pos);
+ v = get_notnull_info(winobj, abs_pos, argno);
if (v == NN_NULL) /* this row is known to be NULL */
goto advance;
@@ -3404,7 +3409,7 @@ ignorenulls_getfuncarginframe(WindowObject winobj, int argno,
notnull_offset++;
/* record the row status */
- put_notnull_info(winobj, abs_pos, *isnull);
+ put_notnull_info(winobj, abs_pos, argno, *isnull);
}
else /* this row is known to be NOT NULL */
{
@@ -3444,16 +3449,20 @@ out_of_frame:
* Initialize non null map.
*/
static void
-init_notnull_info(WindowObject winobj)
+init_notnull_info(WindowObject winobj, WindowStatePerFunc perfuncstate)
{
/* initial number of notnull info members */
#define INIT_NOT_NULL_INFO_NUM 128
+ int numargs = perfuncstate->numArguments;
if (winobj->ignore_nulls == PARSER_IGNORE_NULLS)
{
Size size = NN_POS_TO_BYTES(INIT_NOT_NULL_INFO_NUM);
- winobj->notnull_info = palloc0(size);
+ winobj->notnull_info = palloc(sizeof(uint8 *) * numargs);
+ for (int i = 0; i < numargs; i++)
+ /* allocate notnull_info for each argument */
+ winobj->notnull_info[i] = palloc0(size);
winobj->num_notnull_info = INIT_NOT_NULL_INFO_NUM;
}
}
@@ -3462,9 +3471,10 @@ init_notnull_info(WindowObject winobj)
* grow_notnull_info
* expand notnull_info if necessary.
* pos: not null info position
+ * argno: argument number
*/
static void
-grow_notnull_info(WindowObject winobj, int64 pos)
+grow_notnull_info(WindowObject winobj, int64 pos, int argno)
{
if (pos >= winobj->num_notnull_info)
{
@@ -3473,8 +3483,8 @@ grow_notnull_info(WindowObject winobj, int64 pos)
Size oldsize = NN_POS_TO_BYTES(winobj->num_notnull_info);
Size newsize = oldsize * 2;
- winobj->notnull_info =
- repalloc0(winobj->notnull_info, oldsize, newsize);
+ winobj->notnull_info[argno] =
+ repalloc0(winobj->notnull_info[argno], oldsize, newsize);
winobj->num_notnull_info = NN_BYTES_TO_POS(newsize);
if (winobj->num_notnull_info > pos)
break;
@@ -3486,16 +3496,19 @@ grow_notnull_info(WindowObject winobj, int64 pos)
* get_notnull_info
* retrieve a map
* pos: map position
+ * argno: argument number
*/
static uint8
-get_notnull_info(WindowObject winobj, int64 pos)
+get_notnull_info(WindowObject winobj, int64 pos, int argno)
{
+ uint8 *mbp;
uint8 mb;
int64 bpos;
- grow_notnull_info(winobj, pos);
+ grow_notnull_info(winobj, pos, argno);
bpos = NN_POS_TO_BYTES(pos);
- mb = winobj->notnull_info[bpos];
+ mbp = winobj->notnull_info[argno];
+ mb = mbp[bpos];
return (mb >> (NN_SHIFT(pos))) & NN_MASK;
}
@@ -3503,22 +3516,26 @@ get_notnull_info(WindowObject winobj, int64 pos)
* put_notnull_info
* update map
* pos: map position
+ * argno: argument number
+ * isnull: indicate NULL or NOT
*/
static void
-put_notnull_info(WindowObject winobj, int64 pos, bool isnull)
+put_notnull_info(WindowObject winobj, int64 pos, int argno, bool isnull)
{
+ uint8 *mbp;
uint8 mb;
int64 bpos;
uint8 val = isnull ? NN_NULL : NN_NOTNULL;
int shift;
- grow_notnull_info(winobj, pos);
+ grow_notnull_info(winobj, pos, argno);
bpos = NN_POS_TO_BYTES(pos);
- mb = winobj->notnull_info[bpos];
+ mbp = winobj->notnull_info[argno];
+ mb = mbp[bpos];
shift = NN_SHIFT(pos);
mb &= ~(NN_MASK << shift); /* clear map */
mb |= (val << shift); /* update map */
- winobj->notnull_info[bpos] = mb;
+ mbp[bpos] = mb;
}
/***********************************************************************
@@ -3717,6 +3734,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
{
WindowAggState *winstate;
int64 abs_pos;
+ int64 mark_pos;
Datum datum;
bool null_treatment;
int notnull_offset;
@@ -3772,6 +3790,23 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
forward = relpos > 0 ? 1 : -1;
myisout = false;
datum = 0;
+ /*
+ * IGNORE NULLS + WINDOW_SEEK_CURRENT + relpos > 0 case, we would fetch
+ * beyond the current row + relpos to find out the target row. If we mark
+ * at abs_pos, next call to WinGetFuncArgInPartition or
+ * WinGetFuncArgInFrame (in case when a window function have multiple
+ * args) could fail with "cannot fetch row before WindowObject's mark
+ * position". So keep the mark position at currentpos.
+ */
+ if (seektype == WINDOW_SEEK_CURRENT && relpos > 0)
+ mark_pos = winstate->currentpos;
+ else
+ /*
+ * For other cases we have no idea what position of row callers would
+ * fetch next time. Also for relpos < 0 case (we go backward), we
+ * cannot set mark either. For those cases we always set mark at 0.
+ */
+ mark_pos = 0;
/*
* Get the next nonnull value in the partition, moving forward or backward
@@ -3787,7 +3822,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
break;
/* check NOT NULL cached info */
- nn_info = get_notnull_info(winobj, abs_pos);
+ nn_info = get_notnull_info(winobj, abs_pos, argno);
if (nn_info == NN_NOTNULL) /* this row is known to be NOT NULL */
notnull_offset++;
else if (nn_info == NN_NULL) /* this row is known to be NULL */
@@ -3802,7 +3837,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
if (!*isnull)
notnull_offset++;
/* record the row status */
- put_notnull_info(winobj, abs_pos, *isnull);
+ put_notnull_info(winobj, abs_pos, argno, *isnull);
}
} while (notnull_offset < notnull_relpos);
@@ -3810,7 +3845,7 @@ WinGetFuncArgInPartition(WindowObject winobj, int argno,
datum = gettuple_eval_partition(winobj, argno,
abs_pos, isnull, &myisout);
if (!myisout && set_mark)
- WinSetMarkPosition(winobj, abs_pos);
+ WinSetMarkPosition(winobj, mark_pos);
if (isout)
*isout = myisout;
--
2.43.0
view thread (91+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox