From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Peter Geoghegan <pg(at)bowt(dot)ie>, Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com> |
Cc: | pgsql-committers <pgsql-committers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: pgsql: Optimize btree insertions for common case of increasing values |
Date: | 2018-04-10 22:27:10 |
Message-ID: | d505a996-8c8e-4f81-9125-85937e00d2f2@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers |
On 04/10/2018 03:30 PM, Peter Geoghegan wrote:
> On Tue, Apr 10, 2018 at 12:07 PM, Pavan Deolasee
> <pavan(dot)deolasee(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2018-04-10 22:33:20 | Re: pgsql: Optimize btree insertions for common case of increasing values |
Previous Message | Andrew Dunstan | 2018-04-10 22:21:38 | pgsql: Adjustments to the btree fastpath optimization. |