Re: pgsql: Optimize btree insertions for common case of increasing values

From: Peter Geoghegan <pg(at)bowt(dot)ie>
To: Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)com>
Cc: Andrew Dunstan <andrew(at)dunslane(dot)net>, 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 19:30:27
Message-ID: CAH2-Wz=whBsrnmF=CJKBBG=K1euAfmWRg4gx_M=6R+w7Y=d5vw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

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.

Thanks
--
Peter Geoghegan

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Peter Geoghegan 2018-04-10 19:46:49 Re: pgsql: Optimize btree insertions for common case of increasing values
Previous Message Pavan Deolasee 2018-04-10 19:07:16 Re: pgsql: Optimize btree insertions for common case of increasing values