Re: WIP: Covering + unique indexes.
От | Peter Geoghegan |
---|---|
Тема | Re: WIP: Covering + unique indexes. |
Дата | |
Msg-id | CAH2-WzmiWHiEMnHQFZbg=ijOF=vmdh-31K=w9Pt+7VEb8KrfPg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: WIP: Covering + unique indexes. (Teodor Sigaev <teodor@sigaev.ru>) |
Ответы |
Re: WIP: Covering + unique indexes.
(Alexander Korotkov <a.korotkov@postgrespro.ru>)
|
Список | pgsql-hackers |
On Thu, Apr 12, 2018 at 9:21 AM, Teodor Sigaev <teodor@sigaev.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
Вложения
В списке pgsql-hackers по дате отправления: