Re: WIP: Covering + unique indexes.

On Thu, Mar 8, 2018 at 7:13 PM, Anastasia Lubennikova <
a(dot)lubennikova(at)postgrespro(dot)ru> wrote:

> 06.03.2018 11:52, Thomas Munro:
>
>> On Wed, Jan 31, 2018 at 3:09 AM, Anastasia Lubennikova
>> <a(dot)lubennikova(at)postgrespro(dot)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(Rela
tionGetDescr(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.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company