Re: Fixes for two separate bugs in nbtree VACUUM's page deletion

On Tue, Apr 28, 2020 at 12:21 AM Masahiko Sawada
<masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> For the first fix it seems better to push down the logic to the page
> deletion code as your 0001 patch does so. The following change changes
> the page deletion code so that it emits LOG message indicating the
> index corruption when a deleted page is passed. But we used to ignore
> in this case.

Attached is v2 of the patch set, which includes a new patch that I
want to target the master branch with. This patch (v2-0003-*)
refactors btvacuumscan().

This revision also changed the first bugfix patch. I have simplified
some of the details in nbtree.c that were added by commit 857f9c36cda.
Can't we call _bt_update_meta_cleanup_info() at a lower level, like in
the patch? AFAICT it makes more sense to just call it in
btvacuumscan(). Please let me know what you think of those changes.

The big change in the new master-only refactoring patch (v2-0003-*) is
that I now treat a call to btvacuumpage() that has to
"backtrack/recurse" and doesn't find a page that looks like the
expected right half of a page split (a page split that occurred since
the VACUUM operation began) as indicating corruption. This corruption
is logged. I believe that we should never see this happen, for reasons
that are similar to the reasons why _bt_pagedel() never finds a
deleted page when moving right, as I went into in my recent e-mail to
this thread. I think that this is worth tightening up for Postgres 13.

I will hold off on committing v2-0003-*, since I need to nail down the
reasoning for treating this condition as corruption. Plus it's not
urgent. I think that the general direction taken in v2-0003-* is the
right one, in any case. The "recursion" in btvacuumpage() doesn't make
much sense -- it obscures what's really going on IMV. Also, using the
variable name "scanblkno" at every level -- btvacuumscan(),
btvacuumpage(), and _bt_pagedel() -- makes it clear that it's the same
block at all levels of the code. And that the "backtrack/recursion"
case doesn't kill tuples from the "scanblkno" block (and doesn't
attempt to call _bt_pagedel(), which is designed only to handle blocks
passed by btvacuumpage() when they are the current "scanblkno"). It
seems unlikely that the VACUUM VERBOSE pages deleted accounting bug
would have happened if the code was already structured this way.

--
Peter Geoghegan