Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 18 Mar 2024 10:04:45 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Message-ID: <ZfeTLfltCTimaVbU@paquier.xyz>
References: <10fd6cdd-c5d9-46fe-9fa1-7e661191309e@iki.fi>
 <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="a7kUcSh+/lJuQ3ky"
Content-Disposition: inline
In-Reply-To: <ZfTcZW1-Kh3fFLnZ@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZfeTLfltCTimaVbU%40paquier.xyz>
Precedence: bulk


--a7kUcSh+/lJuQ3ky
Content-Type: multipart/mixed; boundary="px/YwvpEat+2amFt"
Content-Disposition: inline


--px/YwvpEat+2amFt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote:
> Linking all the points to a PID with a injection_points_attach_local()
> that switches a static flag while registering a before_shmem_exit() to
> do an automated cleanup sounds like the simplest approach to me based
> on what I'm reading on this thread.

Please find a patch to do exactly that, without touching the backend
APIs.  0001 adds a new function call injection_points_local() that can
be added on top of a SQL test to make it concurrent-safe.  0002 is the
fix for the GIN tests.

I am going to add an open item to not forget about all that.

Comments are welcome.
--
Michael

--px/YwvpEat+2amFt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0001-Introduce-runtime-conditions-in-module-injection_.patch"
Content-Transfer-Encoding: quoted-printable

=46rom a4c219f0142a36b2a009af1f9e9affdeab6ab383 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 18 Mar 2024 09:53:05 +0900
Subject: [PATCH v2 1/2] Introduce runtime conditions in module
 injection_points

This adds a new SQL function injection_points_local() that can be used
to force injection points to be run only on the process where they are
attached.  This is handy for SQL tests to:
- detach automatically local injection points when the process exits.
- allow tests with injection points to run concurrently with other test
suites.
---
 .../expected/injection_points.out             |  77 ++++++++
 .../injection_points--1.0.sql                 |  11 ++
 .../injection_points/injection_points.c       | 176 ++++++++++++++++++
 .../injection_points/sql/injection_points.sql |  20 ++
 src/tools/pgindent/typedefs.list              |   1 +
 5 files changed, 285 insertions(+)

diff --git a/src/test/modules/injection_points/expected/injection_points.ou=
t b/src/test/modules/injection_points/expected/injection_points.out
index 827456ccc5..f24a602def 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -115,4 +115,81 @@ NOTICE:  notice triggered for injection point TestInje=
ctionLog2
 =20
 (1 row)
=20
+SELECT injection_points_detach('TestInjectionLog2');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
+-- Runtime conditions
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+-- Any follow-up injection point attached will be local to this process.
+SELECT injection_points_local();
+ injection_points_local=20
+------------------------
+=20
+(1 row)
+
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+SELECT injection_points_attach('TestConditionLocal2', 'notice');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+SELECT injection_points_run('TestConditionLocal1'); -- error
+ERROR:  error triggered for injection point TestConditionLocal1
+SELECT injection_points_run('TestConditionLocal2'); -- notice
+NOTICE:  notice triggered for injection point TestConditionLocal2
+ injection_points_run=20
+----------------------
+=20
+(1 row)
+
+-- reload, local injection points should be gone.
+\c
+SELECT injection_points_run('TestConditionLocal1'); -- nothing
+ injection_points_run=20
+----------------------
+=20
+(1 row)
+
+SELECT injection_points_run('TestConditionLocal2'); -- nothing
+ injection_points_run=20
+----------------------
+=20
+(1 row)
+
+SELECT injection_points_run('TestConditionError'); -- error
+ERROR:  error triggered for injection point TestConditionError
+SELECT injection_points_detach('TestConditionError');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
+-- Attaching injection points that use the same name as one defined locally
+-- previously should work.
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+SELECT injection_points_detach('TestConditionLocal1');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
 DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/=
src/test/modules/injection_points/injection_points--1.0.sql
index 0a2e59aba7..1813a4d6d8 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -34,6 +34,17 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'injection_points_wakeup'
 LANGUAGE C STRICT PARALLEL UNSAFE;
=20
+--
+-- injection_points_local()
+--
+-- Trigger switch to link any future injection points attached to the
+-- current process, useful to make SQL tests concurrently-safe.
+--
+CREATE FUNCTION injection_points_local()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_local'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
 --
 -- injection_points_detach()
 --
diff --git a/src/test/modules/injection_points/injection_points.c b/src/tes=
t/modules/injection_points/injection_points.c
index 0730254d54..8be081db99 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -18,8 +18,10 @@
 #include "postgres.h"
=20
 #include "fmgr.h"
+#include "miscadmin.h"
 #include "storage/condition_variable.h"
 #include "storage/dsm_registry.h"
+#include "storage/ipc.h"
 #include "storage/lwlock.h"
 #include "storage/shmem.h"
 #include "utils/builtins.h"
@@ -31,6 +33,23 @@ PG_MODULE_MAGIC;
 /* Maximum number of waits usable in injection points at once */
 #define INJ_MAX_WAIT	8
 #define INJ_NAME_MAXLEN	64
+#define	INJ_MAX_CONDITION	4
+
+/*
+ * Conditions related to injection points.  This tracks in shared memory t=
he
+ * runtime conditions under which an injection point is allowed to run.
+ *
+ * If more types of runtime conditions need to be tracked, this structure
+ * should be expanded.
+ */
+typedef struct InjectionPointCondition
+{
+	/* Name of the injection point related to its  */
+	char		name[INJ_NAME_MAXLEN];
+
+	/* ID of the process where the injection point is allowed to run */
+	int			pid;
+} InjectionPointCondition;
=20
 /* Shared state information for injection points. */
 typedef struct InjectionPointSharedState
@@ -44,6 +63,9 @@ typedef struct InjectionPointSharedState
 	/* Names of injection points attached to wait counters */
 	char		name[INJ_MAX_WAIT][INJ_NAME_MAXLEN];
=20
+	/* Condition to run an injection point */
+	InjectionPointCondition condition[INJ_MAX_CONDITION];
+
 	/* Condition variable used for waits and wakeups */
 	ConditionVariable wait_point;
 } InjectionPointSharedState;
@@ -55,6 +77,8 @@ extern PGDLLEXPORT void injection_error(const char *name);
 extern PGDLLEXPORT void injection_notice(const char *name);
 extern PGDLLEXPORT void injection_wait(const char *name);
=20
+/* track if injection points attached in this process are linked to it */
+static bool injection_point_local =3D false;
=20
 /*
  * Callback for shared memory area initialization.
@@ -67,6 +91,7 @@ injection_point_init_state(void *ptr)
 	SpinLockInit(&state->lock);
 	memset(state->wait_counts, 0, sizeof(state->wait_counts));
 	memset(state->name, 0, sizeof(state->name));
+	memset(state->condition, 0, sizeof(state->condition));
 	ConditionVariableInit(&state->wait_point);
 }
=20
@@ -87,16 +112,92 @@ injection_init_shmem(void)
 								   &found);
 }
=20
+/*
+ * Check runtime conditions associated to an injection point.
+ *
+ * Returns true if the named injection point is allowed to run, and false
+ * otherwise.  Multiple conditions can be associated to a single injection
+ * point, so check them all.
+ */
+static bool
+injection_point_allowed(const char *name)
+{
+	bool		result =3D true;
+
+	if (inj_state =3D=3D NULL)
+		injection_init_shmem();
+
+	SpinLockAcquire(&inj_state->lock);
+
+	for (int i =3D 0; i < INJ_MAX_CONDITION; i++)
+	{
+		InjectionPointCondition *condition =3D &inj_state->condition[i];
+
+		if (strcmp(condition->name, name) =3D=3D 0)
+		{
+			/*
+			 * Check if this injection point is allowed to run in this
+			 * process.
+			 */
+			if (MyProcPid !=3D condition->pid)
+			{
+				result =3D false;
+				break;
+			}
+		}
+	}
+
+	SpinLockRelease(&inj_state->lock);
+
+	return result;
+}
+
+/*
+ * before_shmem_exit callback to remove injection points linked to a
+ * specific process.
+ */
+static void
+injection_points_cleanup(int code, Datum arg)
+{
+	/* Leave if nothing is tracked locally */
+	if (!injection_point_local)
+		return;
+
+	SpinLockAcquire(&inj_state->lock);
+	for (int i =3D 0; i < INJ_MAX_CONDITION; i++)
+	{
+		InjectionPointCondition *condition =3D &inj_state->condition[i];
+
+		if (condition->name[0] =3D=3D '\0')
+			continue;
+
+		if (condition->pid !=3D MyProcPid)
+			continue;
+
+		/* Detach the injection point and unregister condition */
+		InjectionPointDetach(condition->name);
+		condition->name[0] =3D '\0';
+		condition->pid =3D 0;
+	}
+	SpinLockRelease(&inj_state->lock);
+}
+
 /* Set of callbacks available to be attached to an injection point. */
 void
 injection_error(const char *name)
 {
+	if (!injection_point_allowed(name))
+		return;
+
 	elog(ERROR, "error triggered for injection point %s", name);
 }
=20
 void
 injection_notice(const char *name)
 {
+	if (!injection_point_allowed(name))
+		return;
+
 	elog(NOTICE, "notice triggered for injection point %s", name);
 }
=20
@@ -111,6 +212,9 @@ injection_wait(const char *name)
 	if (inj_state =3D=3D NULL)
 		injection_init_shmem();
=20
+	if (!injection_point_allowed(name))
+		return;
+
 	/*
 	 * Use the injection point name for this custom wait event.  Note that
 	 * this custom wait event name is not released, but we don't care much for
@@ -182,6 +286,35 @@ injection_points_attach(PG_FUNCTION_ARGS)
=20
 	InjectionPointAttach(name, "injection_points", function);
=20
+	if (injection_point_local)
+	{
+		int			index =3D -1;
+
+		/*
+		 * Register runtime condition to link this injection point to the
+		 * current process.
+		 */
+		SpinLockAcquire(&inj_state->lock);
+		for (int i =3D 0; i < INJ_MAX_CONDITION; i++)
+		{
+			InjectionPointCondition *condition =3D &inj_state->condition[i];
+
+			if (condition->name[0] =3D=3D '\0')
+			{
+				index =3D i;
+				strlcpy(condition->name, name, INJ_NAME_MAXLEN);
+				condition->pid =3D MyProcPid;
+				break;
+			}
+		}
+		SpinLockRelease(&inj_state->lock);
+
+		if (index < 0)
+			elog(FATAL,
+				 "could not find free slot for condition of injection point %s",
+				 name);
+	}
+
 	PG_RETURN_VOID();
 }
=20
@@ -235,6 +368,32 @@ injection_points_wakeup(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
=20
+/*
+ * injection_points_local
+ *
+ * Track if any injection point created in this process ought to run only
+ * in this process.  Such injection points are detached automatically when
+ * this process exits.  This is useful to make test suites concurrent-safe.
+ */
+PG_FUNCTION_INFO_V1(injection_points_local);
+Datum
+injection_points_local(PG_FUNCTION_ARGS)
+{
+	/* Enable flag to add a runtime condition based on this process ID */
+	injection_point_local =3D true;
+
+	if (inj_state =3D=3D NULL)
+		injection_init_shmem();
+
+	/*
+	 * Register a before_shmem_exit callback to remove any injection points
+	 * linked to this process.
+	 */
+	before_shmem_exit(injection_points_cleanup, (Datum) 0);
+
+	PG_RETURN_VOID();
+}
+
 /*
  * SQL function for dropping an injection point.
  */
@@ -246,5 +405,22 @@ injection_points_detach(PG_FUNCTION_ARGS)
=20
 	InjectionPointDetach(name);
=20
+	if (inj_state =3D=3D NULL)
+		injection_init_shmem();
+
+	/* Clean up any conditions associated to this injection point */
+	SpinLockAcquire(&inj_state->lock);
+	for (int i =3D 0; i < INJ_MAX_CONDITION; i++)
+	{
+		InjectionPointCondition *condition =3D &inj_state->condition[i];
+
+		if (strcmp(condition->name, name) =3D=3D 0)
+		{
+			condition->pid =3D 0;
+			condition->name[0] =3D '\0';
+		}
+	}
+	SpinLockRelease(&inj_state->lock);
+
 	PG_RETURN_VOID();
 }
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/s=
rc/test/modules/injection_points/sql/injection_points.sql
index 23c7e435ad..27424bdfe7 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -30,5 +30,25 @@ SELECT injection_points_run('TestInjectionLog2'); -- not=
ice
 SELECT injection_points_detach('TestInjectionLog'); -- fails
=20
 SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_detach('TestInjectionLog2');
+
+-- Runtime conditions
+SELECT injection_points_attach('TestConditionError', 'error');
+-- Any follow-up injection point attached will be local to this process.
+SELECT injection_points_local();
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+SELECT injection_points_attach('TestConditionLocal2', 'notice');
+SELECT injection_points_run('TestConditionLocal1'); -- error
+SELECT injection_points_run('TestConditionLocal2'); -- notice
+-- reload, local injection points should be gone.
+\c
+SELECT injection_points_run('TestConditionLocal1'); -- nothing
+SELECT injection_points_run('TestConditionLocal2'); -- nothing
+SELECT injection_points_run('TestConditionError'); -- error
+SELECT injection_points_detach('TestConditionError');
+-- Attaching injection points that use the same name as one defined locally
+-- previously should work.
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+SELECT injection_points_detach('TestConditionLocal1');
=20
 DROP EXTENSION injection_points;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs=
=2Elist
index 6ca93b1e47..d429d6035a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1210,6 +1210,7 @@ InitSampleScan_function
 InitializeDSMForeignScan_function
 InitializeWorkerForeignScan_function
 InjectionPointCacheEntry
+InjectionPointCondition
 InjectionPointEntry
 InjectionPointSharedState
 InlineCodeBlock
--=20
2.43.0


--px/YwvpEat+2amFt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2-0002-Switch-GIN-tests-with-injection-points-to-be-conc.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 8d7338f68b6add2b28acfebd7541ef5c21913264 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 18 Mar 2024 09:56:35 +0900
Subject: [PATCH v2 2/2] Switch GIN tests with injection points to be
 concurrent-safe

---
 src/test/modules/gin/Makefile                           | 3 ---
 src/test/modules/gin/expected/gin_incomplete_splits.out | 7 +++++++
 src/test/modules/gin/meson.build                        | 2 --
 src/test/modules/gin/sql/gin_incomplete_splits.sql      | 3 +++
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/test/modules/gin/Makefile b/src/test/modules/gin/Makefile
index da4c9cea5e..e007e38ac2 100644
--- a/src/test/modules/gin/Makefile
+++ b/src/test/modules/gin/Makefile
@@ -4,9 +4,6 @@ EXTRA_INSTALL =3D src/test/modules/injection_points
=20
 REGRESS =3D gin_incomplete_splits
=20
-# The injection points are cluster-wide, so disable installcheck
-NO_INSTALLCHECK =3D 1
-
 ifdef USE_PGXS
 PG_CONFIG =3D pg_config
 PGXS :=3D $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/=
test/modules/gin/expected/gin_incomplete_splits.out
index 822b78cffc..f503d3dd3c 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -12,6 +12,13 @@
 -- This uses injection points to cause errors that leave some page
 -- splits in "incomplete" state
 create extension injection_points;
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_local();
+ injection_points_local=20
+------------------------
+=20
+(1 row)
+
 -- Use the index for all the queries
 set enable_seqscan=3Doff;
 -- Print a NOTICE whenever an incomplete split gets fixed
diff --git a/src/test/modules/gin/meson.build b/src/test/modules/gin/meson.=
build
index 5ec0760a27..9734b51de2 100644
--- a/src/test/modules/gin/meson.build
+++ b/src/test/modules/gin/meson.build
@@ -12,7 +12,5 @@ tests +=3D {
     'sql': [
       'gin_incomplete_splits',
     ],
-    # The injection points are cluster-wide, so disable installcheck
-    'runningcheck': false,
   },
 }
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/=
modules/gin/sql/gin_incomplete_splits.sql
index 71253b71f3..f413b54708 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -14,6 +14,9 @@
 -- splits in "incomplete" state
 create extension injection_points;
=20
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_local();
+
 -- Use the index for all the queries
 set enable_seqscan=3Doff;
=20
--=20
2.43.0


--px/YwvpEat+2amFt--

--a7kUcSh+/lJuQ3ky
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmX3ky0ACgkQnvQgOdby
QH01Gg/+LEFRJ0XLx6i6tnuboSHI45PpdAM+GBmSN2pdYQ3B3mEO6kgxP79UQiSa
J9fwKybhzm8hNtNXyIBFmuBb5utYdzISH6HHBKCfvdY2vIvIC8YbE5LZo1/Uv9sw
vOJI+YQmsV0jcmpbbQxqdLxHupUh7PVgr0aQbQlcIMPKqi1LmpsFwSLr63ozlp/e
pz1AayByatSkNpYTmMl2ubp4jqjuxk8mkYQDQFKZ9dzaHudtZEz6ciYrP/mI3eze
uWnlKYrQo9hwAdfGH5syELTw59sRHqTqGo80JOhQgABJ74g6CcjeGPhbAskLfcLs
+VwHir2ogst+PT9zbUPCF+RlOxA1E4TqfjuVVwzSEGMf5DeB3JO66nAFX6CyIz1/
r7Itgwpgx+LPmtDOikhpnHGgu0iI3h69ZwEpXgZvDiXDKLGWy/Avwt6KolUHu8gQ
xj7iZe8vmDC5+ZRpogf4THbirQsquMvks2NByIlzAsjAFMDOsW3nBkNCv6CnIM/K
5xWwINNK6cuK3RZmdcD+SenUCAFLEV4B0p2xBFVYWtfQockrD8szhuq03DX++M8Q
P5nmdtpL/FCwDdi5kYoYoMbsd1RzfQYJppQOWogD6UH9life6E9WNUFhH0a31Yk9
uV7MKYOexNyJRAigLQpCXfYRGr7SwMiAhs0r+JQkFuxnhOvJNVo=
=mZKL
-----END PGP SIGNATURE-----

--a7kUcSh+/lJuQ3ky--