Thread: Handling GIN incomplete splits
Here's another part of my crusade against xlog cleanup routines. This series of patches gets rid of the gin_cleanup() function, which is currently used to finish splits of GIN b-tree pages, if the system crashes (or an error occurs) between splitting a page and inserting its downlink to the parent. The first three patches just move code around. IMHO they make the code more readable, so they should be committed in any case. The meat is in the fourth patch. Thoughts, objections? Alexander, I'm sorry if this conflicts with your GIN patches. Feel free to post the latest versions of your patches against the current master, ignoring patches. I can fix the bitrot. That said, I think these refactorings will make your code look a little bit nicer too, so you might want to rebase because of that anyway. - Heikki
Attachment
Hi,
Here is a review of the first three patches:
1) Further gin refactoring:
make check passes (core tests and contrib tests).
Code compiles without warnings.
Here is a review of the first three patches:
1) Further gin refactoring:
make check passes (core tests and contrib tests).
Code compiles without warnings.
Then... About the patch... Even if I got little experience with code of gin, moving the flag for search mode out of btree, as well as removing the logic of PostingTreeScan really makes the code lighter and easier to follow.
Just wondering, why not simplifying as well ginTraverseLock:ginbtree.c at the same time to something similar to that?
if (!GinPageIsLeaf(page) || searchMode == TRUE)
return access;
/* we should relock our page */
LockBuffer(buffer, GIN_UNLOCK);
LockBuffer(buffer, GIN_EXCLUSIVE);
/* But root can become non-leaf during relock */
if (!GinPageIsLeaf(page))
{
/* restore old lock type (very rare) */
LockBuffer(buffer, GIN_UNLOCK);
LockBuffer(buffer, GIN_SHARE);
}
else
access = GIN_EXCLUSIVE;
return access;
Just wondering, why not simplifying as well ginTraverseLock:ginbtree.c at the same time to something similar to that?
if (!GinPageIsLeaf(page) || searchMode == TRUE)
return access;
/* we should relock our page */
LockBuffer(buffer, GIN_UNLOCK);
LockBuffer(buffer, GIN_EXCLUSIVE);
/* But root can become non-leaf during relock */
if (!GinPageIsLeaf(page))
{
/* restore old lock type (very rare) */
LockBuffer(buffer, GIN_UNLOCK);
LockBuffer(buffer, GIN_SHARE);
}
else
access = GIN_EXCLUSIVE;
return access;
Feel free to discard as I can imagine that changing such code would make back-branch maintenance more difficult and it would increase conflicts with patches currently in development.
2) Refactoring of internal gin btree (needs patch 1 applied first):
make check passes (core tests and contrib tests).
Code compiles without warnings.
make check passes (core tests and contrib tests).
Code compiles without warnings.
Yep, removing ginPageGetLinkItup makes sense. Just to be picky, I would have put the arguments of GinFormInteriorTuple replacing ginPageGetLinkItup in 3 separate lines just for lisibility.
In dataPrepareDownlink:gindatapage.c, depending on if lpage is a leaf page or not, isn't it inconsistent with the older code not to use GinDataPageGetItemPointer and GinDataPageGetPostingItem to set btree->pitem.key.
In ginContinueSplit:ginxlog.c, could it be possible to remove this code? It looks that its deletion has been forgotten: /*
* elog(NOTICE,"ginContinueSplit root:%u l:%u r:%u", split->rootBlkno,
* split->leftBlkno, split->rightBlkno);
*/
* elog(NOTICE,"ginContinueSplit root:%u l:%u r:%u", split->rootBlkno,
* split->leftBlkno, split->rightBlkno);
*/
Except the doubt about dataPrepareDownlink (related to my lack of knowledge of the code), patch looks good.
3) More refactoring (needs patches 1 and 2):
make check passes (core tests and contrib tests).
Code compiles without warnings.
make check passes (core tests and contrib tests).
Code compiles without warnings.
Perhaps this patch would have been easier to read with context diffs :) It just moves code around so nothing to say.
Then, I have done a small test with all 3 patches applied. Test is done with pg_trgm by uploading the book "Les Miserables":
=# CREATE TABLE les_miserables (num serial, line text);
CREATE TABLE
=# \copy les_miserables (line) FROM '~/Desktop/pg135.txt';
=# select count(*) from les_miserables;
count
-------
68116
(1 row)
=# CREATE INDEX les_miserables_idx ON les_miserables USING gin (line gin_trgm_ops);
CREATE INDEX
And here is the result of this query (average of a couple of 5 runs):
=# explain analyse SELECT * FROM les_miserables where line ~~ '%Cosette%';
Vanilla server: 5.289 ms
With patch 1 only: 5.283 ms
=# CREATE TABLE les_miserables (num serial, line text);
CREATE TABLE
=# \copy les_miserables (line) FROM '~/Desktop/pg135.txt';
=# select count(*) from les_miserables;
count
-------
68116
(1 row)
=# CREATE INDEX les_miserables_idx ON les_miserables USING gin (line gin_trgm_ops);
CREATE INDEX
And here is the result of this query (average of a couple of 5 runs):
=# explain analyse SELECT * FROM les_miserables where line ~~ '%Cosette%';
Vanilla server: 5.289 ms
With patch 1 only: 5.283 ms
With patches 1+2: 5.232 ms
With patches 1+2+3: 5.232 ms
With patches 1+2+3: 5.232 ms
Based on that there is no performance degradation.
I just began reading the 4th patch. As it is a bit more complex and needs more testing, I'll provide feedback later.
Regards,
On Thu, Nov 14, 2013 at 1:49 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
Here's another part of my crusade against xlog cleanup routines. This series of patches gets rid of the gin_cleanup() function, which is currently used to finish splits of GIN b-tree pages, if the system crashes (or an error occurs) between splitting a page and inserting its downlink to the parent.
The first three patches just move code around. IMHO they make the code more readable, so they should be committed in any case. The meat is in the fourth patch.
Thoughts, objections?
--
Michael
Here are some comments about the 4th patch.
1) Compiles without warnings, passes make check.3) Server crashes when trying to create a gin index index creation (see example of previous email with pg_trgm). Here is the backtrace of the crash:
* thread #1: tid = 0x15221, 0x00007fff8c59f866 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread, stop reason = signal SIGABRT
frame #0: 0x00007fff8c59f866 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007fff8e60735c libsystem_pthread.dylib`pthread_kill + 92
frame #2: 0x00007fff91586bba libsystem_c.dylib`abort + 125
frame #3: 0x000000010e953ed9 postgres`ExceptionalCondition(conditionName=0x000000010ea31055, errorType=0x000000010e9b5973, fileName=0x000000010ea2fd3d, lineNumber=1175) + 137 at assert.c:54
frame #4: 0x000000010e79073a postgres`UnpinBuffer(buf=0x000000010f37f9c0, fixOwner='\0') + 282 at bufmgr.c:1175
frame #5: 0x000000010e793465 postgres`ReleaseBuffer(buffer=3169) + 565 at bufmgr.c:2540
frame #6: 0x000000010e414e43 postgres`freeGinBtreeStack(stack=0x00007fa2138adcf8) + 67 at ginbtree.c:196
frame #7: 0x000000010e415330 postgres`ginInsertValue(btree=0x00007fff51807e80, stack=0x00007fa2138a6dd8, insertdata=0x00007fff51807e70, buildStats=0x00007fff51809fa0) + 1216 at ginbtree.c:728
frame #8: 0x000000010e404ebf postgres`ginEntryInsert(ginstate=0x00007fff51807fe0, attnum=1, key=7828073, category='\0', items=0x0000000117d0ab28, nitem=76, buildStats=0x00007fff51809fa0) + 1055 at gininsert.c:218
frame #9: 0x000000010e405ad6 postgres`ginbuild(fcinfo=0x00007fff5180a050) + 1590 at gininsert.c:392
frame #10: 0x000000010e9609ba postgres`OidFunctionCall3Coll(functionId=2738, collation=0, arg1=4693605424, arg2=4693760992, arg3=140334089145912) + 186 at fmgr.c:1649
frame #11: 0x000000010e4f4b30 postgres`index_build(heapRelation=0x0000000117c2bc30, indexRelation=0x0000000117c51be0, indexInfo=0x00007fa213888e38, isprimary='\0', isreindex='\0') + 464 at index.c:1963
frame #12: 0x000000010e4f2f07 postgres`index_create(heapRelation=0x0000000117c2bc30, indexRelationName=0x00007fa213888b30, indexRelationId=16445, relFileNode=0, indexInfo=0x00007fa213888e38, indexColNames=0x00007fa2138892d8, accessMethodObjectId=2742, tableSpaceId=0, collationObjectId=0x00007fa213889330, classObjectId=0x00007fa213889350, coloptions=0x00007fa213889370, reloptions=0, isprimary='\0', isconstraint='\0', deferrable='\0', initdeferred='\0', allow_system_table_mods='\0', skip_build='\0', concurrent='\0', is_internal='\0') + 3591 at index.c:1082
frame #13: 0x000000010e5da885 postgres`DefineIndex(stmt=0x00007fa213888b90, indexRelationId=0, is_alter_table='\0', check_rights='\x01', skip_build='\0', quiet='\0') + 4181 at indexcmds.c:595
frame #14: 0x000000010e7dd4a3 postgres`ProcessUtilitySlow(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 2931 at utility.c:1163
frame #15: 0x000000010e7dc4d7 postgres`standard_ProcessUtility(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 3511 at utility.c:873
frame #16: 0x000000010e7db719 postgres`ProcessUtility(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 185 at utility.c:352
frame #17: 0x000000010e7db0e5 postgres`PortalRunUtility(portal=0x00007fa213889638, utilityStmt=0x00007fa21384b530, isTopLevel='\x01', dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 325 at pquery.c:1187
frame #18: 0x000000010e7da002 postgres`PortalRunMulti(portal=0x00007fa213889638, isTopLevel='\x01', dest=0x00007fa21384b918, altdest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 514 at pquery.c:1318
frame #19: 0x000000010e7d95c4 postgres`PortalRun(portal=0x00007fa213889638, count=9223372036854775807, isTopLevel='\x01', dest=0x00007fa21384b918, altdest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 964 at pquery.c:816
frame #20: 0x000000010e7d4d77 postgres`exec_simple_query(query_string=0x00007fa21384a838) + 1207 at postgres.c:1048
frame #21: 0x000000010e7d3fc1 postgres`PostgresMain(argc=1, argv=0x00007fa21301a3c0, dbname=0x00007fa21301a228, username=0x00007fa21301a208) + 2753 at postgres.c:3992
frame #22: 0x000000010e75868c postgres`BackendRun(port=0x00007fa212e00240) + 700 at postmaster.c:4085
frame #23: 0x000000010e757c81 postgres`BackendStartup(port=0x00007fa212e00240) + 433 at postmaster.c:3774
frame #24: 0x000000010e7544fe postgres`ServerLoop + 606 at postmaster.c:1585
frame #25: 0x000000010e751d74 postgres`PostmasterMain(argc=3, argv=0x00007fa212c041f0) + 5380 at postmaster.c:1240
frame #26: 0x000000010e6930fd postgres`main(argc=3, argv=0x00007fa212c041f0) + 653 at main.c:196
* thread #1: tid = 0x15221, 0x00007fff8c59f866 libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread, stop reason = signal SIGABRT
frame #0: 0x00007fff8c59f866 libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x00007fff8e60735c libsystem_pthread.dylib`pthread_kill + 92
frame #2: 0x00007fff91586bba libsystem_c.dylib`abort + 125
frame #3: 0x000000010e953ed9 postgres`ExceptionalCondition(conditionName=0x000000010ea31055, errorType=0x000000010e9b5973, fileName=0x000000010ea2fd3d, lineNumber=1175) + 137 at assert.c:54
frame #4: 0x000000010e79073a postgres`UnpinBuffer(buf=0x000000010f37f9c0, fixOwner='\0') + 282 at bufmgr.c:1175
frame #5: 0x000000010e793465 postgres`ReleaseBuffer(buffer=3169) + 565 at bufmgr.c:2540
frame #6: 0x000000010e414e43 postgres`freeGinBtreeStack(stack=0x00007fa2138adcf8) + 67 at ginbtree.c:196
frame #7: 0x000000010e415330 postgres`ginInsertValue(btree=0x00007fff51807e80, stack=0x00007fa2138a6dd8, insertdata=0x00007fff51807e70, buildStats=0x00007fff51809fa0) + 1216 at ginbtree.c:728
frame #8: 0x000000010e404ebf postgres`ginEntryInsert(ginstate=0x00007fff51807fe0, attnum=1, key=7828073, category='\0', items=0x0000000117d0ab28, nitem=76, buildStats=0x00007fff51809fa0) + 1055 at gininsert.c:218
frame #9: 0x000000010e405ad6 postgres`ginbuild(fcinfo=0x00007fff5180a050) + 1590 at gininsert.c:392
frame #10: 0x000000010e9609ba postgres`OidFunctionCall3Coll(functionId=2738, collation=0, arg1=4693605424, arg2=4693760992, arg3=140334089145912) + 186 at fmgr.c:1649
frame #11: 0x000000010e4f4b30 postgres`index_build(heapRelation=0x0000000117c2bc30, indexRelation=0x0000000117c51be0, indexInfo=0x00007fa213888e38, isprimary='\0', isreindex='\0') + 464 at index.c:1963
frame #12: 0x000000010e4f2f07 postgres`index_create(heapRelation=0x0000000117c2bc30, indexRelationName=0x00007fa213888b30, indexRelationId=16445, relFileNode=0, indexInfo=0x00007fa213888e38, indexColNames=0x00007fa2138892d8, accessMethodObjectId=2742, tableSpaceId=0, collationObjectId=0x00007fa213889330, classObjectId=0x00007fa213889350, coloptions=0x00007fa213889370, reloptions=0, isprimary='\0', isconstraint='\0', deferrable='\0', initdeferred='\0', allow_system_table_mods='\0', skip_build='\0', concurrent='\0', is_internal='\0') + 3591 at index.c:1082
frame #13: 0x000000010e5da885 postgres`DefineIndex(stmt=0x00007fa213888b90, indexRelationId=0, is_alter_table='\0', check_rights='\x01', skip_build='\0', quiet='\0') + 4181 at indexcmds.c:595
frame #14: 0x000000010e7dd4a3 postgres`ProcessUtilitySlow(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 2931 at utility.c:1163
frame #15: 0x000000010e7dc4d7 postgres`standard_ProcessUtility(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 3511 at utility.c:873
frame #16: 0x000000010e7db719 postgres`ProcessUtility(parsetree=0x00007fa21384b530, queryString=0x00007fa21384a838, context=PROCESS_UTILITY_TOPLEVEL, params=0x0000000000000000, dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 185 at utility.c:352
frame #17: 0x000000010e7db0e5 postgres`PortalRunUtility(portal=0x00007fa213889638, utilityStmt=0x00007fa21384b530, isTopLevel='\x01', dest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 325 at pquery.c:1187
frame #18: 0x000000010e7da002 postgres`PortalRunMulti(portal=0x00007fa213889638, isTopLevel='\x01', dest=0x00007fa21384b918, altdest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 514 at pquery.c:1318
frame #19: 0x000000010e7d95c4 postgres`PortalRun(portal=0x00007fa213889638, count=9223372036854775807, isTopLevel='\x01', dest=0x00007fa21384b918, altdest=0x00007fa21384b918, completionTag=0x00007fff5180b020) + 964 at pquery.c:816
frame #20: 0x000000010e7d4d77 postgres`exec_simple_query(query_string=0x00007fa21384a838) + 1207 at postgres.c:1048
frame #21: 0x000000010e7d3fc1 postgres`PostgresMain(argc=1, argv=0x00007fa21301a3c0, dbname=0x00007fa21301a228, username=0x00007fa21301a208) + 2753 at postgres.c:3992
frame #22: 0x000000010e75868c postgres`BackendRun(port=0x00007fa212e00240) + 700 at postmaster.c:4085
frame #23: 0x000000010e757c81 postgres`BackendStartup(port=0x00007fa212e00240) + 433 at postmaster.c:3774
frame #24: 0x000000010e7544fe postgres`ServerLoop + 606 at postmaster.c:1585
frame #25: 0x000000010e751d74 postgres`PostmasterMain(argc=3, argv=0x00007fa212c041f0) + 5380 at postmaster.c:1240
frame #26: 0x000000010e6930fd postgres`main(argc=3, argv=0x00007fa212c041f0) + 653 at main.c:196
Test has been done on OSX 10.9.
This is all I have for now... I will have a look at the code later, as this patch changes quite a bit the internals of gin, it is going to take a little bit of time.
Thanks,
--
Michael
On 19.11.2013 14:48, Michael Paquier wrote: > Here is a review of the first three patches: > 1) Further gin refactoring: > make check passes (core tests and contrib tests). > Code compiles without warnings. Committed. > Then... About the patch... Even if I got little experience with code of > gin, moving the flag for search mode out of btree, as well as removing the > logic of PostingTreeScan really makes the code lighter and easier to follow. > Just wondering, why not simplifying as well ginTraverseLock:ginbtree.c at > the same time to something similar to that? > if (!GinPageIsLeaf(page) || searchMode == TRUE) > return access; > > /* we should relock our page */ > LockBuffer(buffer, GIN_UNLOCK); > LockBuffer(buffer, GIN_EXCLUSIVE); > > /* But root can become non-leaf during relock */ > if (!GinPageIsLeaf(page)) > { > /* restore old lock type (very rare) */ > LockBuffer(buffer, GIN_UNLOCK); > LockBuffer(buffer, GIN_SHARE); > } > else > access = GIN_EXCLUSIVE; > return access; > Feel free to discard as I can imagine that changing such code would make > back-branch maintenance more difficult and it would increase conflicts with > patches currently in development. Yeah, might be more readable to write it that way. There's a lot of cleanup that could be done to the gin code, these patches are by no means the end of it. > 2) Refactoring of internal gin btree (needs patch 1 applied first): > make check passes (core tests and contrib tests). > Code compiles without warnings. Committed. > Yep, removing ginPageGetLinkItup makes sense. Just to be picky, I would > have put the arguments of GinFormInteriorTuple replacing ginPageGetLinkItup > in 3 separate lines just for lisibility. Ok, did that. > In dataPrepareDownlink:gindatapage.c, depending on if lpage is a leaf page > or not, isn't it inconsistent with the older code not to use > GinDataPageGetItemPointer and GinDataPageGetPostingItem to set > btree->pitem.key. Hmm. The old code in dataSplitPage() didn't use GinDataPageGetItemPointer/PostingItem either. The corresponding code in ginContinueSplit did, though. There was actually an inconsistency there: the ginContinueSplit function took the downlink's key from the last item on the page (using maxoff), while dataSplitPage took it from the right bound using GinDataPageGetRightBound(). Both are the same, dataSplitPage copies the value from the last item to the right bound, so it doesn't make a difference. They would diverge if the last item on the page is deleted, though, so the old coding in ginContinueSplit was actually a bit suspicious. > In ginContinueSplit:ginxlog.c, could it be possible to remove this code? It > looks that its deletion has been forgotten: > /* > > * elog(NOTICE,"ginContinueSplit root:%u l:%u r:%u", split->rootBlkno, > * split->leftBlkno, split->rightBlkno); > */ Yeah, that's just leftover debug code. But again, I'll leave that for another patch (in fact, the whole function will go away with the fourth patch, anyway). > 3) More refactoring (needs patches 1 and 2): > make check passes (core tests and contrib tests). > Code compiles without warnings. > Perhaps this patch would have been easier to read with context diffs :) It > just moves code around so nothing to say. Committed. Thanks for the review! I'll let you finish the review of the fourth patch. Meanwhile, I'll take another look at Alexander's gin packed posting items patch, and see how badly these commits bitrotted it. - Heikki
On 20.11.2013 15:27, Michael Paquier wrote: > Here are some comments about the 4th patch. > 1) Compiles without warnings, passes make check. > 2) s/ginFinshSplit/ginFinishSplit > 3) Server crashes when trying to create a gin index index creation (see > example of previous email with pg_trgm). Thanks, fixed! Here's a new version. To ease the review, I split the remaining patch again into two, where the first patch is just yet more refactoring. I also fixed some bugs in WAL logging and replay that I bumped into while testing. - Heikki
Attachment
On Thu, Nov 21, 2013 at 9:44 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > Here's a new version. To ease the review, I split the remaining patch again > into two, where the first patch is just yet more refactoring. > > I also fixed some bugs in WAL logging and replay that I bumped into while > testing. Cool. Here is the review of the two remaining patches. 1) More refactoring, general remarks: - Code compiles without warnings - Passes make check - If I got it correctly... this patch separates the part managing data to be inserted from ginbtree. I can understand the meaning behind that if we consider that GinBtree is used only to define methods and search conditions (flags and keys). However I am wondering if this does not make the code more complicated... Particularly the flag isDelete that is only passed to ginxlogInsert meritates its comment IMO. Note that I haven't read the 2nd patch when writing that :) - With this patch, previous SELECT example takes 5.236 ms in average, runtime does not change. 1-1) This block of code is repeated several times and should be refactored into a single function: /* During index build, count the new page */ if (buildStats) { if (btree->isData) buildStats->nDataPages++; else buildStats->nEntryPages++; } Something with a function like that perhaps: static void ginCountNewPage(GinBtree btree, GinStatsData *buildStats); 1-2) Could it be possible to change the variable name of "GinBtreeEntryInsertData *entry" in entryIsEnoughSpace? entry->entry is kind of hard to apprehend... Renaming it to either insertEntry. Another idea would be to rename entry in GinBtreeEntryInsertData to entryData or entryTuple. 1-3) This block of code is present two times: + if (!isleaf) + { + PostingItem *pitem = GinDataPageGetPostingItem(lpage, off); + PostingItemSetBlockNumber(pitem, updateblkno); + } Should the update of a downlink to point to the next page be a separate function? 2) post recovery cleanup: - OK, so roughly the soul of this patch is to change the update mechanism for a left child gin page so as the parent split is always done first before any new data is inserted in this child. And this ensures that we can remove the xlog cleanup mechanism for gin page splits in the same fashion as gist... xlog redo mechanism is then adapted according to that. - Compilation fails becausze the flags GIN_SPLIT_ISLEAF and GIN_SPLIT_ISDATA are missing in gin_private.h. The patch attached fixes that though. - With my additional patch, it passes make check, compilation shows no warnings. - I did some tests with the patch: -- Index creation time vanilla: 3266.834 with the two patches: 3412.473 ms -- Tried the new redo mechanism by simulating some server failures a couple of times and saw no failures - I am seeing similar run times for queries like the example used in previous emails of this thread. So no problem on this side. - And... Here are some comments about the code: 2-1) In ginFindParents, is the case where the stack has no parent possible (aka the stack is the root itself)? Shouldn't this code path check if root is NULL or not? 2-2) Not sure that this structure is in-line with the project policy: struct { BlockNumber left; BlockNumber right; } children; Why not adding a complementary structure in gin_private.h doing that? It could be used as well in ginxlogSplit to specify a left/right family of block numbers. 2-3) s/kepy/kept (comments of ginFinishSplit) Other than that, the patch looks great. I particularly like the new redo logic in ginxlog.c, the code is more easily understandable with the split redo removed. Even if I am just a noob for this code, it is nicely built and structured. Regards, -- Michael
Attachment
On 11/22/13 15:04, Michael Paquier wrote: > On Thu, Nov 21, 2013 at 9:44 PM, Heikki Linnakangas > <hlinnakangas@vmware.com> wrote: >> Here's a new version. To ease the review, I split the remaining patch again >> into two, where the first patch is just yet more refactoring. >> >> I also fixed some bugs in WAL logging and replay that I bumped into while >> testing. > Cool. Here is the review of the two remaining patches. > 1) More refactoring, general remarks: > - Code compiles without warnings > - Passes make check > - If I got it correctly... this patch separates the part managing data > to be inserted from ginbtree. I can understand the meaning behind that > if we consider that GinBtree is used only to define methods and search > conditions (flags and keys). However I am wondering if this does not > make the code more complicated... Well, I think it's an improvement in readability to separate the insertion payload from the search information. With the second patch it becomes more important, because if an incompletely split page is encountered while you're inserting a value, you needs to insert the downlink for the old incomplete split first, and then continue the insertion of the original vaule where you left. That "context switching" becomes a lot easier when value you're inserting is passed around separately. > Particularly the flag isDelete that > is only passed to ginxlogInsert meritates its comment IMO. Note that I > haven't read the 2nd patch when writing that :) Yeah, that gets removed in the second patch, which changes the WAL record format. :-) > 1-2) Could it be possible to change the variable name of > "GinBtreeEntryInsertData *entry" in entryIsEnoughSpace? entry->entry > is kind of hard to apprehend... Renaming it to either insertEntry. > Another idea would be to rename entry in GinBtreeEntryInsertData to > entryData or entryTuple. ok, renamed the variable to insertData. > 1-3) This block of code is present two times: > + if (!isleaf) > + { > + PostingItem *pitem = GinDataPageGetPostingItem(lpage, off); > + PostingItemSetBlockNumber(pitem, updateblkno); > + } > Should the update of a downlink to point to the next page be a > separate function? Doesn't seem any better to me. Thanks, committed this refactoring patch now. Will continue on the main patch. - Heikki
On Wed, Nov 13, 2013 at 8:49 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
Here's another part of my crusade against xlog cleanup routines. This series of patches gets rid of the gin_cleanup() function, which is currently used to finish splits of GIN b-tree pages, if the system crashes (or an error occurs) between splitting a page and inserting its downlink to the parent.
The first three patches just move code around. IMHO they make the code more readable, so they should be committed in any case. The meat is in the fourth patch.
Thoughts, objections?
Alexander, I'm sorry if this conflicts with your GIN patches. Feel free to post the latest versions of your patches against the current master, ignoring patches. I can fix the bitrot. That said, I think these refactorings will make your code look a little bit nicer too, so you might want to rebase because of that anyway.
Hi Heikki,
The commit 04eee1fa9ee80dabf7 of this series causes a self-deadlock in the LWLock code during the operation below, with it trying to take an LW_EXCLUSIVE on a high, even-numbered lockid when it already holds the same lockid.
CREATE INDEX planet_osm_ways_nodes ON planet_osm_ways USING gin (nodes) WITH (FASTUPDATE=OFF);
It happens pretty reliably using osm2pgsql.
I will try to come up with a simple reproducible demonstration, and stack trace, over the weekend.
Cheers,
Jeff
On 11/22/13 15:04, Michael Paquier wrote: > 2) post recovery cleanup: > - OK, so roughly the soul of this patch is to change the update > mechanism for a left child gin page so as the parent split is always > done first before any new data is inserted in this child. And this > ensures that we can remove the xlog cleanup mechanism for gin page > splits in the same fashion as gist... xlog redo mechanism is then > adapted according to that. > - I did some tests with the patch: > -- Index creation time > vanilla: 3266.834 > with the two patches: 3412.473 ms Hmm. I didn't expect any change in index creation time. Is that repeatable, or within the margin of error? > 2-1) In ginFindParents, is the case where the stack has no parent > possible (aka the stack is the root itself)? Shouldn't this code path > check if root is NULL or not? No. A root split doesn't need to find the parent (because there isn't any), so we never call ginFindParents with a stack containing just the root. If you look at the only caller of ginFindParents, it's quite clear that stack->parent always exists. > 2-2) Not sure that this structure is in-line with the project policy: > struct > { > BlockNumber left; > BlockNumber right; > } children; > Why not adding a complementary structure in gin_private.h doing that? > It could be used as well in ginxlogSplit to specify a left/right > family of block numbers. I don't think we have a project policy against in-line structs. Might be better style to not do it, anyway, though. Come to think of it, that children variable mustn't be declared inside the if-statement; it's no longer in scope when the XLogInsert was done, so the &children was pointing to potentially uninitialized piece of stack. Valgrind would've complained. I ended up using BlockIdData[2] for that. Committed, thanks for the review! - Heikki
On Wed, Nov 27, 2013 at 9:40 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
On Wed, Nov 13, 2013 at 8:49 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:Here's another part of my crusade against xlog cleanup routines. This series of patches gets rid of the gin_cleanup() function, which is currently used to finish splits of GIN b-tree pages, if the system crashes (or an error occurs) between splitting a page and inserting its downlink to the parent.
The first three patches just move code around. IMHO they make the code more readable, so they should be committed in any case. The meat is in the fourth patch.
Thoughts, objections?
Alexander, I'm sorry if this conflicts with your GIN patches. Feel free to post the latest versions of your patches against the current master, ignoring patches. I can fix the bitrot. That said, I think these refactorings will make your code look a little bit nicer too, so you might want to rebase because of that anyway.Hi Heikki,The commit 04eee1fa9ee80dabf7 of this series causes a self-deadlock in the LWLock code during the operation below, with it trying to take an LW_EXCLUSIVE on a high, even-numbered lockid when it already holds the same lockid.CREATE INDEX planet_osm_ways_nodes ON planet_osm_ways USING gin (nodes) WITH (FASTUPDATE=OFF);It happens pretty reliably using osm2pgsql.I will try to come up with a simple reproducible demonstration, and stack trace, over the weekend.
Whatever the problem, it seems to have been fixed in ce5326eed386959aa, "More GIN refactoring".
Cheers,
Jeff
On 12/01/2013 10:40 PM, Jeff Janes wrote: > On Wed, Nov 27, 2013 at 9:40 AM, Jeff Janes <jeff.janes@gmail.com> wrote: > >> The commit 04eee1fa9ee80dabf7 of this series causes a self-deadlock in the >> LWLock code during the operation below, with it trying to take >> an LW_EXCLUSIVE on a high, even-numbered lockid when it already holds the >> same lockid. >> >> CREATE INDEX planet_osm_ways_nodes ON planet_osm_ways USING gin (nodes) >> WITH (FASTUPDATE=OFF); >> >> It happens pretty reliably using osm2pgsql. >> >> I will try to come up with a simple reproducible demonstration, and stack >> trace, over the weekend. > > Whatever the problem, it seems to have been fixed in ce5326eed386959aa, > "More GIN refactoring". That's good, I guess :-). Thanks for the testing. Did you import the full planet.osm? I tried with a subset containing just Finland, but didn't see any problems. - Heikki
On Mon, Dec 2, 2013 at 1:26 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote:
On 12/01/2013 10:40 PM, Jeff Janes wrote:The commit 04eee1fa9ee80dabf7 of this series causes a self-deadlock in the
LWLock code during the operation below, with it trying to take
an LW_EXCLUSIVE on a high, even-numbered lockid when it already holds the
same lockid.
CREATE INDEX planet_osm_ways_nodes ON planet_osm_ways USING gin (nodes)
WITH (FASTUPDATE=OFF);
It happens pretty reliably using osm2pgsql.
I will try to come up with a simple reproducible demonstration, and stack
trace, over the weekend.
Whatever the problem, it seems to have been fixed in ce5326eed386959aa,
"More GIN refactoring".
That's good, I guess :-). Thanks for the testing. Did you import the full planet.osm? I tried with a subset containing just Finland, but didn't see any problems.
I used Antarctica. I don't have the RAM to process the full planet, or the bandwidth to download it very easily.
Do you think it is worth chasing down where the problem was, to make sure it was truly fixed rather than simply changed in a way that happens not to trigger any more in this situation?
Cheers,
Jeff