On Thu, Mar 29, 2018 at 4:39 AM, Peter Geoghegan <pg@bowt.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.
