Re: WIP: Covering + unique indexes.

01.04.2017 02:31, Peter Geoghegan:
>
> * index_truncate_tuple() should have as an argument the number of
> attributes. No need to "#include utils/rel.h" that way.
Will fix.
>
> * I think that we should store this (the number of attributes), and
> use it directly when comparing, per my remarks to Tom over on that
> other thread. We should also use the free bit within
> IndexTupleData.t_info, to indicate that the IndexTuple was truncated,
> just to make it clear to everyone that might care that that's how
> these truncated IndexTuples need to be represented.
>
> Doing this would have no real impact on your patch, because for you
> this will be 100% redundant. It will help external tools, and perhaps
> another, more general suffix truncation patch that comes in the
> future. We should try very hard to have a future-proof on-disk
> representation. I think that this is quite possible.
To be honest, I think that it'll make the patch overcomplified, because
this exact patch has nothing to do with suffix truncation.
Although, we can add any necessary flags if this work will be continued
in the future.
> * I suggest adding a "can't happen" defensive check + error that
> checks that the tuple returned by index_truncate_tuple() is sized <=
> the original. This cannot be allowed to ever happen. (Not that I think
> it will.)
There is already an assertion.
Assert(IndexTupleSize(newitup) <= IndexTupleSize(olditup));
Do you think it is not enough?
> * I see a small bug. You forgot to teach _bt_findsplitloc() about
> truncation. It does this currently, which you did not update:
>
> /*
> * 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;
>
> I think that this accounting needs to be fixed.
Could you explain, what's wrong with this accounting? We may expect to
take more space on the left page, than will be taken after highkey
truncation. But I don't see any problem here.

> * Note sure about one thing. What's the reason for this change?
>
>> - /* Log left page */
>> - if (!isleaf)
>> - {
>> - /*
>> - * 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.
>> - */
>> - itemid = PageGetItemId(origpage, P_HIKEY);
>> - item = (IndexTuple) PageGetItem(origpage, itemid);
>> - XLogRegisterBufData(0, (char *) item, MAXALIGN(IndexTupleSize(item)));
>> - }
>> + /*
>> + * 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.
>> + */
>> + itemid = PageGetItemId(origpage, P_HIKEY);
>> + item = (IndexTuple) PageGetItem(origpage, itemid);
>> + XLogRegisterBufData(0, (char *) item, MAXALIGN(IndexTupleSize(item)));
> Is this related to the problem that you mentioned to me that you'd
> fixed when we spoke in person earlier today? You said something about
> WAL logging, but I don't recall any details. I don't remember seeing
> this change in prior versions.
>
> Anyway, whatever the reason for doing this on the leaf level now, the
> comments should be updated to explain it.
This change related to the bug described in this message.
https://www.postgresql.org/message-id/20170330192706.GA2565%40e733.localdomain
After fix it is not reproducible. I will update comments in the next patch.
> * Speaking of WAL-logging, I think that there is another bug in
> btree_xlog_split(). You didn't change this existing code at all:
>
> /*
> * On leaf level, the high key of the left page is equal to the first key
> * on the right page.
> */
> if (isleaf)
> {
> ItemId hiItemId = PageGetItemId(rpage, P_FIRSTDATAKEY(ropaque));
>
> left_hikey = PageGetItem(rpage, hiItemId);
> left_hikeysz = ItemIdGetLength(hiItemId);
> }
>
> It seems like this was missed when you changed WAL-logging, since you
> do something for this on the logging side, but not here, on the replay
> side. No?
>
I changed it. Now we always use highkey saved in xlog.
This code don't needed anymore and can be deleted. Thank you for the
notice. I will send updated patch today.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company