On Mon, Feb 8, 2010 at 10:37 PM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> 2010/2/9 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> 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.
> 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
> As expected. I don't mind splitting patch to be committable if users
> who expected this feature don't mind.
Well, they'll likely be happier with a partial feature than no feature
at all... I agree with Tom that there's no time time now to resolve
the issue of how + and - should be handled.
In response to
pgsql-hackers by date
|Next:||From: Andrew Dunstan||Date: 2010-02-09 04:56:52|
|Subject: Re: CVS checkout source code for different branches|
|Previous:||From: Robert Haas||Date: 2010-02-09 04:43:08|
|Subject: Re: [CFReview] Red-Black Tree|