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

> 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