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
