On 04/10/2018 03:30 PM, Peter Geoghegan wrote: > On Tue, Apr 10, 2018 at 12:07 PM, Pavan Deolasee > <pavan.deola...@gmail.com> wrote: >> Hmm. I am a bit confused why we want to mention anything about something >> we're not even considering seriously, let alone any patch or work in that >> direction. If we at all change everything to use extent based storage, there >> would be many other things that will break and require changes, no? > I guess you're right. It's implied by what you already say in the > nbtree README. No need to do more. > >> Apart from that, other changes requested are included in the patch. This >> also takes care of Heikki's observation regarding UNLOGGED tables on the >> other thread. > Cool. > > Feedback on this version: > >> +without a backend's cached page also being detected as invalidated, but >> +only when we happen to recycle a page that once again becomes the >> +rightmost leaf page. > I suggest wording this as "...but only when we happen to recycle a > block that once again gets recycled as the rightmost leaf page". (I'm > correcting my own wording here, I think.) > > * No need to start a sentence with "So" here, IMV: > >> + * Note the fact that whenever we fail to take the fastpath, we clear >> + * the cached block. So checking for a valid cached block at this >> point >> + * is enough to decide whether we're in a fastpath or not. > * This should test "if (P_RIGHTMOST(lpageop) && P_ISLEAF(lpageop) && > !P_ISROOT(lpageop))", because you don't want to use the optimization > for internal pages that happen to not be the root -- there is no leaf > check here: > >> + /* >> + * Cache the block information if we just inserted into the rightmost >> + * leaf page of the index and it's not the root page. For very small >> + * index where root is also the leaf, there is no point trying for >> any >> + * optimisation. >> + */ >> + if (P_RIGHTMOST(lpageop) && !P_ISROOT(lpageop)) >> + cachedBlock = BufferGetBlockNumber(buf); > * I don't think that you should have a #define inline with code like this: > >> +#define BTREE_FASTPATH_MIN_LEVEL 2 >> + if (BlockNumberIsValid(cachedBlock) && >> + _bt_getrootheight(rel) >= BTREE_FASTPATH_MIN_LEVEL) >> + RelationSetTargetBlock(rel, cachedBlock); > I suggest putting this in nbtree.h instead. You can put it just after > BTREE_NONLEAF_FILLFACTOR, with a comment, in a separate block/section. > > Other than that, looks good to me. >
Committed with light editing. I didn't put the #define in the .h file - it's only used here and that seemed like unnecessary clutter. I moved it to the top of the file. I also standardized the spelling of "optimization". cheers andrew
