Re: add more frame types in window functions (ROWS)

2009/12/7 Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>:
>>>>>> "Hitoshi" == Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
>
>  Hitoshi> Attached is updated version. I added AggGetMemoryContext()
>  Hitoshi> in executor/nodeAgg.h (though I'm not sure where to go...)
>  Hitoshi> and its second argument "iswindowagg" is output parameter to
>  Hitoshi> know whether the call context is Agg or WindowAgg. Your
>  Hitoshi> proposal of APIs to know whether the function is called as
>  Hitoshi> Aggregate or not is also a candidate to be, but it seems out
>  Hitoshi> of this patch scope, so it doesn't touch anything.
>
> I don't really like the extra argument; aggregate functions should
> almost never have to care about whether they're being called as window
> functions rather than aggregate functions. And if it does care, I
> don't see why this is the appropriate function for it. At the very
> least the function should accept NULL for the "iswindowagg" pointer to
> avoid useless variables in the caller.

I've just remembered such situation before, around here:
http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/array_userfuncs.c.diff?r1=1.30;r2=1.31

Of course it is fixed now. Still thinking about error reporting or
something, there should be a way to know which context you are called.
OK, I agree iswindowagg can be nullable, though most "isnull"
parameter cannot be NULL and forcing caller to put boolean stack value
don't seem a hard work.

> So for this and the regression test problem mentioned in the other mail,
> I'm setting this back to "waiting on author".

In my humble opinion, view regression test is not necessary in both my
patch and yours. At least window test has not been testing views so
far since it was introduced. Am I missing?

Regards,

--
Hitoshi Harada