On Tue, May 07, 2024 at 10:17:49AM +0900, Michael Paquier wrote:
> 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
Yes, the base was 713cfaf (Sunday).
> 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.
Yes, we'd be raising INJ_MAX_CONDITION more often under this approach.
> 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,
Can you say more about that? The only behavior change known to me is that a
given injection point workload uses more of INJ_MAX_CONDITION. If there's
another behavior change, it was likely unintended.
> 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.
The problem I'm trying to tackle in this thread is to make
src/test/modules/gin installcheck-safe. $SUBJECT's commit 5105c90 started
that work, having seen the intarray test suite break when run concurrently
with the injection_points test suite. That combination still does break at
the exit-time race condition. To reproduce, apply this attachment to add
sleeps, and run:
make -C src/test/modules/gin installcheck USE_MODULE_DB=1 & sleep 2; make -C
contrib/intarray installcheck USE_MODULE_DB=1
Separately, I see injection_points_attach() populates InjectionPointCondition
after InjectionPointAttach(). Shouldn't InjectionPointAttach() come last, to
avoid the same sort of race? I've not tried to reproduce that one.
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out
b/src/test/modules/gin/expected/gin_incomplete_splits.out
index 15574e5..4bc5ef1 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -126,6 +126,12 @@ SELECT
injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
select insert_until_fail(:next_i) as next_i
\gset
NOTICE: failed with: error triggered for injection point
gin-leave-leaf-split-incomplete
+SELECT pg_sleep(10);
+ pg_sleep
+----------
+
+(1 row)
+
SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
injection_points_detach
-------------------------
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql
b/src/test/modules/gin/sql/gin_incomplete_splits.sql
index ebf0f62..fb66bac 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -118,6 +118,7 @@ select verify(:next_i);
SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
select insert_until_fail(:next_i) as next_i
\gset
+SELECT pg_sleep(10);
SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
-- Verify that a scan works even though there's an incomplete split
diff --git a/src/test/modules/injection_points/injection_points.c
b/src/test/modules/injection_points/injection_points.c
index a74a4a2..f9e8a1f 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -18,6 +18,7 @@
#include "postgres.h"
#include "fmgr.h"
+#include "libpq/pqsignal.h"
#include "miscadmin.h"
#include "storage/condition_variable.h"
#include "storage/dsm_registry.h"
@@ -213,6 +214,14 @@ injection_points_cleanup(int code, Datum arg)
void
injection_error(const char *name)
{
+ if (strcmp(name, "gin-leave-leaf-split-incomplete") == 0 &&
+ !injection_point_allowed(name))
+ {
+ sigprocmask(SIG_SETMASK, &BlockSig, NULL);
+ pg_usleep(20 * USECS_PER_SEC); /* let other suite detach */
+ sigprocmask(SIG_SETMASK, &UnBlockSig, NULL);
+ }
+
if (!injection_point_allowed(name))
return;
