Date: Tue, 7 May 2024 11:53:10 -0700
From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Message-ID: <20240507185310.d3@rfd.leadboat.com>
References: <2393552.1710458033@sss.pgh.pa.us>
 <ZfP7IDs9TvrKe49x@paquier.xyz>
 <e1a7ab78-1fa9-419c-b0d1-36d971bdf946@iki.fi>
 <ZfTcZW1-Kh3fFLnZ@paquier.xyz>
 <20240501231214.40@rfd.leadboat.com>
 <ZjNAUN5Hek-MyVWv@paquier.xyz>
 <20240502193555.a1.nmisch@google.com>
 <ZjgsaYs44ici4QMW@paquier.xyz>
 <20240506212324.2e.nmisch@google.com>
 <ZjmBPeB6LQeHVHZ7@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="u9qoEUcSzx4glh93"
Content-Disposition: inline
In-Reply-To: <ZjmBPeB6LQeHVHZ7@paquier.xyz>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240507185310.d3%40rfd.leadboat.com>
Precedence: bulk


--u9qoEUcSzx4glh93
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--u9qoEUcSzx4glh93
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="repro-inj-exit-race-v1.patch"

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;
 

--u9qoEUcSzx4glh93--