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. Thanks -- Peter Geoghegan