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. Regards, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
