Re: [PATCH] Covering SPGiST index

I added some extra comments and mentions in manual to make all the things clear (see v7 patch) 

 

The patch implements the proposed functionality, passes tests, and in general looks good to me.
I don't mind using a macro to differentiate tuples with and without included attributes. Any approach will require code changes. Though, I don't have a strong opinion about that.
 
A bit of nitpicking:

1) You mention backward compatibility in some comments. But, after this patch will be committed, it will be uneasy to distinct new and old phrases.  So I suggest to rephrase them.  You can either refer a specific version or just call it "compatibility with indexes without included attributes".

2) SpgLeafSize() function name seems misleading, as it actually refers to a tuple's size, not a leaf page. I suggest to rename it to SpgLeafTupleSize().

3) I didn't quite get the meaning of the assertion, that is added in a few places:
     Assert(so->state.includeTupdesc->natts);
Should it be Assert(so->state.includeTupdesc->natts > 1) ?

4) There are a few typos in comments and docs:
s/colums/columns
s/include attribute/included attribute

and so on.

5) This comment in index_including.sql is outdated:
  * 7. Check various AMs. All but btree and gist must fail.

6) New test lacks SET enable_seqscan TO off;
in addition to SET enable_bitmapscan TO off;

I also wonder, why both index_including_spgist.sql and index_including.sql tests are stable without running 'vacuum analyze' before the EXPLAIN that shows Index Only Scan plan. Is autovacuum just always fast enough to fill a visibility map, or I miss something?

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company