Re: WIP: Covering + unique indexes. - Mailing 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.indexrelid) AS indexdef, i.indnkeya...

                                                             ^

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(RelationGetDescr(idxrel));

+ 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 would also like all the patches in patchset version to have same version number.

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.