Re: nbtree backwards scan test coverage

On Mon, Jul 20, 2026 at 8:14 AM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> Some nits:
>
> 1. The "returns a half-dead page" case
>
> Maybe this is out of scope of this patch.
>
> Returning a half-dead page is the ordinary return, not one
> of the recovery paths this patch is about. The actual half-dead
> handling then lives in the caller's P_IGNORE branch.

coverage.postgresql.org shows that that P_IGNORE branch only has one
hit, which is for a forwards scan. It's quite unlikely that that's a
half-dead page; a fully deleted page is much more typical. Whereas
with a backwards scan, it has to be a half-dead page
(_bt_lock_and_validate_left won't ever return a fully deleted page).

BTW, coverage.postgresql.org now accurately represents the coverage
situation. My commit from last night (commit
b087a88c40e45453682bf6d24c42eff33f401662) makes the nbtree module
actually run on the machine that these reports are built on. That
explains why incomplete splits seemed to have no coverage prior prior
to last night.

> There is an existing nbtree_half_dead_pages regress test, which already
> tests a half-dead page, but only forwards:
>
> > select * from nbtree_half_dead_pages where id > 99998 and id < 120002;
>
> A sibling query with ORDER BY id DESC (a backward Index Scan) would
> close the loop for the P_IGNORE-on-the-left branch without adding a
> permutation here. Purely optional, just mentioning.

I don't think that it would, unless there were injection points in
_bt_lock_and_validate_left that the test used.

> 2. Only two of the four points are lwlock-free; the other two fire
> under a buffer lock
>
> The test only ever 'wait's on lock-and-validate-left and uses 'notice'
> for the two in-lock points, so it is fine as written.
> Optionally consider a one-line comment near to INJECTION_POINT noting
> that it fires under buffer lock.

I don't think that it matters if an injection point fires under a buffer lock.

> 3. Fixed BLCKSZ
>
> I think that is fine, and there is recent precedent in GIN amcheck tests.
> When that went in, Arseniy noted that "some tests will fail with not
> standard block size", and Tomas's conclusion was [2]:
>
> > AFAIK we don't expect tests to be 100% stable with other block
> > sizes. It shouldn't crash / segfault, ofc, but some tests may be
> > sensitive to this.

Cool, that's a useful precedent to point to.

Even if you ran the tests with a non-standard BLCKSZ, you'd still have
to enable injection points to see test "failures".

> Personally, I'd like to enable test on various page sizes one day. But
> it is large project anyway. An isolation spec has no runtime block_size
> hook right now to skip the test.

The tests wouldn't test anything with a non-standard BLCKSZ. You'd
have to write an entirely different version of the test for each block
size, I think (at least if we expect to count the number of times
_bt_lock_and_validate_left moves right to recover from the
lastcurrblkno being deleted).

> There's another similar test that may be useful. When we converted B-tree
> vacuum to the read stream API, Melanie built essentially the same style of
> test for the concurrent page splits during a physical-order nbtree scan
> forcing backtracking [0,1]. At that time we decided to postpone the test.

That's not a bad idea. You'd really need more injection points to make
such a test reliable, which we now have.

--
Peter Geoghegan