Thread

1. KNNGiST for knn-search

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-23T17:44:01Z

   Hi there,
   
   http://www.sigaev.ru/misc/knngist-0.11.tar.gz
   
   we'd like to present contrib module for CVS HEAD, which contains implementation 
   of knn (k nearest neighbourhood) search in PostgreSQL, see README.knngist for
   details. KNNGiST is an extension of existing GiST, which inherits most of
   their code, so it's has recovery (WAL-logged)  and concurrency support.
   Basically, it introduces a new distance-based priority queue tree traversal
   algorithm (instead of depth-first in plain GiST), so index scan returns rows
   sorted by closiness to a query. Notice, index returns all rows, so one should
   use LIMIT clause to specify k (which is usually small) to get real benefits.
   We get 300x times better performance on real-life data (about 1 mln points):
   
   Module requires rbtree and point_ops patches applied. 
   (http://archives.postgresql.org/message-id/4B0A8DFA.7050009@sigaev.ru and 
   http://archives.postgresql.org/message-id/4B0A8F0F.3020308@sigaev.ru)
   
   Old way:
   SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM spots
   order by dist asc LIMIT 10;
   
   Time: 1024.242 ms
   
   knn-search:
   SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM spots
   WHERE coordinates  >< '5.0,5.0'::point LIMIT 10;
   
   Time: 3.158 ms
   
   
   We didn't patch existing implementation of GiST for several reasons:
   
   1. KNNGiST is about 5% slower than GiST on non-knn search queries, like
      contains or contained by, because of some overhead of new algorithm of
      tree traversal
   2.  KNNGiST can't be used in  bitmap index scan, which destroys order of results,
      We don't know the way to forbid bitmap index scan only for knn queries.
      Current version of KNNGiST doesn't distinguish knn-search and usual search
      and postgres doesn't know about ordered output from KNNGiST.
   
   We see several ways to add KNNGiST to PostgreSQL:
   
   1. Patch existing GiST.
       Con - see problems above.
       Pro - any existing GiST opclasses will work with KNNGiST.
   
   2. add KNNGIST as a contrib module like now.
       Con - there is no way in PostgreSQL to test other modules, which depends
             on KNNGiST. For example, it's easy to add support for trigrams, but
             then we add dependence on contrib/pg_trgm module.
   
   3. add KNNGiST as separate access method into core of PostgreSQL.
       We think it the best way, since we don't touch existing GiST and opclasses,
       and could use KNNGiST in other contrib modules
   
   
   Separate problem is query planning.
   
   1. To use KNNGiST we need to use index scan ! To prevent seqscan on table,
       operator >< has extremely high cost.
   2. To prevent bitmap index scan, KNNGiST doesn't have bitmap index scan
       interface at all (no amgetbitmap method).
   
   
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   

2. Re: KNNGiST for knn-search

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-24T07:33:04Z

   Teodor Sigaev wrote:
   > we'd like to present contrib module for CVS HEAD, which contains
   > implementation of knn (k nearest neighbourhood) search in PostgreSQL,
   > see README.knngist for
   > details.
   
   Cool!
   
   > Old way:
   > SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM spots
   > order by dist asc LIMIT 10;
   > 
   > Time: 1024.242 ms
   > 
   > knn-search:
   > SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM spots
   > WHERE coordinates  >< '5.0,5.0'::point LIMIT 10;
   > 
   > Time: 3.158 ms
   
   I think you'll need to work on that. A WHERE qual shouldn't imply a sort
   order. You'll have to teach the planner how to use the index to speed up
   a query in the first form.
   
   > We didn't patch existing implementation of GiST for several reasons:
   > 
   > 1. KNNGiST is about 5% slower than GiST on non-knn search queries, like
   >   contains or contained by, because of some overhead of new algorithm of
   >   tree traversal
   
   Is it possible to use the regular GiST traversal algorithm on a
   KNNGiST-tree, when performing regular GiST searches that don't require a
   particular order?
   
   > 2.  KNNGiST can't be used in  bitmap index scan, which destroys order of
   > results,
   >   We don't know the way to forbid bitmap index scan only for knn queries.
   >   Current version of KNNGiST doesn't distinguish knn-search and usual
   > search
   >   and postgres doesn't know about ordered output from KNNGiST.
   
   Yeah, you really need to modify the planner to understand the ordering
   and plan accordingly.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

3. Re: KNNGiST for knn-search

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-24T11:49:40Z

   > I think you'll need to work on that. A WHERE qual shouldn't imply a sort
   > order. You'll have to teach the planner how to use the index to speed up
   > a query in the first form.
   Of course, right now it is a working prototype.
   
   >> 1. KNNGiST is about 5% slower than GiST on non-knn search queries, like
   >>   contains or contained by, because of some overhead of new algorithm of
   >>   tree traversal
   > 
   > Is it possible to use the regular GiST traversal algorithm on a
   > KNNGiST-tree, when performing regular GiST searches that don't require a
   > particular order?
   New algorithm works much more with memory for allocation/free to manage lists 
   and it's a single reason of performance loss. Choosing of algorithm could not be 
   done by consistent function, it should be done at least in amrescan method or 
   even earlier - in planner.
   
   
   > 
   >> 2.  KNNGiST can't be used in  bitmap index scan, which destroys order of
   >> results,
   >>   We don't know the way to forbid bitmap index scan only for knn queries.
   >>   Current version of KNNGiST doesn't distinguish knn-search and usual
   >> search
   >>   and postgres doesn't know about ordered output from KNNGiST.
   > 
   > Yeah, you really need to modify the planner to understand the ordering
   > and plan accordingly.
   Hmm, I thought about it, but still have no a good idea.
   One idea:
   SELECT p FROM pt WHERE p << '5.0,5.0'::point ORDER BY (p <-> '5.0,5.0'::point) 
   DESC LIMIT 10;
   And add <-> to opclass (but for now any indexable operation should return 
   boolean type). Of course, KNNGiST should be modified to support not only 
   k-nearest search but k-"farest" search and NULLS LAST/FIRST.
   
   Not very convenient, because it's needed to look into expression of ORDER BY. 
   And now you can specify p >< 'one point' AND p >< 'another point', but it's 
   impossible to do that by ORDER BY clause.
   
   Second idea with non-standard syntax.
   SELECT ... ORDER BY PROXIMITY OF expression[, expression [..]] TO expression[, 
   expression [..]] USING [operator [, operator [..]]
   and operator is distance operator, i.e. it's not a member of btree opclass, but 
   returns non-negative float8 value.
   
   Without index it will be essentially the same as
   ORDER BY expression operator expression[ + ..] DESC NULLS LAST
   
   
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   
   

4. Re: KNNGiST for knn-search

   Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-11-24T12:37:29Z

   Teodor Sigaev wrote:
   >>> 1. KNNGiST is about 5% slower than GiST on non-knn search queries, like
   >>>   contains or contained by, because of some overhead of new algorithm of
   >>>   tree traversal
   >>
   >> Is it possible to use the regular GiST traversal algorithm on a
   >> KNNGiST-tree, when performing regular GiST searches that don't require a
   >> particular order?
   > New algorithm works much more with memory for allocation/free to manage
   > lists and it's a single reason of performance loss. Choosing of
   > algorithm could not be done by consistent function, it should be done at
   > least in amrescan method or even earlier - in planner.
   
   Ok, that sounds good. The bottom line is that you can use the same
   on-disk tree with both algorithms. No need for a separate indexam in
   that case.
   
   > One idea:
   > SELECT p FROM pt WHERE p << '5.0,5.0'::point ORDER BY (p <->
   > '5.0,5.0'::point) DESC LIMIT 10;
   > And add <-> to opclass (but for now any indexable operation should
   > return boolean type).
   
   You really shouldn't need to have a WHERE clause.
   
   > Of course, KNNGiST should be modified to support
   > not only k-nearest search but k-"farest" search and NULLS LAST/FIRST.
   
   Well, as long as the planner knows the capabilities of the indexam, it
   can just fall back to a seqscan+sort if the query can't be sped up with
   the index.
   
   > And now you can specify p >< 'one point' AND p >< 'another
   > point', but it's impossible to do that by ORDER BY clause.
   
   Huh, what does that mean? Is it like "ORDER BY (min( p >< 'one point', p
   >< 'another point')" ?
   
   > Second idea with non-standard syntax.
   > SELECT ... ORDER BY PROXIMITY OF expression[, expression [..]] TO
   > expression[, expression [..]] USING [operator [, operator [..]]
   > and operator is distance operator, i.e. it's not a member of btree
   > opclass, but returns non-negative float8 value.
   > 
   > Without index it will be essentially the same as
   > ORDER BY expression operator expression[ + ..] DESC NULLS LAST
   
   We already have the syntax to represent the query, using ORDER BY. IMHO
   we just need to teach the planner that when it sees a query like that,
   it can use a GiST index to speed it up. A number of indexam and operator
   class API changes are probably required, but it should be invisible to
   the user.
   
   -- 
     Heikki Linnakangas
     EnterpriseDB   http://www.enterprisedb.com
   
   

5. Re: KNNGiST for knn-search

   Simon Riggs <simon@2ndquadrant.com> — 2009-11-25T02:13:10Z

   On Mon, 2009-11-23 at 20:44 +0300, Teodor Sigaev wrote:
   > Old way:
   > SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM
   > spots
   > order by dist asc LIMIT 10;
   > 
   > Time: 1024.242 ms
   > 
   > knn-search:
   > SELECT coordinates, (coordinates <-> '5.0,5.0'::point) AS dist FROM
   > spots
   > WHERE coordinates  >< '5.0,5.0'::point LIMIT 10;
   > 
   > Time: 3.158 ms
   > 
   > 
   > We didn't patch existing implementation of GiST for several reasons:
   > 
   > 1. KNNGiST is about 5% slower than GiST on non-knn search queries,
   > like
   >    contains or contained by, because of some overhead of new algorithm
   > of
   >    tree traversal
   > 2.  KNNGiST can't be used in  bitmap index scan, which destroys order
   > of results,
   >    We don't know the way to forbid bitmap index scan only for knn
   > queries.
   >    Current version of KNNGiST doesn't distinguish knn-search and usual
   > search
   >    and postgres doesn't know about ordered output from KNNGiST.
   
   Sounds very cool.
   
   Seems like you should look at the way sorted_path works in
   query_planner().
   
   If you have a query like this
   
     explain select col1 from s order by col1 limit 10;
   
   then we currently understand that we should use an IndexScan for that.
   We don't specifically exclude the bitmap scan, it's just that we know
   that the results from the index are ordered and therefore the cost of
   sorting the output need not be added. In the bitmap case the cost of the
   sort must be added and that's enough to ensure we almost never do that.
   
   I notice that a query like 
   
     explain select col1 from s order by abs(col1 - 5) limit 10;
   
   is the one-dimensional equivalent of the type of query you're proposing
   and that doesn't work either until you put an index on abs(col1 - 5),
   then it just works, but only for k = 5.
   
   Maybe you should look at the above query and see if there are any usable
   similarities for the Knn index.
   
   Part of your problem appears to be that cost_sort does not include
   anything about the cost of the comparison operators for different
   datatypes.
   
   -- 
    Simon Riggs           www.2ndQuadrant.com
   
   
   

6. Re: KNNGiST for knn-search (WIP)

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-26T16:34:21Z

   Hi!
   
   Contrib module is reworked as a patch for current GiST. Now GiST supports 
   KNN-search, the query looks like
   SELECT * FROM point_tbl ORDER BY f1 <-> '0,1';
   or
   SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> '0,1';
   Plans are:
   EXPLAIN (COSTS OFF)
   SELECT * FROM point_tbl ORDER BY f1 <-> '0,1';
                   QUERY PLAN
   -----------------------------------------
     Index Scan using gpointind on point_tbl
       Index Cond: (f1 <-> '(0,1)'::point)
   EXPLAIN (COSTS OFF)
   SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> '0,1';
                                      QUERY PLAN
   ------------------------------------------------------------------------------
     Index Scan using gpointind on point_tbl
       Index Cond: ((f1 <@ '(10,10),(-10,-10)'::box) AND (f1 <-> '(0,1)'::point))
   
   
   pg_am now has new column amcanorderbyop (can order by operation), indexes with 
   this flag enabled can be used to speedup operations, which returns non-boolean 
   value, currently type of returned value should have default btree operator class 
     to perform sort.
   
   Planner (find_usable_indexes function, actually) could push pathkey expression 
   into restriction clauses of index. I'm not fully satisfied with this piece of 
   code, but, may be, someone has a better idea. I though about adding separate 
   indexorderquals in IndexPath structure...
   
   Both GiST's get methods are optimized and there is no overhead, since gistrescan 
   method can choose what traversal algorithm to use using information about types 
   of values returned by operations. If at least one of them returns non-boolean 
   result, then KNN-search will be performed.
   
   The only change in interface of supporting functions is: consistentFn function 
   could return float8 non-negative value and it's mandatory to perform KNN-search. 
   Old-style consistent functions are supported.
   
   Patch contains (it still requires rbtree-0.5 and point_ops-0.4 patches):
   - GiST changes
   - changes in point_ops to support knn-search
   - contrib/pg_trgm now has new operation <-> returns distance between texts. This 
   operation is supported in KNN-search
   - contrib/btree_gist provides <-> and its support for GiST for types int2, int4, 
   int8, float4, float8, money, oid, interval, time, date, timestamp and timestamptz
   
   TODO:
   - selectivity of ordering operation should be 1.0
   - current patch remove support of  IndexScanDesc->kill_prior_tuple, it's needed 
   to restore support if it will not create too big overhead
   - documentation
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   

7. Re: KNNGiST for knn-search

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-26T16:37:13Z

   >   explain select col1 from s order by abs(col1 - 5) limit 10;
   > 
   > is the one-dimensional equivalent of the type of query you're proposing
   Exactly, it's already done in next version of patch :)
   
   > and that doesn't work either until you put an index on abs(col1 - 5),
   > then it just works, but only for k = 5.
   
   BTW, it's possible to add this feature to plain btree by changing traversal 
   algorithm, but I'm fill enough power to do it.
   
   
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   
   

8. Re: KNNGiST for knn-search

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-26T16:42:26Z

   > BTW, it's possible to add this feature to plain btree by changing 
   > traversal algorithm, but I'm fill enough power to do it.
   
   Sorry, I'm NOT fill enough power to do it.
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   
   

9. Re: KNNGiST for knn-search

   Teodor Sigaev <teodor@sigaev.ru> — 2009-11-26T16:45:58Z

   >> BTW, it's possible to add this feature to plain btree by changing 
   >> traversal algorithm, but I'm fill enough power to do it.
   > 
   > Sorry, I'm NOT fill enough power to do it.
   
   %-), I'm NOT FEEL enough power to do it.
   -- 
   Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                       WWW: http://www.sigaev.ru/
   
   

10. Re: KNNGiST for knn-search (WIP)

    Teodor Sigaev <teodor@sigaev.ru> — 2009-11-27T15:23:50Z

    > Planner (find_usable_indexes function, actually) could push pathkey 
    > expression into restriction clauses of index. I'm not fully satisfied 
    > with this piece of code, but, may be, someone has a better idea. I 
    > though about adding separate indexorderquals in IndexPath structure...
    
    Done, IndexScan and IndexPath have separate field to store order clauses. That's 
    allowed to improve explain output:
    #  EXPLAIN (COSTS OFF)
       SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1 <-> 
    '0,1';
                        QUERY PLAN
    ------------------------------------------------
      Index Scan using gpointind on point_tbl
        Index Cond: (f1 <@ '(10,10),(-10,-10)'::box)
        Sort Cond: (f1 <-> '(0,1)'::point)
    (3 rows)
    
    We are waiting feedback to choose a way of planner support of knn-search.
    
    Still TODO:
    - cost of index scan
    - Sort condition should not be ANDed in explain output
    - current patch remove support of  IndexScanDesc->kill_prior_tuple, it's needed 
    to restore support if it will not create too big overhead
    - documentation
    
    -- 
    Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                        WWW: http://www.sigaev.ru/
    

11. Re: KNNGiST for knn-search (WIP)

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-27T15:38:14Z

    Teodor Sigaev <teodor@sigaev.ru> writes:
    >> Planner (find_usable_indexes function, actually) could push pathkey 
    >> expression into restriction clauses of index. I'm not fully satisfied 
    >> with this piece of code, but, may be, someone has a better idea. I 
    >> though about adding separate indexorderquals in IndexPath structure...
    
    > Done, IndexScan and IndexPath have separate field to store order clauses.
    
    Why?  Isn't that redundant with the pathkey structures?  We generate
    enough paths during a complex planning problem that I'm not in favor
    of adding unnecessary structures to them.
    
    			regards, tom lane
    
    

12. Re: KNNGiST for knn-search (WIP)

    Teodor Sigaev <teodor@sigaev.ru> — 2009-11-27T16:26:22Z

    >> Done, IndexScan and IndexPath have separate field to store order clauses.
    > 
    > Why?  Isn't that redundant with the pathkey structures?  We generate
    > enough paths during a complex planning problem that I'm not in favor
    > of adding unnecessary structures to them.
    I found two ways to add support of knn-seaech to planner
    - 0.4 version: add sorting clauses to restrictclauses in find_usable_indexes,
       and there is two issues:
       - find_usable_indexes could not be used to find indexes for index and bitmap
         scans at once, because sorting clauses will be used in bitmap scan. Full
         scan of index with knn-ordering on large index is much more expensive.
       - implied/refused predicate machinery is teached to ignore sorting clauses,
         but it's not so obvious: it should ignore operation returning non-boolean
         values
    - 0.4.1 version: pull sort clauses separately and merge them with regular
       clauses at create_indexscan_plan function. That's solves problems above
    
    Do you suggest to construct that clauses from pathkey representation? And append 
      constructed clauses to indexquals in create_indexscan_plan?
    -- 
    Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                        WWW: http://www.sigaev.ru/
    
    

13. Re: KNNGiST for knn-search (WIP)

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-11-27T16:47:24Z

    Teodor Sigaev <teodor@sigaev.ru> writes:
    > Do you suggest to construct that clauses from pathkey representation? And append 
    >   constructed clauses to indexquals in create_indexscan_plan?
    
    I'd probably have to read the patch to make any intelligent suggestions
    ... and right now I'm busy with patches that are already in the
    commitfest.  But usually it's a good idea to postpone work to createplan
    time if you can.
    
    			regards, tom lane
    
    

14. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2009-12-30T06:55:26Z

    2009/11/26 Teodor Sigaev <teodor@sigaev.ru>:
    > Hi!
    >
    > Contrib module is reworked as a patch for current GiST. Now GiST supports
    > KNN-search, the query looks like
    > SELECT * FROM point_tbl ORDER BY f1 <-> '0,1';
    > or
    > SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1
    > <-> '0,1';
    > Plans are:
    > EXPLAIN (COSTS OFF)
    > SELECT * FROM point_tbl ORDER BY f1 <-> '0,1';
    >               QUERY PLAN
    > -----------------------------------------
    >  Index Scan using gpointind on point_tbl
    >   Index Cond: (f1 <-> '(0,1)'::point)
    > EXPLAIN (COSTS OFF)
    > SELECT * FROM point_tbl WHERE f1 <@ '(-10,-10),(10,10)':: box ORDER BY f1
    > <-> '0,1';
    >                                  QUERY PLAN
    > ------------------------------------------------------------------------------
    >  Index Scan using gpointind on point_tbl
    >   Index Cond: ((f1 <@ '(10,10),(-10,-10)'::box) AND (f1 <-> '(0,1)'::point))
    >
    >
    > pg_am now has new column amcanorderbyop (can order by operation), indexes
    > with this flag enabled can be used to speedup operations, which returns
    > non-boolean value, currently type of returned value should have default
    > btree operator class  to perform sort.
    >
    > Planner (find_usable_indexes function, actually) could push pathkey
    > expression into restriction clauses of index. I'm not fully satisfied with
    > this piece of code, but, may be, someone has a better idea. I though about
    > adding separate indexorderquals in IndexPath structure...
    >
    > Both GiST's get methods are optimized and there is no overhead, since
    > gistrescan method can choose what traversal algorithm to use using
    > information about types of values returned by operations. If at least one of
    > them returns non-boolean result, then KNN-search will be performed.
    >
    > The only change in interface of supporting functions is: consistentFn
    > function could return float8 non-negative value and it's mandatory to
    > perform KNN-search. Old-style consistent functions are supported.
    >
    > Patch contains (it still requires rbtree-0.5 and point_ops-0.4 patches):
    > - GiST changes
    > - changes in point_ops to support knn-search
    > - contrib/pg_trgm now has new operation <-> returns distance between texts.
    > This operation is supported in KNN-search
    > - contrib/btree_gist provides <-> and its support for GiST for types int2,
    > int4, int8, float4, float8, money, oid, interval, time, date, timestamp and
    > timestamptz
    >
    > TODO:
    > - selectivity of ordering operation should be 1.0
    > - current patch remove support of  IndexScanDesc->kill_prior_tuple, it's
    > needed to restore support if it will not create too big overhead
    > - documentation
    
    Based on the feedback provided on this patch so far, it looks like
    some changes are probably needed, but it's not entirely clear whether
    the feedback provided is sufficient to provide guidance on what
    changes should be made.  It does also need to be updated to CVS HEAD,
    as it no longer applies cleanly.
    
    I tend to feel that we should probably target this for 8.6 rather than
    8.5.  We are down to the last CommitFest, and while we don't have a
    nailed-down criterion for what is "too big" for the last CommitFest of
    a given release cycle, this is definitely a big, invasive patch.  This
    patch weights in at over 2400 adds/removes, and it's not boilerplate
    stuff like updates to pg_proc entries, but real, significant changes.
    I'm worried that applying something like this late in the release
    cycle is just not a good idea, especially given the fact that it
    probably still needs significant revising.  However, I'm fairly
    conservative by nature, so perhaps someone else will have a different
    opinion, or maybe there is a way to restructure it so that the needed
    changes are less invasive.
    
    ...Robert
    
    

15. Re: KNNGiST for knn-search (WIP)

    Oleg Bartunov <oleg@sai.msu.su> — 2009-12-30T14:20:32Z

    Robert,
    
    On Wed, 30 Dec 2009, Robert Haas wrote:
    
    > Based on the feedback provided on this patch so far, it looks like
    > some changes are probably needed, but it's not entirely clear whether
    > the feedback provided is sufficient to provide guidance on what
    > changes should be made.  It does also need to be updated to CVS HEAD,
    > as it no longer applies cleanly.
    
    this is not a problem.
    
    >
    > I tend to feel that we should probably target this for 8.6 rather than
    > 8.5.  We are down to the last CommitFest, and while we don't have a
    > nailed-down criterion for what is "too big" for the last CommitFest of
    > a given release cycle, this is definitely a big, invasive patch.  This
    > patch weights in at over 2400 adds/removes, and it's not boilerplate
    > stuff like updates to pg_proc entries, but real, significant changes.
    > I'm worried that applying something like this late in the release
    > cycle is just not a good idea, especially given the fact that it
    > probably still needs significant revising.  However, I'm fairly
    > conservative by nature, so perhaps someone else will have a different
    > opinion, or maybe there is a way to restructure it so that the needed
    > changes are less invasive.
    
    the patch adds new strategy of gist tree traverse and doesn't change old one,
    so there is no risk to ruin old code. I'm all for  good conservatism, but
    this is not the case, else we wouldn't have GiST at all. We are very
    interested in the KNN to be in the 8.5 and we're ready to fix any issues.
    
    From metodological point of view I don't quite understand how to measure
    the value of development, I mean what'is a "big patch", "invasive patch".
    Should we prefer cosmetic pathces, spelling fixes, etc ? Of course, they are
    easy for refering, but people are waiting from us not just fixes, but new
    features. For example, KNN-GiST is a big improvement for PostGIS community,
    which is a big part of postgres users. Actually, it's PostGIS community, which
    supported our work. Now, what we should say them ? The patch was too big and
    invasive, so, sorry, wait one year more ? I think it's not good.
    
    Robert, I'm not against you, it's your right to have your opinion. I address
    this to other developers. It's important for us, since we have several
    other patches ready, for example, long awaited phrase search 
    (http://www.sai.msu.su/~megera/wiki/2009-08-12). We postponed it, since 
    it was supposed that EDB will support it, but, hey, it wont. We did it for our
    own. Teodor insist to submit it for 8.5, but I'm now begin to hesitate, what if
    this patch will be also too big.
    
     	Regards,
     		Oleg
    _____________________________________________________________
    Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
    Sternberg Astronomical Institute, Moscow University, Russia
    Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
    phone: +007(495)939-16-83, +007(495)939-23-83
    
    

16. Re: KNNGiST for knn-search (WIP)

    Teodor Sigaev <teodor@sigaev.ru> — 2009-12-30T14:59:53Z

    > changes should be made.  It does also need to be updated to CVS HEAD,
    > as it no longer applies cleanly.
    
    The reason was a point_ops patch, some OIDs become duplicated. Both attached 
    patches are synced with current CVS.
    >
    > I tend to feel that we should probably target this for 8.6 rather than
    > 8.5.  We are down to the last CommitFest, and while we don't have a
    > nailed-down criterion for what is "too big" for the last CommitFest of
    > a given release cycle, this is definitely a big, invasive patch.  This
    
    Is we really have rule to accept only small patches at last CommitFest? May be, 
    FixFest name is better for it? :)
    
    Actually, it's easy to split patch to several ones:
    - contrib/pg_trgm
    - contrib/btree_gist
    - knngist itself
    - planner changes
    
    And knngist depends on rbtree and point_ops patch, in summary 6 dependent 
    patches. Is it more comfortable?
    
    -- 
    Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                        WWW: http://www.sigaev.ru/
    

17. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2009-12-30T16:56:17Z

    On Wed, Dec 30, 2009 at 9:20 AM, Oleg Bartunov <oleg@sai.msu.su> wrote:
    > From metodological point of view I don't quite understand how to measure
    > the value of development, I mean what'is a "big patch", "invasive patch".
    
    I want to speak specifically to this question because I think it's a
    good one.  Of course, I also want to make clear that I have nothing
    against you or your patch and that it sounds like a really nice
    feature.
    
    From my point of view, what makes a patch invasive is the likelihood
    that it might break something other than itself.  For example, your
    patch touches the core planner code and the core GIST code, so it
    seems possible that adding support for this feature might break
    something else in one of those areas.  All things being equal, we
    would prefer to take that risk at the beginning of a development cycle
    rather than the end.  If your patch was the same size, but consisted
    mostly of new code with very few changes to what is there now, it
    might still be difficult to properly review and verify - but any bugs
    we missed would likely affect only the NEW functionality, not any
    EXISTING functionality.
    
    Please understand that the previous paragraph is intended to be a
    general statement about software development in general more than a
    specific commentary on your particular patch.  Whether applying your
    patch in particular will break anything is, of course, something
    that's difficult to know until we do it and see what happens, and at
    this point I haven't even reviewed it.  It's also possible that I'm
    doing a poor job estimating the risk of breakage, and I certainly
    welcome other opinions from other people in a position to make a
    technical judgement on that point.  I might also have a different
    opinion myself after I review the patch in more detail, so please do
    post an updated version.
    
    > Should we prefer cosmetic pathces, spelling fixes, etc ? Of course, they are
    > easy for refering, but people are waiting from us not just fixes, but new
    > features. For example, KNN-GiST is a big improvement for PostGIS community,
    > which is a big part of postgres users. Actually, it's PostGIS community,
    > which
    > supported our work. Now, what we should say them ? The patch was too big and
    > invasive, so, sorry, wait one year more ? I think it's not good.
    
    Well, I understand your point, but there is obviously some deadline
    for patches to be submitted for any particular release.  Clearly,
    after the last CommitFest is over, that deadline is past.  However, we
    have previously discussed having a policy that no new large patches
    will be accepted for the last CommitFest that were not also submitted
    for the second-to-last CommitFest.  Hopefully it's obvious that I have
    no desire to keep cool new features away from the PostGIS community,
    the PostgreSQL community, or anyone else, but we have to weigh that
    against the desire to have a stable and bug-free release, and applying
    big patches at the last minute makes that less likely.
    
    As an example, the change to run the background writer during recovery
    and the changes in semi/anti join planning for 8.4 have both resulted
    in multiple bug reports.  The former was half the footprint of your
    patch and applied at the very end of the release cycle; the latter was
    slightly larger and applied in August 2008, so considerably earlier in
    the cycle than this one could possibly be - and there were still
    things we did not catch before release.
    
    ...Robert
    
    

18. Re: KNNGiST for knn-search (WIP)

    Alvaro Herrera <alvherre@commandprompt.com> — 2009-12-30T17:05:00Z

    Teodor Sigaev escribió:
    
    > Actually, it's easy to split patch to several ones:
    > - contrib/pg_trgm
    > - contrib/btree_gist
    > - knngist itself
    > - planner changes
    
    +1 on the split patches.  I wonder about the opr_sanity test change ...
    why is it necessary?
    
    -- 
    Alvaro Herrera                                http://www.CommandPrompt.com/
    PostgreSQL Replication, Consulting, Custom Development, 24x7 support
    
    

19. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2009-12-30T17:16:41Z

    2009/12/30 Teodor Sigaev <teodor@sigaev.ru>:
    >> changes should be made.  It does also need to be updated to CVS HEAD,
    >> as it no longer applies cleanly.
    >
    > The reason was a point_ops patch, some OIDs become duplicated. Both attached
    > patches are synced with current CVS.
    
    Thanks!  I will take a look.
    
    >> I tend to feel that we should probably target this for 8.6 rather than
    >> 8.5.  We are down to the last CommitFest, and while we don't have a
    >> nailed-down criterion for what is "too big" for the last CommitFest of
    >> a given release cycle, this is definitely a big, invasive patch.  This
    >
    > Is we really have rule to accept only small patches at last CommitFest? May
    > be, FixFest name is better for it? :)
    
    See here and following for some of the previous discussion - which was
    not unanimous on all points:
    
    http://archives.postgresql.org/pgsql-hackers/2009-09/msg00139.php
    
    I think the intention is not to accept only bug fixes, but to limit
    large features to those that have already been through a CommitFest or
    two.
    
    > Actually, it's easy to split patch to several ones:
    > - contrib/pg_trgm
    > - contrib/btree_gist
    > - knngist itself
    > - planner changes
    >
    > And knngist depends on rbtree and point_ops patch, in summary 6 dependent
    > patches. Is it more comfortable?
    
    I'm not sure. One of the problems with separating out contrib module
    changes is that it tends to obscure the point of the changes to the
    core code.  On the other hand if some of the core code changes can be
    split out into an infrastructure patch that is of some independent
    usefulness, that can certainly be worthwhile.  It's not obvious to me
    without looking at this more than I have whether there is a possble
    split that makes sense here; I will read your updated patch.
    
    ...Robert
    
    

20. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2009-12-30T19:37:35Z

    On Wed, Dec 30, 2009 at 12:16 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > 2009/12/30 Teodor Sigaev <teodor@sigaev.ru>:
    >>> changes should be made.  It does also need to be updated to CVS HEAD,
    >>> as it no longer applies cleanly.
    >>
    >> The reason was a point_ops patch, some OIDs become duplicated. Both attached
    >> patches are synced with current CVS.
    >
    > Thanks!  I will take a look.
    
    OK, I'm confused.  First, there are two versions of the patch here, so
    I'm not sure which one I'm supposed to be looking at.  Second, when I
    attempt to apply either one, I get:
    
    $ patch -p0 < ~/Download/builtin_knngist-0.5
    patching file src/backend/access/gist/gistget.c
    patching file src/backend/access/gist/gistproc.c
    Reversed (or previously applied) patch detected!  Assume -R? [n]
    
    ...regardless of how I answer that question, it then goes on to apply
    most of the rest of the patch successfully.
    
    Help?
    
    ...Robert
    
    

21. Re: KNNGiST for knn-search (WIP)

    Greg Stark <gsstark@mit.edu> — 2009-12-31T12:26:46Z

    On Wed, Dec 30, 2009 at 4:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    >
    > From my point of view, what makes a patch invasive is the likelihood
    > that it might break something other than itself.  For example, your
    > patch touches the core planner code and the core GIST code, so it
    > seems possible that adding support for this feature might break
    > something else in one of those areas.
    
    It doesn't seem obvious to me that this is a high-risk patch. It's
    touching the planner which is tricky but it's not the kind of massive
    overhaul that touches every module that HOT or HS were.  I'm really
    glad HS got in before the end because lots of people with different
    areas of expertise and different use cases are going to get to
    exercise it in the time remaining. This patch I would expect
    relatively few people to need to try it out before any oversights are
    caught.
    
    -- 
    greg
    
    

22. Re: KNNGiST for knn-search (WIP)

    Paul Ramsey <pramsey@cleverelephant.ca> — 2010-01-04T22:33:25Z

    I'm sure whatever conclusion -hackers comes to in the end will be the
    best for pgsql, and I'll be supportive. But until then, let me note
    from the PostGIS point-of-view: sure would be great to get this in for
    8.5 :)
    
    P.
    
    On Thu, Dec 31, 2009 at 4:26 AM, Greg Stark <gsstark@mit.edu> wrote:
    > On Wed, Dec 30, 2009 at 4:56 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    >>
    >> From my point of view, what makes a patch invasive is the likelihood
    >> that it might break something other than itself.  For example, your
    >> patch touches the core planner code and the core GIST code, so it
    >> seems possible that adding support for this feature might break
    >> something else in one of those areas.
    >
    > It doesn't seem obvious to me that this is a high-risk patch. It's
    > touching the planner which is tricky but it's not the kind of massive
    > overhaul that touches every module that HOT or HS were.  I'm really
    > glad HS got in before the end because lots of people with different
    > areas of expertise and different use cases are going to get to
    > exercise it in the time remaining. This patch I would expect
    > relatively few people to need to try it out before any oversights are
    > caught.
    >
    > --
    > greg
    >
    > --
    > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
    > To make changes to your subscription:
    > http://www.postgresql.org/mailpref/pgsql-hackers
    >
    
    

23. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2010-01-04T23:02:18Z

    On Mon, Jan 4, 2010 at 5:33 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
    > I'm sure whatever conclusion -hackers comes to in the end will be the
    > best for pgsql, and I'll be supportive. But until then, let me note
    > from the PostGIS point-of-view: sure would be great to get this in for
    > 8.5 :)
    
    That's good to know.   The current status is that I've been waiting
    for a patch that applies cleanly for 6 days, and we have 41 days left
    until the end of the last CommitFest.  There's not much I can do to
    move this along until I have a clean patch to work with.
    
    ...Robert
    
    

24. Re: KNNGiST for knn-search (WIP)

    Oleg Bartunov <oleg@sai.msu.su> — 2010-01-05T08:11:42Z

    Robert,
    
    On Mon, 4 Jan 2010, Robert Haas wrote:
    
    > On Mon, Jan 4, 2010 at 5:33 PM, Paul Ramsey <pramsey@cleverelephant.ca> wrote:
    >> I'm sure whatever conclusion -hackers comes to in the end will be the
    >> best for pgsql, and I'll be supportive. But until then, let me note
    >> from the PostGIS point-of-view: sure would be great to get this in for
    >> 8.5 :)
    >
    > That's good to know.   The current status is that I've been waiting
    > for a patch that applies cleanly for 6 days, and we have 41 days left
    > until the end of the last CommitFest.  There's not much I can do to
    > move this along until I have a clean patch to work with.
    
    sorry, it's a long holiday in Russia, we'll be able to sync next week.
    
    
     	Regards,
     		Oleg
    _____________________________________________________________
    Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru),
    Sternberg Astronomical Institute, Moscow University, Russia
    Internet: oleg@sai.msu.su, http://www.sai.msu.su/~megera/
    phone: +007(495)939-16-83, +007(495)939-23-83
    
    

25. Re: KNNGiST for knn-search (WIP)

    Teodor Sigaev <teodor@sigaev.ru> — 2010-01-12T19:21:03Z

    Changes:
    
    - split patch to several ones
    - sync with current CVS
    
    Patch set is based on 0.5.1 version, difference between 0.5 and 0.6 should be 
    only in planner patch.
    
    builtin_knngist_itself-0.6.gz  - patch to the gist itself
    builtin_knngist_proc-0.6.gz - patch for support knnsearch in point_ops
    builtin_knngist_planner-0.6.gz - planner patch to support knnearch
    builtin_knngist_contrib_btree_gist-0.6.gz - patch for contrib/btree_gist module
                    patch provides <-> operation for various scalar types which is
                    exactly  abs(a - b) function
    builtin_knngist_contrib_pg_trgm-0.6.gz - contrib/pg_trgm, like above,patch
                    provides <-> distance between strings
    
    
    Patch set sill requires rbtree patch and point_ops patch (with Robert's changes)
    
    -- 
    Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                        WWW: http://www.sigaev.ru/
    

26. Re: KNNGiST for knn-search (WIP)

    Robert Haas <robertmhaas@gmail.com> — 2010-01-15T01:49:26Z

    2010/1/12 Teodor Sigaev <teodor@sigaev.ru>:
    > Changes:
    >
    > - split patch to several ones
    > - sync with current CVS
    >
    > Patch set is based on 0.5.1 version, difference between 0.5 and 0.6 should
    > be only in planner patch.
    >
    > builtin_knngist_itself-0.6.gz  - patch to the gist itself
    > builtin_knngist_proc-0.6.gz - patch for support knnsearch in point_ops
    > builtin_knngist_planner-0.6.gz - planner patch to support knnearch
    > builtin_knngist_contrib_btree_gist-0.6.gz - patch for contrib/btree_gist
    > module
    >               patch provides <-> operation for various scalar types which is
    >               exactly  abs(a - b) function
    > builtin_knngist_contrib_pg_trgm-0.6.gz - contrib/pg_trgm, like above,patch
    >               provides <-> distance between strings
    >
    >
    > Patch set sill requires rbtree patch and point_ops patch (with Robert's
    > changes)
    
    Please update commitfest.postgresql.org.
    
    ...Robert