On Mon, May 6, 2019 at 12:48 PM Peter Geoghegan <pg@bowt.ie> 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 key for the left/old page via suffix
> truncation, which happens to occur after the right buffer page is
> first modified by inserted its high key (the original/left page's
> original high key). I think that there may be a risk that VACUUM's
> page deletion code will get confused by finding an errant right
> sibling page from a failed page split when there is a high key. If so,
> that would be a risk that was introduced in Postgres 11, and made much
> more likely in practice in Postgres 12. (I haven't got as far as doing
> an analysis of the risks to page deletion, though. The "fastpath"
> rightmost page insertion optimization that was also added to Postgres
> 11 seems like it also might need to be considered here.)
It seems like my fears about page deletion were well-founded, at least
if you assume that the risk of an OOM at the wrong time is greater
than negligible.
If I simulate an OOM error during suffix truncation, then
non-rightmost page splits leave the tree in a state that confuses
VACUUM/page deletion. When I simulate an OOM on page 42, we will later
see the dreaded "failed to re-find parent key in index "foo" for
deletion target page 42" error message from a VACUUM. That's not good.
It doesn't matter if the same things happens when splitting a
rightmost page, which naturally doesn't insert a new high key on the
new right half. This confirms my theory that the PageGetTempPage()
memory allocation can fail without confusing VACUUM, since that
allocation occurs before the critical-but-not-critical point (the
point that we really start to modify the new right half of the split).
Fortunately, this bug seems easy enough to fix: we can simply move the
"insert new high key on right page" code so that it comes after suffix
truncation. This makes it safe for suffix truncation to have an OOM,
or at least as safe as the PageGetTempPage() allocation that seems
safe to me.
--
Peter Geoghegan