Re: Weird test mixup

On Mon, May 06, 2024 at 02:23:24PM -0700, Noah Misch wrote:
> Here's how I've patched it locally.  It does avoid changing the backend-side,
> which has some attraction.  Shall I just push this?

It looks like you did not rebase on top of HEAD to avoid the spinlock
taken with InjectionPointDetach() in the shmem callback.  I think that
you'd mean the attached, once rebased (apologies I've reset the author
field).

+ *    s1: local-attach to POINT
+ *    s2: yield CPU before InjectionPointRun(POINT) calls injection_callback()
+ *    s1: exit()
+ *    s2: run POINT as though it had been non-local

I see.  So you are taking a shortcut in the shape of never resetting
the name of a condition, so as it is possible to let the point of step
4 check the runtime condition with a condition still stored while the
point has been detached, removed from the hash table.

         if (strcmp(condition->name, name) == 0)
         {
+            condition->valid = false;
             condition->pid = 0;
-            condition->name[0] = '\0';
         }
     }

As of HEAD, we rely on InjectionPointCondition->name to be set to
check if a condition is valid.  Your patch adds a different variable
to do mostly the same thing, and never clears the name in any existing
conditions.  A side effect is that this causes the conditions to pile
up on a running server when running installcheck, and assuming that
many test suites are run on a server left running this could cause
spurious failures when failing to find a new slot.  Always resetting
condition->name when detaching a point is a simpler flow and saner
IMO.

Overall, this switches from one detach behavior to a different one,
which may or may not be intuitive depending on what one is looking
for.  FWIW, I see InjectionPointCondition as something that should be
around as long as its injection point exists, with the condition
entirely gone once the point is detached because it should not exist
anymore on the server running, with no information left in shmem.

Through your patch, you make conditions have a different meaning, with
a mix of "local" definition, but it is kind of permanent as it keeps a
trace of the point's name in shmem.  I find the behavior of the patch
less intuitive.  Perhaps it would be interesting to see first the bug
and/or problem you are trying to tackle with this different behavior
as I feel like we could do something even with the module as-is.  As
far as I understand, the implementation of the module on HEAD allows
one to emulate a breakpoint with a wait/wake, which can avoid the
window mentioned in step 2.  Even if a wait point is detached
concurrently, it can be awaken with its traces in shmem removed.
--
Michael