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