Re: Handling GIN incomplete splits

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