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
