Re: post-recovery amcheck expectations

On Mon, Oct 23, 2023 at 04:46:23PM -0700, Peter Geoghegan wrote:
> On Fri, Oct 20, 2023 at 8:55 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > > I lean toward fixing this by
> > > > having amcheck scan left; if left links reach only half-dead or deleted pages,
> > > > that's as good as the present child block being P_LEFTMOST.
> > >
> > > Also my preference.
> >
> > Done mostly that way, except I didn't accept deleted pages. Making this work
> > on !readonly would take more than that, and readonly shouldn't need that.
>
> That makes sense to me. I believe that it's not possible to have a
> string of consecutive sibling pages that are all half-dead (regardless
> of the BlockNumber order of sibling pages, even). But I'd probably
> have written the fix in roughly the same way. Although...maybe you
> should try to detect a string of half-dead pages? Hard to say if it's
> worth the trouble.

I imagined a string of half-dead siblings could arise in structure like this:

* 1
* / | \
* 4 <-> 2 <-> 3

With events like this:

- DELETE renders blk 4 deletable.
- Crash with concurrent VACUUM, leaving 4 half-dead after having visited 1-4.
- DELETE renders blk 2 deletable.
- Crash with concurrent VACUUM, leaving 2 half-dead after having visited 1-2.

I didn't try to reproduce that, and something may well prevent it.

> Suggest adding a CHECK_FOR_INTERRUPTS() call to the loop, too, just
> for good luck.

Added. That gave me the idea to check for circular links, like other parts of
amcheck do. Net diff:

--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -949,11 +949,16 @@ bt_leftmost_ignoring_half_dead(BtreeCheckState *state,
Page page = palloc_btree_page(state, reached);
BTPageOpaque reached_opaque = BTPageGetOpaque(page);

+ CHECK_FOR_INTERRUPTS();
+
/*
- * _bt_unlink_halfdead_page() writes that side-links will continue to
- * point to the siblings. We can easily check btpo_next.
+ * Try to detect btpo_prev circular links. _bt_unlink_halfdead_page()
+ * writes that side-links will continue to point to the siblings.
+ * Check btpo_next for that property.
*/
- all_half_dead = P_ISHALFDEAD(reached_opaque) &&
+ all_half_dead = P_ISHALFDEAD(reached_opaque);
+ reached != start &&
+ reached != reached_from &&
reached_opaque->btpo_next == reached_from;
if (all_half_dead)
{