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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
