Re: Bug in amcheck?

On 25/11/2025 22:51, Peter Geoghegan wrote:
> On Tue, Nov 25, 2025 at 3:03 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> To fix this, I guess we need to teach bt_index_parent_check() about
>> half-dead pages. Anyone volunteer to write that patch?
> 
> It's not like bt_index_parent_check doesn't generally know about them.
> For example, bt_downlink_missing_check goes to great lengths to
> distinguish between legitimate "missing" downlinks caused by an
> interrupted page deletion, and real missing downlinks caused by
> corruption.
> 
> The problem we're seeing here seems likely limited to code added by
> commit d114cc53, which enhanced bt_index_parent_check by adding the
> new bt_child_highkey_check check. bt_child_highkey_check actually
> reuses bt_downlink_missing_check (which deals with half-dead pages
> correctly), but still isn't careful enough about half-dead pages. This
> is kind of surprising, since it *does* account for incomplete splits,
> which are similar.

+1. It took me a moment to understand bt_downlink_missing_check(). The 
comments there talk about interrupted page deletions and incomplete 
splits, but it's actually never called for half-dead pages. The caller 
checks !P_IGNORE(opaque), and P_IGNORE means BTP_DELETED | BTP_HALF_DEAD.

I don't think BTP_DELETED pages should be reachable here at all. So 
perhaps we should specifically check BTP_DELETED and give an ERROR on 
those. And for clarity, perhaps move the check for BTP_HALF_DEAD into 
bt_downlink_missing_check(), alongside the incomplete-split check, so 
that both cases would be checked at the same place. Just for clarity, 
it's not wrong as it is.

> In short, I think that we need to track something like
> BtreeCheckState.previncompletesplit, but for half-dead pages. And then
> actually use that within bt_child_highkey_check, to avoid spurious
> false-positive reports of corruption.

I think it's even simpler than that, and we can just do this:

--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -2268,7 +2268,7 @@ bt_child_highkey_check(BtreeCheckState *state,
  		 * If we visit page with high key, check that it is equal to the
  		 * target key next to corresponding downlink.
  		 */
-		if (!rightsplit && !P_RIGHTMOST(opaque))
+		if (!rightsplit && !P_RIGHTMOST(opaque) && !P_ISHALFDEAD(opaque))
  		{
  			BTPageOpaque topaque;
  			IndexTuple	highkey;


Both BTP_HALF_DEAD and BTP_INCOMPLETE_SPLIT indicate that a downlink is 
missing, but they work slightly differently. If a page is marked as 
BTP_INCOMPLETE_SPLIT, it means that its right sibling has no downlink, 
but if a page is marked as BTP_HALF_DEAD, it means that the page itself 
has no downlink.

- Heikki