Re: _bt_split(), and the risk of OOM before its critical section

2019-05-09 Thread Peter Geoghegan
On Wed, May 8, 2019 at 3:37 PM Peter Geoghegan wrote: > It makes perfect sense for _bt_split() to call _bt_findsplitloc() > directly, since _bt_findsplitloc() is already aware of almost every > _bt_split() implementation detail, whereas those same details are not > of interest anywhere else. I

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-08 Thread Peter Geoghegan
On Tue, May 7, 2019 at 6:15 PM Peter Geoghegan wrote: > I suppose I'm biased, but I prefer the new approach anyway. Adding the > left high key first, and then the right high key seems simpler and > more logical. It emphasizes the similarities and differences between > leftpage and rightpage. I

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-07 Thread Peter Geoghegan
On Mon, May 6, 2019 at 4:11 PM Peter Geoghegan wrote: > I am tempted to move the call to _bt_truncate() out of _bt_split() > entirely on HEAD, possibly relocating it to > nbtsplitloc.c/_bt_findsplitloc(). That way, there is a clearer > separation between how split points are chosen, suffix

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Peter Geoghegan
On Mon, May 6, 2019 at 5:15 PM Peter Geoghegan wrote: > VACUUM asserts P_FIRSTDATAKEY(opaque) > PageGetMaxOffsetNumber(page) > within _bt_mark_page_halfdead(), but doesn't test that condition in > release builds. This means that the earliest modifications of the > right page, before the high key

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Peter Geoghegan
On Mon, May 6, 2019 at 4:11 PM Peter Geoghegan wrote: > The important question is how VACUUM will recognize it. It's clearly > not as bad as something that causes "failed to re-find parent key" > errors, but I think that VACUUM might not be reclaiming it for the FSM > (haven't checked). Note that

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Peter Geoghegan
On Mon, May 6, 2019 at 3:29 PM Tom Lane wrote: > Yeah, as _bt_split is currently coded, _bt_truncate has to be a "no > errors" function, which it isn't. The pfree for its result is being > done in an ill-chosen place, too. I am tempted to move the call to _bt_truncate() out of _bt_split()

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Tom Lane
Peter Geoghegan writes: > Commit 8fa30f906be reduced the elevel of a number of "can't happen" > errors from PANIC to ERROR. These were all critical-section-adjacent > errors involved in nbtree page splits, and nbtree page deletion. It > also established the following convention within

Re: _bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Peter Geoghegan
On Mon, May 6, 2019 at 12:48 PM Peter Geoghegan wrote: > On the other other hand, it seems to me that the PageGetTempPage() > thing might have been okay, because it happens before the high key is > inserted on the new right buffer page. The same cannot be said for the > way we generate a new high

_bt_split(), and the risk of OOM before its critical section

2019-05-06 Thread Peter Geoghegan
Commit 8fa30f906be reduced the elevel of a number of "can't happen" errors from PANIC to ERROR. These were all critical-section-adjacent errors involved in nbtree page splits, and nbtree page deletion. It also established the following convention within _bt_split(), which allowed Tom to keep the