Re: Making all nbtree entries unique by having heap TIDs participatein comparisons
От | Peter Geoghegan |
---|---|
Тема | Re: Making all nbtree entries unique by having heap TIDs participatein comparisons |
Дата | |
Msg-id | CAH2-WzkTEUGMrVnUrMDrYe_6ycHYNz56yTeAhq01JtoAMzxT-w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Making all nbtree entries unique by having heap TIDs participatein comparisons (Heikki Linnakangas <hlinnaka@iki.fi>) |
Ответы |
Re: Making all nbtree entries unique by having heap TIDs participatein comparisons
(Peter Geoghegan <pg@bowt.ie>)
|
Список | pgsql-hackers |
On Mon, Mar 18, 2019 at 4:59 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > I'm getting a regression failure from the 'create_table' test with this: > Are you seeing that? Yes -- though the bug is in your revised v18, not the original v18, which passed CFTester. Your revision fails on Travis/Linux, which is pretty close to what I see locally, and much less subtle than the test failures you mentioned: https://travis-ci.org/postgresql-cfbot/postgresql/builds/507816665 "make check" did pass locally for me with your patch, but "make check-world" (parallel recipe) did not. The original v18 passed both CFTester tests about 15 hour ago: https://travis-ci.org/postgresql-cfbot/postgresql/builds/507643402 I see the bug. You're not supposed to test this way with a heapkeyspace index: > + if (P_RIGHTMOST(lpageop) || > + _bt_compare(rel, itup_key, page, P_HIKEY) != 0) > + break; This is because the presence of scantid makes it almost certain that you'll break when you shouldn't. You're doing it the old way, which is inappropriate for a heapkeyspace index. Note that it would probably take much longer to notice this bug if the "consider secondary factors" patch was also applied, because then you would rarely have cause to step right here (duplicates would never occupy more than a single page in the regression tests). The test failures are probably also timing sensitive, though they happen very reliably on my machine. > Looking at the patches 1 and 2 again: > > I'm still not totally happy with the program flow and all the conditions > in _bt_findsplitloc(). I have a hard time following which codepaths are > taken when. I refactored that, so that there is a separate copy of the > loop for V3 and V4 indexes. The big difference is that you make the possible call to _bt_stepright() conditional on this being a checkingunique index -- the duplicate code is indented in that branch of _bt_findsplitloc(). Whereas I break early in the loop when "checkingunique && heapkeyspace". The flow of the original loop not only had less code. It also contrasted the important differences between heapkeyspace and !heapkeyspace cases: /* If this is the page that the tuple must go on, stop */ if (P_RIGHTMOST(lpageop)) break; cmpval = _bt_compare(rel, itup_key, page, P_HIKEY); if (itup_key->heapkeyspace) { if (cmpval <= 0) break; } else { /* * pg_upgrade'd !heapkeyspace index. * * May have to handle legacy case where there is a choice of which * page to place new tuple on, and we must balance space * utilization as best we can. Note that this may invalidate * cached bounds for us. */ if (cmpval != 0 || _bt_useduplicatepage(rel, heapRel, insertstate)) break; } I thought it was obvious that the "cmpval <= 0" code was different for a reason. It now seems that this at least needs a comment. I still believe that the best way to handle the !heapkeyspace case is to make it similar to the heapkeyspace checkingunique case, regardless of whether or not we're checkingunique. The fact that this bug slipped in supports that view. Besides, the alternative that you suggest treats !heapkeyspace indexes as if they were just as important to the reader, which seems inappropriate (better to make the legacy case follow the new case, not the other way around). I'm fine with the comment tweaks that you made that are not related to _bt_findsplitloc(), though. I won't push the patches today, to give you the opportunity to respond. I am not at all convinced right now, though. -- Peter Geoghegan
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Peter EisentrautДата:
Сообщение: Re: Fix optimization of foreign-key on update actions
Следующее
От: Yun LiДата:
Сообщение: Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activityview?