Re: knngist - 0.8

2010/7/22 Teodor Sigaev <teodor@sigaev.ru>:
> http://www.sigaev.ru/misc/builtin_knngist_core-0.8.gz
> http://www.sigaev.ru/misc/builtin_knngist_itself-0.8.gz
> http://www.sigaev.ru/misc/builtin_knngist_proc-0.8.gz
> http://www.sigaev.ru/misc/builtin_knngist_contrib_pg_trgm-0.8.gz
> http://www.sigaev.ru/misc/builtin_knngist_contrib_btree_gist-0.8.gz
>
> Changes:
> * pg_amop has boolean column amoporder which points to clause's usage of
>  operator
> * Syntax of CREATE OPERATOR CLASS
>     CREATE OPERATOR CLASS ...
>         [ORDER] OPERATOR ....
>  ORDER OPERATOR is marked with pg_amop.amoporder = true
> * Bool-returning operator could not be used as ORDER OPERATOR, but type of
>  returning value still should have a default Btree operator class.
> * Added flag SK_ORDER to SkanKey flag to indicate order operation, so access
>  methods (only GiST right now) should check this flag (in previous versions
> of
>  patch GiST checked returned value of operator's function)

AFAICS, these patches include no documentation.  That's pretty much a
fatal flaw for a feature of this magnitude.  At an absolute minimum,
you need to update the system catalog documentation and the
documentation on CREATE / ALTER OPERATOR CLASS.  There might be some
other places that need to be touched, also.

+               if (opform->oprresult == BOOLOID)
+                       ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                                        errmsg("index ordering
operators must not return boolean")));

My first thought was that this code was there to prevent people from
doing the wrong thing by accident.  But I have a niggling feeling that
you're actually relying on this for the correctness of the system.  I
hope I'm wrong, because I don't think that would be a very good idea.

The GIST code code use more comments; and perhaps the names of some of
the functions and structures could be chosen to be more descriptive.
I think that what used to be called GISTSearchStack has apparently
been replaced with DataPointer; it's not obvious to me that it's good
to change the name, but if it is I don't think DataPointer is a good
choice.  gistindex_keytest has been replaced (sort of) by
processIndexTuple, which again seems more generic than what it
replaced.

Minor nit: the word "shoould" is mis-spelled.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company