Re: WIP: Covering + unique indexes.

On Mon, Apr 16, 2018 at 1:05 AM, Peter Geoghegan <pg@bowt.ie> wrote:

> Attached patch makes the changes that I talked about, and a few
> others. The commit message has full details. The general direction of
> the patch is that it documents our assumptions, and verifies them in
> more cases. Most of the changes I've made are clear improvements,
> though in a few cases I've made changes that are perhaps more
> debatable.


Great, thank you very much!


> These other, more debatable cases are:
>
> * The comments added to _bt_isequal() about suffix truncation may not
> be to your taste. The same is true of the way that I restored the
> previous _bt_isequal() function signature. (Yes, I want to change it
> back despite the fact that I was the person that originally suggested
> we change _bt_isequal().)
>

Hmm, what do you think about making BTreeTupGetNAtts() take tupledesc
argument, not relation>  It anyway doesn't need number of key attributes,
only total number of attributes.  Then _bt_isequal() would be able to use
BTreeTupGetNAtts().

* I added BTreeTupSetNAtts() calls to a few places that don't truly
> need them, such as the point where we generate a dummy 0-attribute
> high key within _bt_mark_page_halfdead(). I think that we should try
> to be as consistent as possible about using BTreeTupSetNAtts(), to set
> a good example. I don't think it's necessary to use BTreeTupSetNAtts()
> for pivot tuples when the number of key attributes matches indnatts
> (it seems inconvenient to have to palloc() our own scratch buffer to
> do this when we don't have to), but that doesn't apply to these
> now-covered cases.
>

+1

>
> > I think, we need move _bt_check_natts() and its call under
> > USE_ASSERT_CHECKING to prevent performance degradation. Users shouldn't
> pay
> > for unused feature.
>
> I eventually decided that you were right about this, and made the
> _bt_compare() call to _bt_check_natts() a simple assertion without
> waiting to hear more opinions on the matter. Concurrency isn't a
> factor here, so adding a check to standard release builds isn't
> particularly likely to detect bugs. Besides, there is really only a
> small number of places that need to do truncation for themselves. And,
> if you want to be sure that the structure is consistent in the field,
> there is always amcheck, which can check _bt_check_natts() (while also
> checking other things that we care about just as much).
>

Good point, risk of performance degradation caused by _bt_check_natts()
in _bt_compare() is high.  So, let's move in to assertion.

Note that I removed some dead code from _bt_insertonpg() that wasn't
> added by the INCLUDE patch. It confused matters for this patch, since
> we don't want to consider what's supposed to happen when there is a
> retail insertion of a new, second negative infinity item -- clearly,
> that should simply never happen (I thought about adding a
> BTreeTupSetNAtts() call, but then decided to just remove the dead code
> and add a new "can't happen" elog error).


I think it's completely OK to fix broken things when you've to touch
them.  Probably, Teodor would decide to make that by separate commit.
So, it's up to him.


> Finally, I made sure that we
> don't drop all tables in the regression tests, so that we have some
> pg_dump coverage for INCLUDE indexes, per a request from Tom.
>

Makes sense, because that've already appeared to be broken.

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