2010/7/22 Teodor Sigaev <teo...@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
