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