Re: WIP: Covering + unique indexes.

On Thu, Apr 12, 2018 at 9:21 AM, Teodor Sigaev <teodor(at)sigaev(dot)ru> wrote:
> Agree, I prefer to add more Assert, even. may be, more than actually needed.
> Assert-documented code :)

Absolutely. The danger with a feature like this is that we'll miss one
place. I suppose that you could say that I am in the Poul-Henning Kamp
camp on assertions [1].

>> I'll add an item to "Decisions to Recheck Mid-Beta" section of the
>> open items page for this patch. We should review the decision to make
>> a call to _bt_check_natts() within _bt_compare(). It might work just
>> as well as an assertion, and it would be unfortunate if workloads that
>> don't use covering indexes had to pay a price for the
>> _bt_check_natts() call, even if it was a small price. I've seen
>> _bt_compare() appear prominently in profiles quite a few times.
>>
>
> Could you show a patch?

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

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

I imagine that you'll have no problem with the other changes in the
patch, which is why I haven't mentioned them here. Let me know what
you think.

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

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

[1] https://queue.acm.org/detail.cfm?id=2220317
--
Peter Geoghegan