From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Date: Tue, 7 May 2024 11:53:10 -0700
Message-ID: <20240507185310.d3@rfd.leadboat.com> (raw)
In-Reply-To: <ZjmBPeB6LQeHVHZ7@paquier.xyz>
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>
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.
Attachments:repro-inj-exit-race-v1.patch (text/plain, 2.1K) downloaddiff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.outindex 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.sqlindex 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.cindex 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;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, michael@paquier.xyz, hlinnaka@iki.fi, tgl@sss.pgh.pa.us
Subject: Re: Weird test mixup
In-Reply-To: <20240507185310.d3@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox