knngist patch preliminary review (2010-09 commitfest)

I haven't quite finished reviewing this, but here is the position so
far. I'm going to continue with some performance and other tests, but
I'm posting this for discussion in the mean time.

1) General patch issues

- applies cleanly and passes regression

- one small warning with ecpg parser build, which I assume is just due
to the patch having touched the main parser in a way the ecpg build
doesn't expect (presumably this will be cleaned up at some later stage)

- *no* documentation (see below)

2) Design questions

Reading through the previous discussion on -hackers, I didn't get the
impression that there was agreement on the question of how to
represent the ordering operators in the catalog. This patch takes the
approach of adding a boolean column pg_amop.amoporder and doing
changes that require touching unrelated code in a fair number of places.

In addition there are the points Robert Haas expressed in his earlier
response.

This approach doesn't feel right to me, but I don't know enough about
it (especially any possible other features that might want to do
something similar) to express a strong opinion on it. So that point is
open for discussion.

3) Documentation

There are problems with the GiST docs that go much deeper than this
patch alone; the manual sections on writing GiST support functions are
wholly inadequate, and many features that have been around for a long
time, such as secondary split in GiST picksplit functions, are essentially
undocumented other than as (uncommented!) example code in contrib modules.

This patch would, as it stands, make that issue worse.

My opinion is that the gist-implementation section of the docs needs
to be substantially expanded so that it explains, for each support
function, exactly what the function is expected to do.

If it would help, I'm prepared to put some time towards actually
writing something up for this.

--
Andrew (irc:RhodiumToad)