Re: knngist - 0.8

2010/9/7 Teodor Sigaev <teodor(at)sigaev(dot)ru>:
>> 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.
>
> Oleg promised to do that

Fair enough, but where is it? It's kind of difficult even to review
this without some documentation, and you wrote this patch in 2009.
And as Tom pointed out the last time we reviewed this, lack of
documentation is sufficient grounds for rejection in and of itself.

http://archives.postgresql.org/pgsql-hackers/2010-02/msg00820.php

>> +               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.
>
> This play is around do we really want to have support of boolean-distance in
> GiST? I think no, because it's a strange idea to measure distance in
> true/false measurement units. I can't imagine such real-life distance
> definition and never heard about that.
>
> Next, pg_amop_opr_fam_index requires uniqueness of operation in operation
> family and a lot of places in planner believes in that. Suppose, changing
> that requires a lot of work which has the single aim to support boolean
> distance in ORDER BY clause.

Tom and I both expressed the opinion 7 months ago that we don't think
this design is acceptable.

http://archives.postgresql.org/pgsql-hackers/2010-02/msg01063.php

I'm not sure why you expect it to be acceptable now if it wasn't
acceptable then. I'm sort of surprised that you haven't taken the
intervening 7 months to rework it along the lines discussed. We had a
very long and detailed discussion about how to make it work, and
committed a huge, invasive patch that Tom's going to be grumpy about
for years to provide the infrastructure for 5-key syscaches --
specifically so you'd be able change pg_amop_fam_strat_index to use a
5-part key. I would actually think that would be a fairly mechanical
transformation.

It seems to me that there is not very much point in reviewing this
further until you incorporate the feedback that was given last time,
and specifically the two major points discussed above.

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