Re: [PATCH] Covering SPGiST index

On 24.08.2020 16:19, Pavel Borisov wrote:
>
> 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