Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

Tatsuo Ishii <ishii@postgresql.org> writes:
> Thank you for the review! I have just pushed the v2 patch.

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),
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.  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.  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.

Alternatively, you could just drop the entire concept of throwing an
error for that.  What's the point?  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.  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.

            regards, tom lane