Re: review: More frame options in window functions

Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> 2010/1/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> Would it make sense to pull some of the infrastructure bits out of
>> this patch and commit those bits separately, so as to reduce the size
>> of the main patch? In particular, the AggGetMemoryContext() stuff
>> looks like a good candidate for that treatment.

> Fair enough. Attached contains that part only.

I started looking at this patch. I believe that we should commit the
AggGetMemoryContext API function --- *not* the window context
management changes that you included here, but only the API abstraction
--- to be sure that that gets into 9.0 so that third-party aggregate
functions can start relying on it instead of looking directly at the
AggState or WindowAggState node. The rest of the patch might or might
not make it, but we can at least help people future-proof their code.

I'm fairly desperately unhappy with the RANGE PRECEDING/FOLLOWING parts
of the patch. We have expended a great deal of sweat over the years
to avoid hard-wiring assumptions about particular operator names into
the code, but this patch is blithely ignoring that history and assuming
that "+" and "-" will do the right thing. Also LookupOperName is
probably not the right thing, since it insists on exact or
binary-compatible match. To the extent that there is any justification
at all for assuming that "+"/"-" are the right operators, it is that the
spec speaks in terms of the range bounds being VSK+V2F etc --- but if
someone were to actually write out such an expression, the parser would
allow for implicit casts to happen, so this code is not implementing
what that expression would produce. Plus the results are dependent on
the current search path, which for example means it'll fail if the
window sort column is a user-defined type whose operators happen to be
out of path at the moment --- even though we would have found its
default sort opclass despite that. And lastly, I'm totally unconvinced
that it's a good idea to accept an operator that returns a type other
than the type of the window sort column. That seems to eliminate
whatever little protection we had against picking up an unsuitable
operator; and it complicates the code as well.

Given the lack of time remaining in this CF, I'm tempted to propose
ripping out the RANGE support and just trying to get ROWS committed.
That should be substantially less controversial from a semantic
standpoint, and it still seems like a considerable improvement in
functionality.

Thoughts?

regards, tom lane