Date: Mon, 6 May 2024 14:23:24 -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: <20240506212324.2e.nmisch@google.com>
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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="G0okDVnm3ADJuraz"
Content-Disposition: inline
In-Reply-To: <ZjgsaYs44ici4QMW@paquier.xyz>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240506212324.2e.nmisch%40google.com>
Precedence: bulk


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

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.

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

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.c
index 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);

--G0okDVnm3ADJuraz--