Re: WIP: Covering + unique indexes. - Mailing list pgsql-hackers
| From | Alexander Korotkov |
|---|---|
| Subject | Re: WIP: Covering + unique indexes. |
| Date | |
| Msg-id | CAPpHfdvPv8BJVNygLrNkyQDUcyKFhsa25epr0m3cExzxmek8VA@mail.gmail.com Whole thread Raw |
| In response to | Re: WIP: Covering + unique indexes. (Anastasia Lubennikova <a.lubennikova@postgrespro.ru>) |
| Responses |
Re: WIP: Covering + unique indexes.
(Alexander Korotkov <a.korotkov@postgrespro.ru>)
Re: WIP: Covering + unique indexes. (Andrey Borodin <x4mmm@yandex-team.ru>) |
| List | pgsql-hackers |
On Thu, Mar 8, 2018 at 7:13 PM, Anastasia Lubennikova <a.lubennikova@postgrespro.ru> wrote:
06.03.2018 11:52, Thomas Munro:On Wed, Jan 31, 2018 at 3:09 AM, Anastasia Lubennikova
<a.lubennikova@postgrespro.ru> wrote:Thank you for reviewing. All mentioned issues are fixed.== Applying patch 0002-covering-btree_v4.patch...
1 out of 1 hunk FAILED -- saving rejects to file
src/backend/access/nbtree/README.rej
1 out of 1 hunk FAILED -- saving rejects to file
src/backend/access/nbtree/nbtxlog.c.rej
Can we please have a new patch set?
Here it is.
Many thanks to Andrey Borodin.
I took a look at this patchset. I have some notes about it.
* I see patch changes dblink, amcheck and tcl contribs. It would be nice to add corresponding
check to dblink and amcheck regression tests. It would be good to do the same with tcn contrib.
But tcn doesn't have regression tests at all. And it's out of scope of this patch to add regression
tests to tcn. So, it's OK to just check that it's working correctly with covering indexes (I hope it's
already done by other reviewers).
* I think that subscription regression tests in src/test/subscription should have some use
of covering indexes. Logical decoding and subscription are heavily using primary keys.
So they need to be tested to work correctly with covering indexes.
* I also think some isolation tests in src/test/isolation need to check covering indexes too.
In particular insert-conflict-*.spec and lock-*.spec and probably more.
* pg_dump doesn't handle old PostgreSQL versions correctly. If I try to dump database
of PostgreSQL 9.6, pg_dump gives me following error:
pg_dump: [archiver (db)] query failed: ERROR: column i.indnkeyatts does not exist
LINE 1: ...atalog.pg_get_indexdef(i.
^
If fact there is a sequence of "if" ... "else if" blocks in getIndexes() which selects
appropriate query depending on remote server version. And for pre-11 we should
use indnatts instead of indnkeyatts.
* There is minor formatting issue in this part of code. Some spaces need to be replaced with tabs.
+IndexTuple
+index_truncate_tuple(Relation idxrel, IndexTuple olditup)
+{
+ TupleDesc itupdesc = CreateTupleDescCopyConstr(Rela
+ Datum values[INDEX_MAX_KEYS];
+ bool isnull[INDEX_MAX_KEYS];
+ IndexTuple newitup;
* I think this comment needs to be rephrased.
+ /*
+ * Code below is concerned to the opclasses which are not used
+ * with the included columns.
+ */
I would write something like this: "Code below checks opclass key type. Included columns
don't have opclasses, and this check is not required for them.". Native english speakers
could provide even better phrasing though.
I understand that "Covering-btree" and "Covering-amcheck" have less previous
versions than "Covering-core". But it's way easier to identify patches belonging to
the same patchset version if they have same version number. For sure, then some
patches would skip some version numbers, but that doesn't seem to be a problem for me.
------
Alexander Korotkov
Postgres Professional: http://www.postg
The Russian Postgres Company
Alexander Korotkov
Postgres Professional: http://www.postg
The Russian Postgres Company
pgsql-hackers by date: