Обсуждение: Can can I make an injection point wait occur no more than once?

I'm working on adding test coverage to _bt_lock_and_validate_left,
which was enhanced by Postgres 18 commit 1bd4bc85ca. In particular,
coverage of its unhappy path: the path where multiple concurrent page
splits necessitate that the scan (which generally moves to the left)
moves to the right multiple times, until finally it gives up. When it
gives up it returns to the original lastcurrblkno to see what's up
with it -- it'll need to get that page's now-current left sibling
link, beginning the whole process anew (by looping back to the start
of _bt_lock_and_validate_left).

An isolation test that uses injection points seems like a natural
approach (actually, it's likely the *only* approach that can produce a
maintainable test). One session should perform a backwards scan that
is forced to wait at the top of _bt_lock_and_validate_left. Another
session then inserts enough index tuples to cause several leaf page
splits that'll make life harder for the backwards scan. Finally, we
wake the backwards scan session, and get the desired test coverage;
it'll reliably have to do things the hard way.

I have all this working already. However, there are certain aspects of
the isolation test (and the injection points themselves) that seem
unsatisfactory. I could really use a way to make the wait within
_bt_lock_and_validate_left happen no more than once, in a way that's
directly under the control of my isolation test.

Any test like this needs to account for various implementation
details. For example, if the test needs to work with non-standard
BLCKSZ (which seems like a good idea), then the number of page splits
required might be greater or fewer than with standard BLCKSZ. This
shouldn't really be a problem; it necessitates inserting more data
than is strictly necessary most of the time: there needs to be some
margin or error to account for these effects. But that shouldn't be
much of a problem.

However, as things stand, this does create a problem: accounting for
these implementation details in this manner makes the number of times
that the injection point is reached unpredictable/hard to control. I
only want the wait within _bt_lock_and_validate_left to happen once,
before the concurrent inserts take place from within the other
isolation test session. I don't want any possible future calls to
_bt_lock_and_validate_left (that come after the other session is done)
to wait at all -- that'll make the backwards scan test session wait
forever (since no other session will be around to wake it up a second
or a third time).

I have successfully simulated "wait no more than once" by adding C
code to nbtree that looks like this:

            if (likely(!P_ISDELETED(opaque) &&
                       opaque->btpo_next == lastcurrblkno))
            {
                /* Found desired page, return it */
#ifdef USE_INJECTION_POINTS
                if (IS_INJECTION_POINT_ATTACHED("lock-and-validate-left"))
                {
                    InjectionPointDetach("lock-and-validate-left");
                }
#endif

But that's pretty ugly and non-modular. There are multiple return
paths within _bt_lock_and_validate_left, and I'd probably need to
cover them all with similar code. That seems borderline unacceptable.

It would be far preferable if I could just use some built-in way of
waiting exactly once, that can be used directly from SQL, through the
injection_points extension. That would allow me to write the isolation
test without having to add any code to nbtsearch.c that knows all
about the requirements of one particular isolation test.

Thanks
-- 
Peter Geoghegan

On Mon, Jul 07, 2025 at 05:31:30PM -0400, Peter Geoghegan wrote:
> I have successfully simulated "wait no more than once" by adding C
> code to nbtree that looks like this:
> 
>             if (likely(!P_ISDELETED(opaque) &&
>                        opaque->btpo_next == lastcurrblkno))
>             {
>                 /* Found desired page, return it */
> #ifdef USE_INJECTION_POINTS
>                 if (IS_INJECTION_POINT_ATTACHED("lock-and-validate-left"))
>                 {
>                     InjectionPointDetach("lock-and-validate-left");
>                 }
> #endif
> 
> But that's pretty ugly and non-modular. There are multiple return
> paths within _bt_lock_and_validate_left, and I'd probably need to
> cover them all with similar code. That seems borderline unacceptable.
> 
> It would be far preferable if I could just use some built-in way of
> waiting exactly once, that can be used directly from SQL, through the
> injection_points extension. That would allow me to write the isolation
> test without having to add any code to nbtsearch.c that knows all
> about the requirements of one particular isolation test.

In your test, just detach the injection point while the backend under test is
waiting at the injection point.  All of
src/test/modules/injection_points/specs/*.spec use that technique.

On Mon, Jul 7, 2025 at 6:02 PM Noah Misch <noah@leadboat.com> wrote:
> In your test, just detach the injection point while the backend under test is
> waiting at the injection point.  All of
> src/test/modules/injection_points/specs/*.spec use that technique.

That appears to work (without the kludge I added to nbtsearch.c),
though I find that I need to detach the injection point *and* wake up
the waiting backend. In that order. Thanks!

For what it's worth, I found
src/test/modules/injection_points/specs/basic.spec (which is supposed
to serve as a template) hard to follow. The comments don't seem to
explain what the detach and wait functions actually do, and how and
why one might want to call them together.

--
Peter Geoghegan

On Mon, Jul 7, 2025 at 7:43 PM Michael Paquier <michael@paquier.xyz> wrote:
> That's a property that Noah was looking after when he's worked on his
> specs with the VACUUM/GRANT frictions, something that one would get
> with a debugger: keep waiting and allow the point to be detached in
> parallel.

I'm finding that the FreeBSD Meson CI target consistently fails with
this setup, though. And with just about any variant I can think of;
seems to fail quite reliably. The initial SELECT backwards scan
statement will complete without ever waiting (though only on CI).

Do you know what that might be? It would be a lot easier if there was
at least a way to debug this locally.

> > For what it's worth, I found
> > src/test/modules/injection_points/specs/basic.spec (which is supposed
> > to serve as a template) hard to follow. The comments don't seem to
> > explain what the detach and wait functions actually do, and how and
> > why one might want to call them together.
>
> If you see ways to improve the existing template, please feel free to
> propose something, sure.

I'll need to figure this out for myself first.

--
Peter Geoghegan

On Mon, Jul 07, 2025 at 09:40:20PM -0400, Peter Geoghegan wrote:
> I'm finding that the FreeBSD Meson CI target consistently fails with
> this setup, though. And with just about any variant I can think of;
> seems to fail quite reliably. The initial SELECT backwards scan
> statement will complete without ever waiting (though only on CI).
> 
> Do you know what that might be? It would be a lot easier if there was
> at least a way to debug this locally.

FreeBSD's scheduler is different enough to exercise quite-different relative
timings of process wake-up.  I got a lot of FreeBSD failures when my tests had
underspecified the order of events.

If it continues to be a problem, consider sharing the patch that's behaving
this way for you.

On Tue, Jul 08, 2025 at 11:21:20AM -0400, Peter Geoghegan wrote:
> On Mon, Jul 7, 2025 at 9:53 PM Noah Misch <noah@leadboat.com> wrote:
> > If it continues to be a problem, consider sharing the patch that's behaving
> > this way for you.
> 
> Attached patch shows my current progress with the isolation test.

Nothing looks suspicious in that code.

> I also attach diff output of the FreeBSD failures. Notice that the
> line "backwards_scan_session: NOTICE:  notice triggered for injection
> point lock-and-validate-new-lastcurrblkno" is completely absent from
> the test output. This absence indicates that the desired test coverage
> is totally missing on FreeBSD -- so the test is completely broken on
> FreeBSD.
> 
> I ran "meson test --suite setup --suite nbtree -q --print-errorlogs"
> in a loop 500 times on my Debian workstation without seeing any
> failures. Seems stable there. Whereas the FreeBSD target hasn't even
> passed once out of more than a dozen attempts. Seems to be reliably
> broken on FreeBSD.

> -backwards_scan_session: NOTICE:  notice triggered for injection point lock-and-validate-new-lastcurrblkno
> +ERROR:  could not find injection point lock-and-validate-left to wake up

Agreed.  Perhaps it's getting a different plan type on FreeBSD, so it's not
even reaching the INJECTION_POINT() calls?  That would be consistent with
these output diffs having no ERROR from attach/detach.  Some things I'd try:

- Add a plain elog(WARNING) before each INJECTION_POINT()
- Use debug_print_plan or similar to confirm the plan type

On Tue, Jul 8, 2025 at 11:04 PM Noah Misch <noah@leadboat.com> wrote:
> > -backwards_scan_session: NOTICE:  notice triggered for injection point lock-and-validate-new-lastcurrblkno
> > +ERROR:  could not find injection point lock-and-validate-left to wake up
>
> Agreed.  Perhaps it's getting a different plan type on FreeBSD, so it's not
> even reaching the INJECTION_POINT() calls?  That would be consistent with
> these output diffs having no ERROR from attach/detach.  Some things I'd try:
>
> - Add a plain elog(WARNING) before each INJECTION_POINT()
> - Use debug_print_plan or similar to confirm the plan type

I added a pair of elog(WARNING) traces before each of the new
INJECTION_POINT() calls.

When I run the test against the FreeBSD CI target with this new
instrumentation, I see a WARNING that indicates that we've reached the
top of _bt_lock_and_validate_left as expected. I don't see any second
WARNING indicating that we've taken _bt_lock_and_validate_left's
unhappy path, though (and the test still fails). This doesn't look
like an issue with the planner.

I attach the relevant regression test output, that shows all this.

Thanks
--
Peter Geoghegan

On Tue, Jul 08, 2025 at 11:43:17PM -0400, Peter Geoghegan wrote:
> On Tue, Jul 8, 2025 at 11:04 PM Noah Misch <noah@leadboat.com> wrote:
> > > -backwards_scan_session: NOTICE:  notice triggered for injection point lock-and-validate-new-lastcurrblkno
> > > +ERROR:  could not find injection point lock-and-validate-left to wake up
> >
> > Agreed.  Perhaps it's getting a different plan type on FreeBSD, so it's not
> > even reaching the INJECTION_POINT() calls?  That would be consistent with
> > these output diffs having no ERROR from attach/detach.  Some things I'd try:
> >
> > - Add a plain elog(WARNING) before each INJECTION_POINT()
> > - Use debug_print_plan or similar to confirm the plan type
> 
> I added a pair of elog(WARNING) traces before each of the new
> INJECTION_POINT() calls.
> 
> When I run the test against the FreeBSD CI target with this new
> instrumentation, I see a WARNING that indicates that we've reached the
> top of _bt_lock_and_validate_left as expected. I don't see any second
> WARNING indicating that we've taken _bt_lock_and_validate_left's
> unhappy path, though (and the test still fails). This doesn't look
> like an issue with the planner.
> 
> I attach the relevant regression test output, that shows all this.

Looking at .cirrus.tasks.yml, I bet the key factor is that CI task using
debug_parallel_query=regress.  I bet the leader is attached to the injection
point, but the WARNING is reached in a parallel worker.

If that matches what you see, I'd use a PARALLEL RESTRICTED or PARALLEL UNSAFE
function in your query to ensure the code in question runs in the leader.
(Simply overriding debug_parallel_query is less robust, because test runs
could use other settings that cause selection of a parallel plan.)

On Wed, Jul 9, 2025 at 10:24 PM Noah Misch <noah@leadboat.com> wrote:
> Looking at .cirrus.tasks.yml, I bet the key factor is that CI task using
> debug_parallel_query=regress.  I bet the leader is attached to the injection
> point, but the WARNING is reached in a parallel worker.

Yep, that was it.

> If that matches what you see, I'd use a PARALLEL RESTRICTED or PARALLEL UNSAFE
> function in your query to ensure the code in question runs in the leader.

That seems like the way to go.

At some point I'll start a new thread with a formal patch proposal,
that'll include the tests on this thread. I also plan on using
injection points to write a simple/serial regression test exercising
the nbtree code that completes an incomplete split (following a hard
crash/error).

Thanks again
--
Peter Geoghegan

On Thu, Jul 10, 2025 at 06:58:58PM -0400, Peter Geoghegan wrote:
> On Wed, Jul 9, 2025 at 10:24 PM Noah Misch <noah@leadboat.com> wrote:
> > Looking at .cirrus.tasks.yml, I bet the key factor is that CI task using
> > debug_parallel_query=regress.  I bet the leader is attached to the injection
> > point, but the WARNING is reached in a parallel worker.
>
> Yep, that was it.

Catching up on things a bit.  Cool to see that you have found out the
origin of the problem.

> At some point I'll start a new thread with a formal patch proposal,
> that'll include the tests on this thread. I also plan on using
> injection points to write a simple/serial regression test exercising
> the nbtree code that completes an incomplete split (following a hard
> crash/error).

It sounds to me that an ERROR in an SQL and/or isolation test would be
enough.  If you are looking at some replay cases, a TAP test would be
the way to go.
--
Michael