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: Mon, 6 May 2024 14:23:24 -0700
Message-ID: <20240506212324.2e.nmisch@google.com> (raw)
In-Reply-To: <ZjgsaYs44ici4QMW@paquier.xyz>
References: <2387221.1710454778@sss.pgh.pa.us>
<ZfOABU4ahF2QZQ6N@paquier.xyz>
<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>
On Mon, May 06, 2024 at 10:03:37AM +0900, Michael Paquier wrote:
> On Thu, May 02, 2024 at 12:35:55PM -0700, Noah Misch wrote:
> > I should have given a simpler example:
> >
> > s1: local-attach to POINT
> > s2: enter InjectionPointRun(POINT), yield CPU just before injection_callback()
> > s1: exit
> > s2: wake up and run POINT as though it had been non-local
Here's how I've patched it locally. It does avoid changing the backend-side,
which has some attraction. Shall I just push this?
> Hmm. Even if you were to emulate that in a controlled manner, you
> would need a second injection point that does a wait in s2, which is
> something that would happen before injection_callback() and before
> scanning the local entry. This relies on the fact that we're holding
> CPU in s2 between the backend shmem hash table lookup and the callback
> being called.
Right. We would need "second-level injection points" to write a test for that
race in the injection point system.
Attachments:inplace030-inj-exit-race-v1.patch (text/plain, 3.0K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Fix race condition in backend exit after injection_points_set_local().
Symptoms were like those prompting commit
f587338dec87d3c35b025e131c5977930ac69077. That is, under installcheck,
backends from unrelated tests could run the injection points.
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.cindex ace386d..75466d3 100644--- a/src/test/modules/injection_points/injection_points.c+++ b/src/test/modules/injection_points/injection_points.c@@ -37,7 +37,13 @@ PG_MODULE_MAGIC;
/*
* Conditions related to injection points. This tracks in shared memory the
- * runtime conditions under which an injection point is allowed to run.+ * runtime conditions under which an injection point is allowed to run. Once+ * set, a name is never changed. That avoids a race condition:+ *+ * 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
*
* If more types of runtime conditions need to be tracked, this structure
* should be expanded.
@@ -49,6 +55,9 @@ typedef struct InjectionPointCondition
/* ID of the process where the injection point is allowed to run */
int pid;
++ /* Should "pid" run this injection point, or not? */+ bool valid;
} InjectionPointCondition;
/* Shared state information for injection points. */
@@ -133,7 +142,7 @@ injection_point_allowed(const char *name)
{
InjectionPointCondition *condition = &inj_state->conditions[i];
- if (strcmp(condition->name, name) == 0)+ if (condition->valid && strcmp(condition->name, name) == 0)
{
/*
* Check if this injection point is allowed to run in this
@@ -168,15 +177,16 @@ injection_points_cleanup(int code, Datum arg)
{
InjectionPointCondition *condition = &inj_state->conditions[i];
- if (condition->name[0] == '\0')+ if (!condition->valid)
continue;
+ Assert(condition->name[0] != '\0');
if (condition->pid != MyProcPid)
continue;
/* Detach the injection point and unregister condition */
InjectionPointDetach(condition->name);
- condition->name[0] = '\0';+ condition->valid = false;
condition->pid = 0;
}
SpinLockRelease(&inj_state->lock);
@@ -299,11 +309,13 @@ injection_points_attach(PG_FUNCTION_ARGS)
{
InjectionPointCondition *condition = &inj_state->conditions[i];
- if (condition->name[0] == '\0')+ if (strcmp(condition->name, name) == 0 ||+ condition->name[0] == '\0')
{
index = i;
strlcpy(condition->name, name, INJ_NAME_MAXLEN);
condition->pid = MyProcPid;
+ condition->valid = true;
break;
}
}
@@ -416,8 +428,8 @@ injection_points_detach(PG_FUNCTION_ARGS)
if (strcmp(condition->name, name) == 0)
{
+ condition->valid = false;
condition->pid = 0;
- condition->name[0] = '\0';
}
}
SpinLockRelease(&inj_state->lock);
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: <20240506212324.2e.nmisch@google.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