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

On Thu, Mar 29, 2018 at 4:39 AM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:

>
>
> Suggested next steps to deal with this:
>
> * A minor point: I don't think you should call
> RelationSetTargetBlock() when the page P_ISROOT(), which, as I
> mentioned, is a condition that can coexist with P_ISLEAF() with very
> small B-Trees. There can be no point in doing so. No need to add
> P_ISROOT() to the main "Is cached page stale?" test within
> _bt_doinsert(), though.
>

Ok. Adding a check for tree height and setting target block only if it's >=
2, as suggested by you and Simon later. Rahila helped me also ran another
round of tests and this does not lead to any performance regression (we
were worried about whether calling _bt_getrootheight will be expensive).

Also moved RelationSetTargetBlock() to a point where we are not holding any
locks and are outside the critical section.

>
> * An assertion would make me feel a lot better about depending on not
> having a page split from a significant distance.
>

Ok. I assume you mean an assertion to check that the target page doesn't
have an in-complete split. Added that though not sure if it's useful since
we concluded that right-most page can't have in-complete split.

Let me know if you mean something else.

> Your optimization should continue to not be used when it would result
> in a page split, but only because that would be slow. The comments
> should say so, IMV.

Added.

> Also, _bt_insertonpg() should have an assertion
> against a page split actually occurring when the optimization was
> used, just in case. When !BufferIsValid(cbuf), we know that we're
> being called from _bt_doinsert() (see existing assertion at the top of
> _bt_insertonpg() as an example of this), so at the point where it's
> clear a page split is needed, we should assert that there is no target
> block that we must have been passed as the target page.
>
>
You mean passing "fastpath" to _bt_insertonpg and then checking it's false
if page split is needed? But isn't page split is only needed if the page
doesn't have enough free space? If so, we have checked for that before
setting "fastpath".

> * The indentation of the main _bt_doinsert() test does not follow
> project guidelines. Please tweak that, too.
>

Ok. Fixed.

Thanks,
Pavan

--
Pavan Deolasee http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services