Re: Handling GIN incomplete splits

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