Re: WIP: Covering + unique indexes.

Hi!

I've revised a patchset.  It has improved comments and documentation.

I also updated some tests:
 * I've fixed checks on adding primary keys with included
columns in index_including.sql.  Previously all tries to
add primary keys were failed, I made some of them pass.
 * pg_index_def() is now covered by regression tests.
 * I made some use of covering indexes in isolation tests,
because covering indexes made some changes to row-level
locks.  Instead of adding extra tests which could significantly
increase isolation check runtime, I just replaced some of
indexes with covering indexes in existing tests.

Also this patchset have fix for logical subscription from Anastasia.

On Fri, Mar 30, 2018 at 2:33 AM, Peter Geoghegan <pg@bowt.ie> wrote:

> On Wed, Mar 28, 2018 at 7:59 AM, Anastasia Lubennikova
> <a.lubennikova@postgrespro.ru> wrote:
> > Here is the new version of the patch set.
> > All patches are rebased to apply without conflicts.
> >
> > Besides, they contain following fixes:
> > - pg_dump bug is fixed
> > - index_truncate_tuple() now has 3rd argument new_indnatts.
> > - new tests for amcheck, dblink and subscription/t/001_rep_changes.pl
> > - info about opclass implementors and included columns is now in sgml doc
>
> This only changes the arguments given to index_truncate_tuple(), which
> is a superficial change. It does not actually change anything about
> the on-disk representation, which is what I sought. Why is that a
> problem? I don't think it's very complicated.
>

I'll try it.  But I'm afraid that it's not as easy as you expect.


> The patch needs a rebase, as Erik mentioned:
>
> 1 out of 19 hunks FAILED -- saving rejects to file
> src/backend/utils/cache/relcache.c.rej
> (Stripping trailing CRs from patch; use --binary to disable.)
>
> I also noticed that you still haven't done anything differently with
> this code in _bt_checksplitloc(), which I mentioned in April of last
> year:
>
>     /* Account for all the old tuples */
>     leftfree = state->leftspace - olddataitemstoleft;
>     rightfree = state->rightspace -
>         (state->olddataitemstotal - olddataitemstoleft);
>
>     /*
>      * The first item on the right page becomes the high key of the left
> page;
>      * therefore it counts against left space as well as right space.
>      */
>     leftfree -= firstrightitemsz;
>
>     /* account for the new item */
>     if (newitemonleft)
>         leftfree -= (int) state->newitemsz;
>     else
>         rightfree -= (int) state->newitemsz;
>
> With an extreme enough case, this could result in a failure to find a
> split point. Or at least, if that isn't true then it's not clear why,
> and I think it needs to be explained.
>

I don't think this could result in a failure to find a split point.
So, it finds a split point without taking into account that hikey
will be shorter.  If such split point exists then split point with
truncated hikey should also exists.  If not, then it would be
failure even without covering indexes.  I've updated comment
accordingly.

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