Обсуждение: KNNGIST next step: adjusting indexAM API
In the current KNNGIST patch, the indexable ORDER BY clauses are
transmitted to the executor by cramming them in with the index qual
conditions (the IndexScan plan node's indexqual list), from whence
they become part of the ScanKey array passed to the index AM.
Robert complained that this was an ingenious way to minimize the
number of lines touched by the patch but utterly ugly from any other
standpoint, and I quite agree. An ORDER BY clause is a completely
different thing from a WHERE qual, so mixing them together doesn't
seem like a good idea.
However, if we hold to that principle then we need to modify the indexAM
API to pass the ordering operators separately. This is no big deal as
far as the built-in AMs are concerned, particularly because 3 of the 4
need only assert that the additional list is empty. The only reason it
would be a problem is if there were third-party index AMs that would be
affected to a larger degree; but I don't know of any. Does anyone have
an objection to that?
(Another thing that might be worth changing, as long as we have to touch
the beginscan and rescan APIs anyway, is to refactor the handling of
the initial set of scan keys. It never made any sense to me for
RelationGetIndexScan to call index_rescan: that seems to accomplish
little except making it difficult for AM beginscan routines to do things
in a sane order. I'm inclined to take that out and let the AM call
rescan internally if it wants to.)
Lastly, I'm pretty un-thrilled with the way that the KNNGIST patch
implements the interface to the opclass-specific hook functions.
Seems like it would be cleaner to leave the Consistent function alone
and invent a new, separate hook function for processing ORDER BY.
Is there a strong reason for having both things done in one call,
or was that just done as a byproduct of trying to cram all the data
into one ScanKey array?
regards, tom lane
On Tue, Nov 30, 2010 at 2:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > In the current KNNGIST patch, the indexable ORDER BY clauses are > transmitted to the executor by cramming them in with the index qual > conditions (the IndexScan plan node's indexqual list), from whence > they become part of the ScanKey array passed to the index AM. > Robert complained that this was an ingenious way to minimize the > number of lines touched by the patch but utterly ugly from any other > standpoint, and I quite agree. An ORDER BY clause is a completely > different thing from a WHERE qual, so mixing them together doesn't > seem like a good idea. > > However, if we hold to that principle then we need to modify the indexAM > API to pass the ordering operators separately. This is no big deal as > far as the built-in AMs are concerned, particularly because 3 of the 4 > need only assert that the additional list is empty. The only reason it > would be a problem is if there were third-party index AMs that would be > affected to a larger degree; but I don't know of any. Does anyone have > an objection to that? None here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Tom Lane <tgl@sss.pgh.pa.us> writes: > Lastly, I'm pretty un-thrilled with the way that the KNNGIST patch > implements the interface to the opclass-specific hook functions. > Seems like it would be cleaner to leave the Consistent function alone > and invent a new, separate hook function for processing ORDER BY. > Is there a strong reason for having both things done in one call, > or was that just done as a byproduct of trying to cram all the data > into one ScanKey array? IIRC, the goal here was to be able to benefit from KNN GiST from existing GiST indexes as soon as you restart the server with the new code compiled in. I'm not sure it's that important in the context of preparing 9.1. It seems that pg_upgrade already has to issue a reindex script for GiST indexes. Now, of course, that's memories from Royal-Oak sessions, so it might be all wrong too :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
On Wed, Dec 1, 2010 at 5:22 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote: > Tom Lane <tgl@sss.pgh.pa.us> writes: >> Lastly, I'm pretty un-thrilled with the way that the KNNGIST patch >> implements the interface to the opclass-specific hook functions. >> Seems like it would be cleaner to leave the Consistent function alone >> and invent a new, separate hook function for processing ORDER BY. >> Is there a strong reason for having both things done in one call, >> or was that just done as a byproduct of trying to cram all the data >> into one ScanKey array? > > IIRC, the goal here was to be able to benefit from KNN GiST from > existing GiST indexes as soon as you restart the server with the new > code compiled in. I'm not sure it's that important in the context of > preparing 9.1. It seems that pg_upgrade already has to issue a reindex > script for GiST indexes. I don't think Tom was proposing to change the on-disk format, just the API. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Dec 1, 2010 at 5:22 AM, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
>> IIRC, the goal here was to be able to benefit from KNN GiST from
>> existing GiST indexes as soon as you restart the server with the new
>> code compiled in. I'm not sure it's that important in the context of
>> preparing 9.1. It seems that pg_upgrade already has to issue a reindex
>> script for GiST indexes.
> I don't think Tom was proposing to change the on-disk format, just the API.
Right, AFAIK there is nothing in KNNGIST that would involve an on-disk
data change.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > Right, AFAIK there is nothing in KNNGIST that would involve an on-disk > data change. Nice, that matches my Royal Oak memories. But any external module relying on GiST will have to provide for the new function you're thinking about, right? Updating was already needed to cope with the newer consistent API, I guess. Will stop commenting now until I've had the time to read the code :) Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Dimitri Fontaine <dimitri@2ndQuadrant.fr> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> Right, AFAIK there is nothing in KNNGIST that would involve an on-disk
>> data change.
> But any external module relying on GiST will have to provide for the new
> function you're thinking about, right? Updating was already needed to
> cope with the newer consistent API, I guess.
ISTM that it should be possible to allow an opclass to not supply the
new hook function if it doesn't support any ordering operators. So
that's not really a serious problem. The existing patch approaches
this by having two different APIs for the Consistent function depending
on whether the opclass supports ordering operators or not. I find that
pretty icky, even though it nominally avoids breaking existing opclass
modules.
regards, tom lane