Re: Differential code coverage between 16 and HEAD

On Sun, Apr 14, 2024 at 6:33 PM Andres Freund <andres@anarazel.de> wrote:
> - Some of the new nbtree code could use a bit more tests:
>   https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/nbtree/nbtutils.c.gcov.html#L1468

I made a conscious decision to not add coverage for the function that
you've highlighted here (_bt_rewind_nonrequired_arrays) back when I
reviewed the coverage situation for the patch, which was about a month
ago now. (FWIW I also decided against adding coverage for the
recursive call to _bt_advance_array_keys, for similar reasons.)

I don't mind adding _bt_rewind_nonrequired_arrays coverage now. I
already wrote 4 tests that show wrong answers (and assertion failures)
when the call to _bt_rewind_nonrequired_arrays is temporarily
commented out. The problem with committing such a test, if any, is
that it'll necessitate creating an index with at least 3 columns,
crafted to trip up this exact issue with non-required arrays -- and it
has to be bigger than one page (probably several pages, at a minimum).
That's a relatively large number of cycles to spend on this fairly
narrow issue -- it's at least a lot relative to the prevailing
standard for these things. Plus I'd be relying on implementation
details that might change, as well as relying on things like BLCKSZ
(not that it'd be the first time that I committed a test like that).

Note also that there is a general rule (explained above
_bt_rewind_nonrequired_arrays) requiring that all non-required arrays
be reset to their initial positions/element (first in the current scan
direction) once _bt_first is reached. If _bt_advance_array_keys
somehow failed to follow that rule (not necessarily due to an issue
with missing the call to _bt_rewind_nonrequired_arrays), then we'd get
an assertion failure within _bt_first -- the
_bt_verify_arrays_bt_first assertion would catch the violation of the
invariant (the_bt_advance_array_keys postcondition invariant/_bt_first
precondition invariant). So we kinda do have some test coverage for
this function already.

--
Peter Geoghegan