Re: WIP: Covering + unique indexes.

On Wed, Mar 21, 2018 at 9:51 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> 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.
>

I have few more notes regarding this patchset.

* indkeyatts is described in the documentation, but I think that
description of indnatts
should be also updated clarifying that indnatts counts "included" columns.

+ <row>
+ <entry><structfield>indnkeyatts</structfield></entry>
+ <entry><type>int2</type></entry>
+ <entry></entry>
+ <entry>The number of key columns in the index. "Key columns" are
ordinary
+ index columns (as opposed to "included" columns).</entry>
+ </row>

* It seems like this paragraph appears in the patchset without any
mentioning
in the thread.

+Notes to Operator Class Implementors
+------------------------------------

Besides I really appreciate it, it seems to be unrelated to the covering
indexes.
I'd like this to be extracted into separate patch and be committed
separately.

* There is a typo here: brtee -> btree
+ * 7. Check various AMs. All but brtee must fail.

* This comment should be updated assuming that we now put left page
hikey to the WAL independently on whether it's leaf page split.

+ /*
+ * We must also log the left page's high key, because the right
+ * page's leftmost key is suppressed on non-leaf levels. Show it
+ * as belonging to the left page buffer, so that it is not stored
+ * if XLogInsert decides it needs a full-page image of the left
+ * page.
+ */

* get_index_def() is adjusted to support covering indexes. I think this
support
deserve to be checked in regression tests.

* In PostgreSQL sentences are sometimes divided by single spacing, sometimes
divided by double spacing. I think we should follow general rule here:
code should
look like its surroundings. Could you please recheck that through the
patch.
I notices that especially in documentation you frequently use single
spacing while
surrounding uses double spacing.

Rest of things look OK to me for now.

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