Re: [PATCH] kNN for btree

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi,
thank you for your work on this patch.

Patch #1 is ready for commit.
It only fixes lack of refactoring after INCLUDE index patch.

Patches 2-7 are ready for committer's review.
I found no significant problems in algorithm or implementation.
Here are few suggestions to improve readability:

1) patch 0002.
* Returned matched index clause exression.
* Number of matched index column is returned in *indexcol_p.

Typos in comment:
exPression
index columnS

2) patch 0002.
+ /*
+ * We allow any column of this index to match each pathkey; they
+ * don't have to match left-to-right as you might expect.
+ */

Before refactoring this comment had a line about gist and sp-gist specific:

- * We allow any column of the index to match each pathkey; they
- * don't have to match left-to-right as you might expect. This is
- * correct for GiST, and it doesn't matter for SP-GiST because
- * that doesn't handle multiple columns anyway, and no other
- * existing AMs support amcanorderbyop. We might need different
- * logic in future for other implementations.

Now, when the code was moved to a separate function, it is not clear why the same logic is ok for b-tree and other index methods.
If I got it right, it doesn't affect the correctness of the algorithm, because b-tree specific checks are performed in another function.
Still it would be better to explain it in the comment for future readers.

3) patch 0004
if (!so->distanceTypeByVal)
{
so->state.currDistance = PointerGetDatum(NULL);
so->state.markDistance = PointerGetDatum(NULL);
}

Why do we reset these fields only for !distanceTypeByVal?

4) patch 0004
+ * _bt_next_item() -- Advance to next tuple on current page;
+ * or if there's no more, try to step to the next page with data.
+ *
+ * If there are no more matching records in the given direction
*/

Looks like the last sentence of the comment is unfinished.

5) patch 0004
_bt_start_knn_scan()

so->currRightIsNearest = _bt_compare_current_dist(so, rstate, lstate);
/* Reset right flag if the left item is nearer. */
right = so->currRightIsNearest;

If this comment relates to the string above it?

6) patch 0004
_bt_parallel_seize()

+ scanPage = state == &so->state
+ ? &btscan->btps_scanPage
+ : &btscan->btps_knnScanPage;

This code looks a bit tricke to me. Why do we need to pass BTScanState state to _bt_parallel_seize() at all?
Won't it be enough to choose the page before function call and pass it?

7) patch 0006
+ <title>Upgrade notes for version 1.6</title>
+
+ <para>
+ In version 1.6 <literal>btree_gist</literal> switched to using in-core
+ distance operators, and its own implementations were removed. References to
+ these operators in <literal>btree_gist</literal> opclasses will be updated
+ automatically during the extension upgrade, but if the user has created
+ objects referencing these operators or functions, then these objects must be
+ dropped manually before updating the extension.

Is this comment still relevant after the latest changes?
Functions are not removed, they're still in contrib.

Patches #8 and #9 need more review and tests.
I'll try to give a feedback on them in the week.

P.S. many thanks for splitting the code into separate patches. It made review a lot easier.

The new status of this patch is: Waiting on Author