From: | Anastasia Lubennikova <a(dot)lubennikova(at)postgrespro(dot)ru> |
---|---|
To: | Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com>, "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru> |
Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] Covering SPGiST index |
Date: | 2020-08-26 22:03:49 |
Message-ID: | 60bfcb1f-48b5-d971-4be3-58c8765d2862@postgrespro.ru |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Ibrar Ahmed | 2020-08-26 22:19:45 | Re: COPY FREEZE and setting PD_ALL_VISIBLE/visibility map bits |
Previous Message | Tom Lane | 2020-08-26 21:43:36 | Re: CREATE RULE may generate duplicate entries in pg_depend |