Re: [HACKERS] KNNGIST next step: adjusting indexAM API
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] KNNGIST next step: adjusting indexAM API
On Wed, Dec 1, 2010 at 5:22 AM, Dimitri Fontaine dimi...@2ndquadrant.fr wrote: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] KNNGIST next step: adjusting indexAM API
Robert Haas robertmh...@gmail.com writes: On Wed, Dec 1, 2010 at 5:22 AM, Dimitri Fontaine dimi...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] KNNGIST next step: adjusting indexAM API
Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] KNNGIST next step: adjusting indexAM API
Dimitri Fontaine dimi...@2ndquadrant.fr writes: Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] KNNGIST next step: adjusting indexAM API
On Tue, Nov 30, 2010 at 2:50 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
