Re: review: More frame options in window functions - Mailing list pgsql-hackers

2010/2/9 Tom Lane <tgl@sss.pgh.pa.us>:
> Hitoshi Harada <umi.tanuki@gmail.com> writes:
>> 2010/1/23 Robert Haas <robertmhaas@gmail.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.

I know "+"/"-" part is an issue. But as far as I know there's been no
infrastructure to handle such situation. My ideal plan is to introduce
some mechanism to make "+"/"-" operation abstract enough such like
sort opclass/opfamily, although I wasn't sure if that is to be
introduced for this (ie RANGE frame) purpose only.

Now that specialized hard-coding "+"/"-" in source seems unacceptable,
I would like to hear how to handle this. Is there any better than
introducing new mechanism such like opclass?

> 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?

As expected. I don't mind splitting patch to be committable if users
who expected this feature don't mind.

Regards,

--
Hitoshi Harada