Re: WIP: Covering + unique indexes.

On Fri, Mar 30, 2018 at 4:24 PM, Alexander Korotkov <
a(dot)korotkov(at)postgrespro(dot)ru> wrote:

> On Fri, Mar 30, 2018 at 2:33 AM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
>
>> On Wed, Mar 28, 2018 at 7:59 AM, Anastasia Lubennikova
>> <a(dot)lubennikova(at)postgrespro(dot)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.
>

So, I have some implementation of storage of number of attributes inside
index tuple itself. I made it as additional patch on top of previous
patchset.
I attach the whole patchset in order to make commitfest.cputube.org happy.

I decided not to use 13th bit of IndexTuple flags. Instead I use only high
bit
of offset which is also always free on regular tuples. In fact, we already
use
assumption that there is at most 11 significant bits of index tuple offset
in
GIN (see ginpostinglist.c).

Anastasia also pointed that if we're going to do on-disk changes, they
should be compatible not only with suffix truncation, but also with
duplicate
compression (which was already posted in thread [1]). However, I think
there is no problem. We can use one of 3 free bits in offset as flag that
it's tuple with posting list. Duplicates compression needs to store
number of posting list items and their offset in the tuple. Free bits
left in item pointer after reserving 2 bits (1 flag of alternative meaning
of offset and 1 flag of posting list) is far enough for that.

However, I find following arguments against implementing this feature
in covering indexes.

* We write number of attributes present into tuple. But how to prove that
it's correct. I add appropriate checks to amcheck. But I don't think all
the
users runs amcheck frequent enough. Thus, in order to be sure that it's
correct we should check number of attributes is written correct everywhere
in the B-tree code. Without that we can face the situation that we've
introduced new on-disk representation better to further B-tree enhancements,
but actually it's broken. And that would be much worse than nothing.
In order to check number of attributes everywhere in the B-tree code, we
need to actually implement significant part of suffix compression. And I
really think we shouldn't do this as part as covering indexes patch.
* Offset number is used now for parent refind (see BTEntrySame() macro).
In the attached patch, this condition is relaxed. But I don't think I
really like
that. This shoud be thought out very carefully...
* Now, hikeys are copied together with original t_tid's. That makes it
possible
to find the origin of this hikey. If we override offset in t_tid, that
becomes not
always possible.
* When index tuple is truncated, then pageinspect probably shouldn't show
offset for it, because it meaningless. Should it rather show number of
attributes in separate column? Anyway that should be part of suffix
truncation
patch. Not part of covering indexes patch, especially added at the last
moment.
* I don't really see how does covering indexes without storing number of
index tuple attributes in the tuple itself blocks future work on suffix
truncation.
The code we have after covering indexes doesn't expect more than nkeyatts
number of attributes in pivot tuples. So, suffix truncation will make them
(sometimes) even shorter. And that smaller number of attributes may be
stored in the tuple itself. But default pivot tuple would be still assumed
to have
nkeyatts. I see no problem there.

So, taking into account the arguments of above, I propose to give up with
idea to stick covering indexes and suffix truncation features together.
That wouldn't accelerate appearance one feature after another, but rather
likely would RIP both of them...

1.
https://www.postgresql.org/message-id/flat/56AB6D30(dot)2040900%40postgrespro(dot)ru#56AB6D30(dot)2040900(at)postgrespro(dot)ru

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