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: Sat, 11 Oct 2025 09:07:31 +0900 (JST)
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CACowWR0t-xgqtwrzW1LipyqXennDUnV3d0qysVHJghBiWZ3HWg@mail.gmail.com>
<[email protected]>
<[email protected]>
> While I'd paid basically zero attention to this patch (the claim
> in the commit message that I reviewed it is a flight of fancy),
Sorry, I added you as a reviewer because you had joined past
discussions regarding this feature. Next time I will add reviewers
only those who actually looked into a patch.
> I've been forced to look through it as a consequence of the mop-up
> that's been happening to silence compiler warnings. There are a
> couple of points that I think were not well done:
>
> 1. WinCheckAndInitializeNullTreatment really needs a rethink.
> You cannot realistically assume that existing user-defined window
> functions will be fixed to call that.
Currently if WinCheckAndInitializeNullTreatment is not called,
RESPECT/IGNORE NULLS option is disregarded and WinGetFuncArgInFrame or
WinGetFuncArgInPartition works as if RESPECT/IGNORE NULLS option is
not given. So I thought it's safe even if existing user-defined window
functions are not fixed.
> I think it should be set up
> so that if the window function fails to call that, then something in
> mainline execution of nodeWindowAgg.c throws an error when there had
> been a RESPECT/IGNORE NULLS option. With that idea, you could drop
> the allowNullTreatment argument and just have the window functions
> that support this syntax call something named along the lines of
> WinAllowNullTreatmentOption.
Does that mean all user defined window functions start to fail after
upgrading to PostgreSQL 19? I am not sure if it's acceptable for
extension developers and their users.
> Also the error is certainly user-facing,
> so using elog() was quite inappropriate. It should be ereport with an
> errcode of (probably) ERRCODE_FEATURE_NOT_SUPPORTED. Rolling your
> own implementation of get_func_name() wasn't great either.
I overlooked the elog() call and "own implementation of
get_func_name()". Will fix.
> Alternatively, you could just drop the entire concept of throwing an
> error for that. What's the point?
If we do that, extensions would need to be re-tested against IGNORE
NULLS option case. I might be wrong but I guess some of (or many of)
extension developers do not plan (or have no time to work on it for
now) to utilize IGNORE NULLS option for their extensions.
For buil-in window functions. I don't want to create test cases how
built-in window functions, that are not allowed IGNORE NULLS option,
behave against IGNORE NULLS option. Instead I prefer to throw an error
as it is done today.
> The implementation is entirely
> within nodeWindowAgg.c and does not depend in any way on the
> cooperation of the window function. I do not in any case like the
> documentation's wording
>
> + This option is only allowed for the following functions: <function>lag</function>,
> + <function>lead</function>, <function>first_value</function>, <function>last_value</function>,
> + <function>nth_value</function>.
> as this fails to account for the possibility of user-defined window
> functions.
The page explains only built-in window functions. Thus for me it's not
that strange that it does not say anything about user defined window
functions.
> IMO we could drop the error check altogether and rewrite
> the docs along the lines of "Not all window functions pay attention
> to this option. Of the built-in window functions, only blah blah
> and blah do."
>
> 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.
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] v1-0001-Allow-multiple-WinGetFuncArgInPartition-Frame-cal.patch (7.3K, ../[email protected]/2-v1-0001-Allow-multiple-WinGetFuncArgInPartition-Frame-cal.patch)
download | inline diff:
From 1a96b19b6f12c5a19ba11c9d4f5ea82f04733e39 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <[email protected]>
Date: Fri, 10 Oct 2025 20:53:17 +0900
Subject: [PATCH v1] 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.
---
src/backend/executor/nodeWindowAgg.c | 67 +++++++++++++++++-----------
1 file changed, 42 insertions(+), 25 deletions(-)
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index e6a53f95391..c7cb97a0643 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;
}
/***********************************************************************
@@ -3787,7 +3804,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 +3819,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);
--
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