Feedback-ID: i0fe9450f:Fastmail
Date: Fri, 15 Mar 2024 16:39:12 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Weird test mixup
Message-ID: <ZfP7IDs9TvrKe49x@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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="Db+sAKqJkSXvqX22"
Content-Disposition: inline
In-Reply-To: <2393552.1710458033@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/ZfP7IDs9TvrKe49x%40paquier.xyz>
Precedence: bulk


--Db+sAKqJkSXvqX22
Content-Type: multipart/mixed; boundary="W++qaCAsSk2jDONr"
Content-Disposition: inline


--W++qaCAsSk2jDONr
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Or we could just disable runningcheck because of the concurrency
>> requirement in this test.  The test would still be able to run, just
>> less times.
>=20
> No, actually we *must* mark all these tests NO_INSTALLCHECK if we
> stick with the current definition of injection points.  The point
> of installcheck mode is that the tests are supposed to be safe to
> run in a live installation.  Side-effects occurring in other
> databases are completely not OK.

I really don't want to plug any runtime conditions into the backend
APIs, because there can be so much more that can be done there than
only restricting a callback to a database.  I can imagine process type
restrictions, process PID restrictions, etc.  So this knowledge should
stick into the test module itself, and be expanded there.  That's
easier ABI-wise, as well.

> I can see that some tests would want to be able to inject code
> cluster-wide, but I bet that's going to be a small minority.
> I suggest that we invent a notion of "global" vs "local"
> injection points, where a "local" one only fires in the DB it
> was defined in.  Then only tests that require a global injection
> point need to be NO_INSTALLCHECK.

Attached is a POC of what could be done.  I have extended the module
injection_points so as it is possible to register what I'm calling a
"condition" in the module that can be defined with a new SQL function.

The condition is stored in shared memory with the point name, then at
runtime the conditions are cross-checked in the callbacks.  With the
interface of this patch, the condition should be registered *before* a
point is attached, but this stuff could also be written so as
injection_points_attach() takes an optional argument with a database
name.  Or this could use a different, new SQL function, say a
injection_points_attach_local() that registers a condition with
MyDatabaseId on top of attaching the point, making the whole happening
while holding once the spinlock of the shmem state for the module.

By the way, modules/gin/ was missing missing a detach, so the test was
not repeatable with successive installchecks.  Adding a pg_sleep of a
few seconds after 'gin-leave-leaf-split-incomplete' is registered
enlarges the window, and the patch avoids failures when running
installcheck in parallel for modules/gin/ and something else using
gin, like contrib/btree_gin/:
while make USE_MODULE_DB=3D1 installcheck; do :; done

0001 is the condition facility for the module, 0002 is a fix for the
GIN test.  Thoughts are welcome.
--
Michael

--W++qaCAsSk2jDONr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Introduce-runtime-conditions-in-module-injection_poi.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 5f00092a0bd193f70d9daff77caacfc2ba83ca52 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 15 Mar 2024 16:03:05 +0900
Subject: [PATCH 1/2] Introduce runtime conditions in module injection_points

This adds a new SQL function injection_points_condition() that can be
used to register a runtime condition to an injection point, declared or
not.  For now, it is possible to register a database name to make an
injection point run only on a defined database.
---
 .../expected/injection_points.out             |  54 ++++++++
 .../injection_points--1.0.sql                 |  13 ++
 .../injection_points/injection_points.c       | 131 ++++++++++++++++++
 .../injection_points/sql/injection_points.sql |  14 ++
 4 files changed, 212 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..ccb83b291f 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -115,4 +115,58 @@ NOTICE:  notice triggered for injection point TestInje=
ctionLog2
 =20
 (1 row)
=20
+SELECT injection_points_detach('TestInjectionLog2');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
+-- Conditions
+SELECT current_database() AS datname \gset
+-- Register injection point to run on database 'postgres'.
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+SELECT injection_points_condition('TestConditionError', 'postgres');
+ injection_points_condition=20
+----------------------------
+=20
+(1 row)
+
+SELECT injection_points_run('TestConditionError'); -- nothing
+ injection_points_run=20
+----------------------
+=20
+(1 row)
+
+SELECT injection_points_detach('TestConditionError');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
+-- Register injection point to run on current database
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach=20
+-------------------------
+=20
+(1 row)
+
+SELECT injection_points_condition('TestConditionError', :'datname');
+ injection_points_condition=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)
+
 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..b707f23dad 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,19 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'injection_points_wakeup'
 LANGUAGE C STRICT PARALLEL UNSAFE;
=20
+--
+-- injection_points_condition()
+--
+-- Register a runtime condition associated to an injection point.
+--
+-- dbname can be specified to restrict an injection point to run on a
+-- wanted database.
+--
+CREATE FUNCTION injection_points_condition(IN point_name TEXT, IN dbname t=
ext)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_condition'
+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..ce9317f0ee 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -17,7 +17,9 @@
=20
 #include "postgres.h"
=20
+#include "commands/dbcommands.h"
 #include "fmgr.h"
+#include "miscadmin.h"
 #include "storage/condition_variable.h"
 #include "storage/dsm_registry.h"
 #include "storage/lwlock.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	8
+
+/*
+ * Conditions related to injection points.  This tracks in shared memory t=
he
+ * some runtime conditions under which an injection point is allowed to ru=
n.
+ *
+ * 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];
+
+	/* Oid of the database where the injection point is allowed to run */
+	Oid		dboid;
+} 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;
@@ -67,6 +89,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 +110,62 @@ 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 that if the database matches with what's used in this
+			 * backend.
+			 */
+			if (MyDatabaseId !=3D condition->dboid)
+			{
+				result =3D false;
+				break;
+			}
+		}
+	}
+
+	SpinLockRelease(&inj_state->lock);
+
+	return result;
+}
+
 /* 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 +180,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
@@ -235,6 +307,48 @@ injection_points_wakeup(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
=20
+/*
+ * injection_points_condition
+ *
+ * Register a runtime condition associated to an injection point.  Current=
ly,
+ * it is possible to define a database name where an injection point can be
+ * restricted to run on.
+ */
+PG_FUNCTION_INFO_V1(injection_points_condition);
+Datum
+injection_points_condition(PG_FUNCTION_ARGS)
+{
+	char	   *name =3D text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *dbname =3D text_to_cstring(PG_GETARG_TEXT_PP(1));
+	Oid			dboid =3D get_database_oid(dbname, false);
+	int			index =3D -1;
+
+	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 (condition->name[0] =3D=3D '\0')
+		{
+			index =3D i;
+			strlcpy(condition->name, name, INJ_NAME_MAXLEN);
+			condition->dboid =3D dboid;
+			break;
+		}
+	}
+	SpinLockRelease(&inj_state->lock);
+
+	if (index < 0)
+		elog(ERROR, "could not find free slot for condition of injection point %=
s",
+			 name);
+
+	PG_RETURN_VOID();
+}
+
 /*
  * SQL function for dropping an injection point.
  */
@@ -246,5 +360,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->dboid =3D InvalidOid;
+			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..1b52e7e6d7 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -30,5 +30,19 @@ 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');
+
+-- Conditions
+SELECT current_database() AS datname \gset
+-- Register injection point to run on database 'postgres'.
+SELECT injection_points_attach('TestConditionError', 'error');
+SELECT injection_points_condition('TestConditionError', 'postgres');
+SELECT injection_points_run('TestConditionError'); -- nothing
+SELECT injection_points_detach('TestConditionError');
+-- Register injection point to run on current database
+SELECT injection_points_attach('TestConditionError', 'error');
+SELECT injection_points_condition('TestConditionError', :'datname');
+SELECT injection_points_run('TestConditionError'); -- error
+SELECT injection_points_detach('TestConditionError');
=20
 DROP EXTENSION injection_points;
--=20
2.43.0


--W++qaCAsSk2jDONr
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0002-Restrict-GIN-test-with-injection-points-to-run-on-lo.patch"
Content-Transfer-Encoding: quoted-printable

=46rom f0b6e81a8652cbfe8733c34bcd035997392011b8 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Fri, 15 Mar 2024 16:26:52 +0900
Subject: [PATCH 2/2] Restrict GIN test with injection points to run on local
 database

It is possible that this test conflicts with parallel installcheck
activities.  While on it, add a forgotten detach to the GIN test to make
it repeatable.
---
 .../gin/expected/gin_incomplete_splits.out    | 27 +++++++++++++++++++
 .../modules/gin/sql/gin_incomplete_splits.sql |  8 ++++++
 2 files changed, 35 insertions(+)

diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/=
test/modules/gin/expected/gin_incomplete_splits.out
index 9a60f8c128..7803dd7a90 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -14,7 +14,16 @@
 create extension injection_points;
 -- Use the index for all the queries
 set enable_seqscan=3Doff;
+-- Limit all the injection points of this script to run on the current
+-- database, for safety under concurrent activity.
+SELECT current_database() AS datname \gset
 -- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_condition('gin-finish-incomplete-split', :'datname=
');
+ injection_points_condition=20
+----------------------------
+=20
+(1 row)
+
 SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
  injection_points_attach=20
 -------------------------
@@ -109,6 +118,12 @@ select verify(:next_i);
 --
 -- Test incomplete leaf split
 --
+SELECT injection_points_condition('gin-leave-leaf-split-incomplete', :'dat=
name');
+ injection_points_condition=20
+----------------------------
+=20
+(1 row)
+
 SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
  injection_points_attach=20
 -------------------------
@@ -145,6 +160,12 @@ select verify(:next_i);
 --
 -- Test incomplete internal page split
 --
+SELECT injection_points_condition('gin-leave-internal-split-incomplete', :=
'datname');
+ injection_points_condition=20
+----------------------------
+=20
+(1 row)
+
 SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'err=
or');
  injection_points_attach=20
 -------------------------
@@ -178,3 +199,9 @@ select verify(:next_i);
  t
 (1 row)
=20
+SELECT injection_points_detach('gin-finish-incomplete-split');
+ injection_points_detach=20
+-------------------------
+=20
+(1 row)
+
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/=
modules/gin/sql/gin_incomplete_splits.sql
index 4e180b2519..5b4521edd9 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -17,7 +17,12 @@ create extension injection_points;
 -- Use the index for all the queries
 set enable_seqscan=3Doff;
=20
+-- Limit all the injection points of this script to run on the current
+-- database, for safety under concurrent activity.
+SELECT current_database() AS datname \gset
+
 -- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_condition('gin-finish-incomplete-split', :'datname=
');
 SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
=20
 --
@@ -111,6 +116,7 @@ select verify(:next_i);
 --
 -- Test incomplete leaf split
 --
+SELECT injection_points_condition('gin-leave-leaf-split-incomplete', :'dat=
name');
 SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
 select insert_until_fail(:next_i) as next_i
 \gset
@@ -129,6 +135,7 @@ select verify(:next_i);
 --
 -- Test incomplete internal page split
 --
+SELECT injection_points_condition('gin-leave-internal-split-incomplete', :=
'datname');
 SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'err=
or');
 select insert_until_fail(:next_i, 100) as next_i
 \gset
@@ -142,3 +149,4 @@ select insert_n(:next_i, 10) as next_i
 \gset
 -- Verify that a scan still works
 select verify(:next_i);
+SELECT injection_points_detach('gin-finish-incomplete-split');
--=20
2.43.0


--W++qaCAsSk2jDONr--

--Db+sAKqJkSXvqX22
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmXz+yAACgkQnvQgOdby
QH344A//Yi/3ooDBoLzLA3pkjPydWiBEG+XHV0I784mEF1FYadcL2PhhyUpl8S/7
vb17eQbDbb8EFxlycubL4Y/7Tmdp7FbEx2oeBocIOxshVz6af841l1RP9WlR8Hj1
eX/MyO/hfN8/pgwkuZfWGJXhL2qZsPzntQgC7/bQCzZREBk8Hn+aSt0ARgilYpI4
0vAEJnUmx7sobtkLtgF4RTb/s1bVK9lwqvlbJ/ovy7HFKM1pmLohJvJh35yWDfoC
6aIz6TlNf6opW3mnbtnc+2g88xFu84Kvu4LDNzWL54KUABeq2qRfVNXcNmCfjHLE
V+uQEYb6rv12e8eCHiayEnsJmd2SHHJjKFuQ/WWCEozLEkqkzqOrjNtcy/OZnJ51
Yaxj3rX4IaLdUDymizr81kNBecXaDRx7/7uu3m2j2KdkM9AtlYYGi4WOKwf3Kaaw
Effi9HcesR16IT+nW8ZxxXosHIAOCFkzhmGUPv9PYdrgjLQ2e828W1zGGq0qJbz/
SAQjLh9CrExVh9Y0whnZsG6gj2LXr4R4Rj+NFURyW0ambi4UXe6wrJ92Q84dDjMO
JW+k/JPcO3OvAN7sRVsq4+/W8QwDqDTIz2xxPiA2INEW7chTdFqzwSwkaK0EYEl7
dbbswuizR5ElGHANDdtgH9WgdyosydwvI/vQvgTkumnD0HaKQRs=
=1pLF
-----END PGP SIGNATURE-----

--Db+sAKqJkSXvqX22--