On Mon, Feb 8, 2010 at 10:37 PM, Hitoshi Harada <umi.tan...@gmail.com> wrote: > 2010/2/9 Tom Lane <t...@sss.pgh.pa.us>: >> Hitoshi Harada <umi.tan...@gmail.com> writes: >>> 2010/1/23 Robert Haas <robertmh...@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.
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. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
