public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
6+ messages / 4 participants
[nested] [flat]

* [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
@ 2020-04-01 01:35  Justin Pryzby <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Justin Pryzby @ 2020-04-01 01:35 UTC (permalink / raw)

---
 doc/src/sgml/ref/cluster.sgml             | 12 ++++-
 doc/src/sgml/ref/vacuum.sgml              | 12 ++++-
 src/backend/commands/cluster.c            | 56 ++++++++++++++---------
 src/backend/commands/matview.c            |  3 +-
 src/backend/commands/tablecmds.c          |  2 +-
 src/backend/commands/vacuum.c             | 40 ++++++++--------
 src/backend/postmaster/autovacuum.c       |  1 +
 src/include/commands/cluster.h            |  4 +-
 src/include/commands/vacuum.h             |  5 +-
 src/test/regress/input/tablespace.source  | 13 ++++++
 src/test/regress/output/tablespace.source | 20 ++++++++
 11 files changed, 117 insertions(+), 51 deletions(-)

diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 6758ef97a6..9c0f5add59 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -29,6 +29,7 @@ CLUSTER [VERBOSE]
 
     VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
     TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+    INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
 
 </synopsis>
  </refsynopsisdiv>
@@ -106,6 +107,15 @@ CLUSTER [VERBOSE]
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>INDEX_TABLESPACE</literal></term>
+    <listitem>
+     <para>
+      Specifies that the table's indexes will be rebuilt on a new tablespace.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><literal>TABLESPACE</literal></term>
     <listitem>
@@ -128,7 +138,7 @@ CLUSTER [VERBOSE]
     <term><replaceable class="parameter">new_tablespace</replaceable></term>
     <listitem>
      <para>
-      The tablespace where the table will be rebuilt.
+      The tablespace where the table or its indexes will be rebuilt.
      </para>
     </listitem>
    </varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 5261a7c727..28cab119b6 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
     TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
     PARALLEL <replaceable class="parameter">integer</replaceable>
     TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+    INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
 
 <phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
 
@@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
     </listitem>
    </varlistentry>
 
+   <varlistentry>
+    <term><literal>INDEX_TABLESPACE</literal></term>
+    <listitem>
+     <para>
+      Specifies that the relation's indexes will be rebuilt on a new tablespace.
+     </para>
+    </listitem>
+   </varlistentry>
+
    <varlistentry>
     <term><replaceable class="parameter">boolean</replaceable></term>
     <listitem>
@@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
     <term><replaceable class="parameter">new_tablespace</replaceable></term>
     <listitem>
      <para>
-      The tablespace where the relation will be rebuilt.
+      The tablespace where the relation or its indexes will be rebuilt.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 626321e3ac..7fc6b69e95 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -71,7 +71,7 @@ typedef struct
 
 
 static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
-							 Oid NewTableSpaceOid);
+							 Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid);
 static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
 							bool verbose, bool *pSwapToastByContent,
 							TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -108,8 +108,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
 	ListCell   *lc;
 	ClusterParams params = {0};
 	bool		verbose = false;
-	/* Name of tablespace to use for clustered relation. */
-	char		*tablespaceName = NULL;
+	/* Names of tablespaces to use for clustered relations. */
+	char		*tablespaceName = NULL,
+				*idxtablespaceName = NULL;
 
 	/* Parse option list */
 	foreach(lc, stmt->params)
@@ -120,6 +121,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
 			verbose = defGetBoolean(opt);
 		else if (strcmp(opt->defname, "tablespace") == 0)
 			tablespaceName = defGetString(opt);
+		else if (strcmp(opt->defname, "index_tablespace") == 0)
+			idxtablespaceName = defGetString(opt);
 		else
 			ereport(ERROR,
 					(errcode(ERRCODE_SYNTAX_ERROR),
@@ -130,18 +133,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
 
 	params.options = (verbose ? CLUOPT_VERBOSE : 0);
 
-	/* Select tablespace Oid to use. */
-	if (tablespaceName)
-	{
-		params.tablespaceOid = get_tablespace_oid(tablespaceName, false);
-
-		/* Can't move a non-shared relation into pg_global */
-		if (params.tablespaceOid == GLOBALTABLESPACE_OID)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					 errmsg("cannot move non-shared relation to tablespace \"%s\"",
-							tablespaceName)));
-	}
+	/* Get tablespaces to use. */
+	params.tablespaceOid = tablespaceName ?
+		get_tablespace_oid(tablespaceName, false) : InvalidOid;
+	params.idxtablespaceOid = idxtablespaceName ?
+		get_tablespace_oid(idxtablespaceName, false) : InvalidOid;
 
 	if (stmt->relation != NULL)
 	{
@@ -293,8 +289,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
  * instead of index order.  This is the new implementation of VACUUM FULL,
  * and error messages should refer to the operation as VACUUM not CLUSTER.
  *
- * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
- * InvalidOid to use its current tablespace.
+ * "tablespaceOid" and "idxtablespaceOid" are the tablespaces where the relation
+ * and its indexes will be rebuilt, or InvalidOid to use their current
+ * tablespaces.
  */
 void
 cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
@@ -464,7 +461,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
 	TransferPredicateLocksToHeapRelation(OldHeap);
 
 	/* rebuild_relation does all the dirty work */
-	rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid);
+	rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid,
+			params->idxtablespaceOid);
 
 	/* NB: rebuild_relation does table_close() on OldHeap */
 
@@ -613,10 +611,11 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
  * NB: this routine closes OldHeap at the right time; caller should not.
  */
 static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid, Oid NewIdxTablespaceOid)
 {
 	Oid			tableOid = RelationGetRelid(OldHeap);
 	Oid			tableSpace = OldHeap->rd_rel->reltablespace;
+	Oid			idxtableSpace;
 	Oid			OIDNewHeap;
 	char		relpersistence;
 	bool		is_system_catalog;
@@ -626,7 +625,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
 
 	/* Use new tablespace if passed. */
 	if (OidIsValid(NewTablespaceOid))
+	{
 		tableSpace = NewTablespaceOid;
+		/* It's not a shared catalog, so refuse to move it to shared tablespace */
+		if (tableSpace == GLOBALTABLESPACE_OID)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("cannot move non-shared relation to tablespace \"%s\"",
+							get_tablespace_name(tableSpace))));
+	}
+
+	if (OidIsValid(NewIdxTablespaceOid))
+		idxtableSpace = NewIdxTablespaceOid;
+	else
+		idxtableSpace = get_rel_tablespace(indexOid);
 
 	/* Mark the correct index as clustered */
 	if (OidIsValid(indexOid))
@@ -655,7 +667,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
 	finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
 					 swap_toast_by_content, false, true,
 					 frozenXid, cutoffMulti,
-					 relpersistence);
+					 relpersistence, idxtableSpace);
 }
 
 
@@ -1403,12 +1415,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 				 bool is_internal,
 				 TransactionId frozenXid,
 				 MultiXactId cutoffMulti,
-				 char newrelpersistence)
+				 char newrelpersistence, Oid idxtableSpace)
 {
 	ObjectAddress object;
 	Oid			mapped_tables[4];
 	int			reindex_flags;
-	ReindexParams reindex_params = {0};
+	ReindexParams reindex_params = { .tablespaceOid = idxtableSpace };
 	int			i;
 
 	/* Report that we are now swapping relation files */
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index c5c25ce11d..ad1bfdc0d2 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -855,7 +855,8 @@ static void
 refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
 {
 	finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
-					 RecentXmin, ReadNextMultiXactId(), relpersistence);
+					 RecentXmin, ReadNextMultiXactId(), relpersistence,
+					 InvalidOid);
 }
 
 /*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 75b04eb161..eba8478f91 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5077,7 +5077,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
 							 !OidIsValid(tab->newTableSpace),
 							 RecentXmin,
 							 ReadNextMultiXactId(),
-							 persistence);
+							 persistence, InvalidOid);
 		}
 		else
 		{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 61565c3f14..0eb9786bea 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -109,9 +109,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 	bool		disable_page_skipping = false;
 	ListCell   *lc;
 
-	/* Name and Oid of tablespace to use for relations after VACUUM FULL. */
-	char		*tablespacename = NULL;
-	Oid 		tablespaceOid = InvalidOid;
+	/* Tablespace to use for relations after VACUUM FULL. */
+	char		*tablespacename = NULL,
+				*idxtablespacename = NULL;
 
 	/* Set default value */
 	params.index_cleanup = VACOPT_TERNARY_DEFAULT;
@@ -151,6 +151,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 			params.truncate = get_vacopt_ternary_value(opt);
 		else if (strcmp(opt->defname, "tablespace") == 0)
 			tablespacename = defGetString(opt);
+		else if (strcmp(opt->defname, "index_tablespace") == 0)
+			idxtablespacename = defGetString(opt);
 		else if (strcmp(opt->defname, "parallel") == 0)
 		{
 			if (opt->arg == NULL)
@@ -211,26 +213,18 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("VACUUM FULL cannot be performed in parallel")));
 
-	/* Get tablespace Oid to use. */
-	if (tablespacename)
-	{
-		if ((params.options & VACOPT_FULL) == 0)
-			ereport(ERROR,
-					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					errmsg("incompatible TABLESPACE option"),
-					errdetail("TABLESPACE can only be used with VACUUM FULL.")));
-
-		tablespaceOid = get_tablespace_oid(tablespacename, false);
-
-		/* Can't move a non-shared relation into pg_global */
-		if (tablespaceOid == GLOBALTABLESPACE_OID)
-			ereport(ERROR,
-					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-					errmsg("cannot move non-shared relation to tablespace \"%s\"",
-							tablespacename)));
+	if ((params.options & VACOPT_FULL) == 0 &&
+		(tablespacename || idxtablespacename))
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				errmsg("incompatible TABLESPACE option"),
+				errdetail("TABLESPACE can only be used with VACUUM FULL.")));
 
-	}
-	params.tablespace_oid = tablespaceOid;
+	/* Get tablespace Oids to use. */
+	params.tablespace_oid = tablespacename ?
+		get_tablespace_oid(tablespacename, false) : InvalidOid;
+	params.idxtablespace_oid = idxtablespacename ?
+		get_tablespace_oid(idxtablespacename, false) : InvalidOid;
 
 	/*
 	 * Make sure VACOPT_ANALYZE is specified if any column lists are present.
@@ -1964,6 +1958,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
 	{
 		ClusterParams cluster_params = {
 			.tablespaceOid = params->tablespace_oid,
+			.idxtablespaceOid = params->idxtablespace_oid,
+			/* Other params initialized to 0/false/NULL */
 		};
 
 		/* close relation before vacuuming, but hold lock until commit */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index a5d98c8c95..fd9eb75e11 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2933,6 +2933,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 		tab->at_params.is_wraparound = wraparound;
 		tab->at_params.log_min_duration = log_min_duration;
 		tab->at_params.tablespace_oid = InvalidOid;
+		tab->at_params.idxtablespace_oid = InvalidOid;
 		tab->at_vacuum_cost_limit = vac_cost_limit;
 		tab->at_vacuum_cost_delay = vac_cost_delay;
 		tab->at_relname = NULL;
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 868dea5ecc..c04a2ce20b 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -28,6 +28,7 @@ typedef struct ClusterParams
 {
 	bits32		options;		/* bitmask of CLUOPT_* */
 	Oid			tablespaceOid;	/* tablespace to rebuild relation, or InvalidOid */
+	Oid			idxtablespaceOid;	/* tablespace to rebuild relation's indexes */
 } ClusterParams;
 
 extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
@@ -45,6 +46,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 							 bool is_internal,
 							 TransactionId frozenXid,
 							 MultiXactId minMulti,
-							 char newrelpersistence);
+							 char newrelpersistence,
+							 Oid idxtablespaceOid);
 
 #endif							/* CLUSTER_H */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 0934487d4b..6c4e085a45 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,8 +227,9 @@ typedef struct VacuumParams
 	 * disabled.
 	 */
 	int			nworkers;
-	Oid			tablespace_oid; /* tablespace to use for relations
-								 * rebuilt by VACUUM FULL */
+	/* tablespaces to use for relations rebuilt by VACUUM FULL */
+	Oid			tablespace_oid;
+	Oid			idxtablespace_oid;
 } VacuumParams;
 
 /* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 2244c1d51d..9a13112605 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -80,6 +80,19 @@ REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; --
 SELECT relname FROM pg_class
 WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
 
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+
+
 -- create a schema we can use
 CREATE SCHEMA testschema;
 
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 889933a9c8..2f8ecf514d 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -114,6 +114,26 @@ WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspa
 ---------
 (0 rows)
 
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+            relname            
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+ relname 
+---------
+(0 rows)
+
 -- create a schema we can use
 CREATE SCHEMA testschema;
 -- try a table
-- 
2.17.0


--tsOsTdHNUZQcU9Ye--





^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: Adding facility for injection points (or probe points?) for more advanced tests
@ 2023-12-12 10:44  Michael Paquier <[email protected]>
  0 siblings, 2 replies; 6+ messages in thread

From: Michael Paquier @ 2023-12-12 10:44 UTC (permalink / raw)
  To: Dilip Kumar <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Postgres hackers <[email protected]>

On Tue, Dec 12, 2023 at 10:27:09AM +0530, Dilip Kumar wrote:
> Oops, I only included the code changes where I am adding injection
> points and some comments to verify that, but missed the actual test
> file. Attaching it here.

I see.  Interesting that this requires persistent connections to work.
That's something I've found clunky to rely on when the scenarios a
test needs to deal with are rather complex.  That's an area that could
be made easier to use outside of this patch..  Something got proposed
by Andrew Dunstan to make the libpq routines usable through a perl
module, for example.

> Note:  I think the latest patches are conflicting with the head, can you rebase?

Indeed, as per the recent manipulations in ipci.c for the shmem
initialization areas.  Here goes a v6.
--
Michael

From 1e11c0400bf802834792f3e9c897b17a3d14bca1 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 12 Dec 2023 11:35:24 +0100
Subject: [PATCH v6 1/4] Add backend facility for injection points

This adds a set of routines allowing developers to attach, detach and
run custom code based on arbitrary code paths set with a centralized
macro called INJECTION_POINT().  Injection points are registered in a
shared hash table.  Processes also use a local cache to over loading
callbacks more than necessary, cleaning up their cache if a callback has
found to be removed.
---
 src/include/pg_config.h.in                    |   3 +
 src/include/utils/injection_point.h           |  36 ++
 src/backend/storage/ipc/ipci.c                |   3 +
 src/backend/storage/lmgr/lwlocknames.txt      |   1 +
 .../utils/activity/wait_event_names.txt       |   1 +
 src/backend/utils/misc/Makefile               |   1 +
 src/backend/utils/misc/injection_point.c      | 317 ++++++++++++++++++
 src/backend/utils/misc/meson.build            |   1 +
 doc/src/sgml/installation.sgml                |  30 ++
 doc/src/sgml/xfunc.sgml                       |  56 ++++
 configure                                     |  34 ++
 configure.ac                                  |   7 +
 meson.build                                   |   1 +
 meson_options.txt                             |   3 +
 src/Makefile.global.in                        |   1 +
 src/tools/pgindent/typedefs.list              |   2 +
 16 files changed, 497 insertions(+)
 create mode 100644 src/include/utils/injection_point.h
 create mode 100644 src/backend/utils/misc/injection_point.c

diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 5f16918243..288bb9cb42 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -698,6 +698,9 @@
 /* Define to build with ICU support. (--with-icu) */
 #undef USE_ICU
 
+/* Define to 1 to build with injection points. (--enable-injection-points) */
+#undef USE_INJECTION_POINTS
+
 /* Define to 1 to build with LDAP support. (--with-ldap) */
 #undef USE_LDAP
 
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
new file mode 100644
index 0000000000..6335260fea
--- /dev/null
+++ b/src/include/utils/injection_point.h
@@ -0,0 +1,36 @@
+/*-------------------------------------------------------------------------
+ * injection_point.h
+ *	  Definitions related to injection points.
+ *
+ * Copyright (c) 2001-2023, PostgreSQL Global Development Group
+ *
+ * src/include/utils/injection_point.h
+ * ----------
+ */
+#ifndef INJECTION_POINT_H
+#define INJECTION_POINT_H
+
+/*
+ * Injections points require --enable-injection-points.
+ */
+#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT(name) InjectionPointRun(name)
+#else
+#define INJECTION_POINT(name) ((void) name)
+#endif
+
+/*
+ * Typedef for callback function launched by an injection point.
+ */
+typedef void (*InjectionPointCallback) (const char *name);
+
+extern Size InjectionPointShmemSize(void);
+extern void InjectionPointShmemInit(void);
+
+extern void InjectionPointAttach(const char *name,
+								 const char *library,
+								 const char *function);
+extern void InjectionPointRun(const char *name);
+extern void InjectionPointDetach(const char *name);
+
+#endif							/* INJECTION_POINT_H */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 0e0ac22bdd..81799c5688 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -49,6 +49,7 @@
 #include "storage/sinvaladt.h"
 #include "storage/spin.h"
 #include "utils/guc.h"
+#include "utils/injection_point.h"
 #include "utils/snapmgr.h"
 #include "utils/wait_event.h"
 
@@ -147,6 +148,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, AsyncShmemSize());
 	size = add_size(size, StatsShmemSize());
 	size = add_size(size, WaitEventExtensionShmemSize());
+	size = add_size(size, InjectionPointShmemSize());
 #ifdef EXEC_BACKEND
 	size = add_size(size, ShmemBackendArraySize());
 #endif
@@ -348,6 +350,7 @@ CreateOrAttachShmemStructs(void)
 	AsyncShmemInit();
 	StatsShmemInit();
 	WaitEventExtensionShmemInit();
+	InjectionPointShmemInit();
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index f72f2906ce..42a048746d 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -54,3 +54,4 @@ XactTruncationLock					44
 WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
+InjectionPointLock				49
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index d7995931bd..5631d29138 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -319,6 +319,7 @@ XactTruncation	"Waiting to execute <function>pg_xact_status</function> or update
 WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consumption."
 NotifyQueueTail	"Waiting to update limit on <command>NOTIFY</command> message storage."
 WaitEventExtension	"Waiting to read or update custom wait events information for extensions."
+InjectionPoint	"Waiting to read or update information related to injection points."
 
 XactBuffer	"Waiting for I/O on a transaction status SLRU buffer."
 CommitTsBuffer	"Waiting for I/O on a commit timestamp SLRU buffer."
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index c2971c7678..d9f59785b9 100644
--- a/src/backend/utils/misc/Makefile
+++ b/src/backend/utils/misc/Makefile
@@ -21,6 +21,7 @@ OBJS = \
 	guc_funcs.o \
 	guc_tables.o \
 	help_config.o \
+	injection_point.o \
 	pg_config.o \
 	pg_controldata.o \
 	pg_rusage.o \
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
new file mode 100644
index 0000000000..6bc349e7c7
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,317 @@
+/*-------------------------------------------------------------------------
+ *
+ * injection_point.c
+ *	  Routines to control and run injection points in the code.
+ *
+ * Injection points can be used to call arbitrary callbacks in specific
+ * places of the code, registering callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/backend/utils/misc/injection_point.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <sys/stat.h>
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "port/pg_bitutils.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/hsearch.h"
+#include "utils/injection_point.h"
+#include "utils/memutils.h"
+
+#ifdef USE_INJECTION_POINTS
+
+/*
+ * Hash table for storing injection points.
+ *
+ * InjectionPointHash is used to find an injection point by name.
+ */
+static HTAB *InjectionPointHash;	/* find points from names */
+
+/* Field sizes */
+#define INJ_NAME_MAXLEN		64
+#define INJ_LIB_MAXLEN		128
+#define INJ_FUNC_MAXLEN		128
+
+typedef struct InjectionPointEntry
+{
+	char		name[INJ_NAME_MAXLEN];	/* hash key */
+	char		library[INJ_LIB_MAXLEN];	/* library */
+	char		function[INJ_FUNC_MAXLEN];	/* function */
+} InjectionPointEntry;
+
+#define INJECTION_POINT_HASH_INIT_SIZE	16
+#define INJECTION_POINT_HASH_MAX_SIZE	128
+
+/*
+ * Local cache of injection callbacks already loaded, stored in
+ * TopMemoryContext.
+ */
+typedef struct InjectionPointArrayEntry
+{
+	char		name[INJ_NAME_MAXLEN];
+	InjectionPointCallback callback;
+} InjectionPointCacheEntry;
+
+static HTAB *InjectionPointCache = NULL;
+
+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+						  InjectionPointCallback callback)
+{
+	InjectionPointCacheEntry *entry;
+	bool		found;
+
+	/* If first time, initialize */
+	if (InjectionPointCache == NULL)
+	{
+		HASHCTL		hash_ctl;
+
+		hash_ctl.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+		hash_ctl.entrysize = sizeof(InjectionPointCacheEntry);
+		hash_ctl.hcxt = TopMemoryContext;
+
+		InjectionPointCache = hash_create("InjectionPoint cache hash",
+										  INJECTION_POINT_HASH_MAX_SIZE,
+										  &hash_ctl,
+										  HASH_ELEM | HASH_STRINGS | HASH_CONTEXT);
+	}
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+	if (!found)
+	{
+		memcpy(entry->name, name, strlen(name));
+		entry->callback = callback;
+	}
+}
+
+/*
+ * Remove entry from the local cache.  Note that this leaks a callback
+ * loaded but removed later on, which should have no consequence from
+ * a testing perspective.
+ */
+static void
+injection_point_cache_remove(const char *name)
+{
+	/* Leave if no cache */
+	if (InjectionPointCache == NULL)
+		return;
+
+	(void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
+}
+
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+	bool		found;
+	InjectionPointCacheEntry *entry;
+
+	/* no callback if no cache yet */
+	if (InjectionPointCache == NULL)
+		return NULL;
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_FIND, &found);
+
+	if (found)
+		return entry->callback;
+
+	return NULL;
+}
+#endif							/* USE_INJECTION_POINTS */
+
+/*
+ * Return the space for dynamic shared hash table.
+ */
+Size
+InjectionPointShmemSize(void)
+{
+#ifdef USE_INJECTION_POINTS
+	Size		sz = 0;
+
+	sz = add_size(sz, hash_estimate_size(INJECTION_POINT_HASH_MAX_SIZE,
+										 sizeof(InjectionPointEntry)));
+	return sz;
+#else
+	return 0;
+#endif
+}
+
+/*
+ * Allocate shmem space for dynamic shared hash.
+ */
+void
+InjectionPointShmemInit(void)
+{
+#ifdef USE_INJECTION_POINTS
+	HASHCTL		info;
+
+	/* key is a NULL-terminated string */
+	info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+	info.entrysize = sizeof(InjectionPointEntry);
+	InjectionPointHash = ShmemInitHash("InjectionPoint hash",
+									   INJECTION_POINT_HASH_INIT_SIZE,
+									   INJECTION_POINT_HASH_MAX_SIZE,
+									   &info,
+									   HASH_ELEM | HASH_STRINGS);
+#endif
+}
+
+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)
+{
+	struct stat st;
+
+	Assert(name != NULL);
+	if (stat(name, &st) == 0)
+		return !S_ISDIR(st.st_mode);
+	else if (!(errno == ENOENT || errno == ENOTDIR))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not access file \"%s\": %m", name)));
+	return false;
+}
+#endif
+
+/*
+ * Attach a new injection point.
+ */
+void
+InjectionPointAttach(const char *name,
+					 const char *library,
+					 const char *function)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+
+	if (strlen(name) >= INJ_NAME_MAXLEN)
+		elog(ERROR, "injection point name %s too long", name);
+	if (strlen(library) >= INJ_LIB_MAXLEN)
+		elog(ERROR, "injection point library %s too long", library);
+	if (strlen(function) >= INJ_FUNC_MAXLEN)
+		elog(ERROR, "injection point function %s too long", function);
+
+	/*
+	 * Allocate and register a new injection point.  A new point should not
+	 * exist.  For testing purposes this should be fine.
+	 */
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_ENTER, &found);
+	if (found)
+	{
+		LWLockRelease(InjectionPointLock);
+		elog(ERROR, "injection point \"%s\" already defined", name);
+	}
+
+	/* Save the entry */
+	memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+	entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+	entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+	entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+
+	LWLockRelease(InjectionPointLock);
+
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Detach an existing injection point.
+ */
+void
+InjectionPointDetach(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	bool		found;
+
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	hash_search(InjectionPointHash, name, HASH_REMOVE, &found);
+	LWLockRelease(InjectionPointLock);
+
+	if (!found)
+		elog(ERROR, "injection point \"%s\" not found", name);
+
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Execute an injection point, if defined.
+ *
+ * Check first the shared hash table, and adapt the local cache
+ * depending on that as it could be possible that an entry to run
+ * has been removed.
+ */
+void
+InjectionPointRun(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+	InjectionPointCallback injection_callback;
+
+	LWLockAcquire(InjectionPointLock, LW_SHARED);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_FIND, &found);
+	LWLockRelease(InjectionPointLock);
+
+	/*
+	 * If not found, do nothing and remove it from the local cache if it
+	 * existed there.
+	 */
+	if (!found)
+	{
+		injection_point_cache_remove(name);
+		return;
+	}
+
+	/*
+	 * Check if the callback exists in the local cache, to avoid unnecessary
+	 * external loads.
+	 */
+	injection_callback = injection_point_cache_get(name);
+	if (injection_callback == NULL)
+	{
+		char		path[MAXPGPATH];
+
+		/* Found, so just run the callback registered */
+		snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+				 entry_by_name->library, DLSUFFIX);
+
+		if (!file_exists(path))
+			elog(ERROR, "could not find injection library \"%s\"", path);
+
+		injection_callback = (InjectionPointCallback)
+			load_external_function(path, entry_by_name->function, true, NULL);
+
+		/* add it to the local cache when found */
+		injection_point_cache_add(name, injection_callback);
+	}
+
+	injection_callback(name);
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/backend/utils/misc/meson.build b/src/backend/utils/misc/meson.build
index f719c97c05..1438859b69 100644
--- a/src/backend/utils/misc/meson.build
+++ b/src/backend/utils/misc/meson.build
@@ -6,6 +6,7 @@ backend_sources += files(
   'guc_funcs.c',
   'guc_tables.c',
   'help_config.c',
+  'injection_point.c',
   'pg_config.c',
   'pg_controldata.c',
   'pg_rusage.c',
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index b23b35cd8e..d5a2fcb084 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1676,6 +1676,21 @@ build-postgresql:
        </listitem>
       </varlistentry>
 
+      <varlistentry id="configure-option-enable-injection-points">
+       <term><option>--enable-injection-points</option></term>
+       <listitem>
+        <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+        </para>
+       </listitem>
+      </varlistentry>
+
       <varlistentry id="configure-option-with-segsize-blocks">
        <term><option>--with-segsize-blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
@@ -3184,6 +3199,21 @@ ninja install
       </listitem>
      </varlistentry>
 
+     <varlistentry id="configure-injection-points-meson">
+      <term><option>-Dinjection_points={ true | false }</option></term>
+      <listitem>
+       <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+       </para>
+      </listitem>
+     </varlistentry>
+
       <varlistentry id="configure-segsize-blocks-meson">
        <term><option>-Dsegsize_blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 89116ae74c..66cc94b03b 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3510,6 +3510,62 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
     </para>
    </sect2>
 
+   <sect2 id="xfunc-addin-injection-points">
+    <title>Injection Points</title>
+
+    <para>
+     Add-ins can define injection points, that can register callbacks
+     to run user-defined code when going through a specific code path,
+     by calling:
+<programlisting>
+extern void InjectionPointAttach(const char *name,
+                                 const char *library,
+                                 const char *function);
+</programlisting>
+
+     <literal>name</literal> is the name of the injection point, that
+     will execute the <literal>function</literal> loaded from
+     <literal>library</library>.
+     Injection points are saved in a hash table in shared memory, and
+     last until the server is shut down.
+    </para>
+
+    <para>
+     Here is an example of callback for
+     <literal>InjectionPointCallback</literal>:
+<programlisting>
+static void
+custom_injection_callback(const char *name)
+{
+    elog(NOTICE, "%s: executed custom callback", name);
+}
+</programlisting>
+    </para>
+
+    <para>
+     Once an injection point is defined, running it requires to use
+     the following macro to trigger the callback given in a wanted code
+     path:
+<programlisting>
+INJECTION_POINT(name);
+</programlisting>
+    </para>
+
+    <para>
+     Optionally, it is possible to detach injection points by calling:
+<programlisting>
+extern void InjectionPointDetach(const char *name);
+</programlisting>
+    </para>
+
+    <para>
+     Enabling injections points requires
+     <option>--enable-injection-points</option> from
+     <command>configure</command> or <option>-Dinjection_points=true</option>
+     from <application>Meson</application>.
+    </para>
+   </sect2>
+
    <sect2 id="extend-cpp">
     <title>Using C++ for Extensibility</title>
 
diff --git a/configure b/configure
index 217704e9ca..fbac8dd23f 100755
--- a/configure
+++ b/configure
@@ -759,6 +759,7 @@ CPPFLAGS
 LDFLAGS
 CFLAGS
 CC
+enable_injection_points
 enable_tap_tests
 enable_dtrace
 DTRACEFLAGS
@@ -839,6 +840,7 @@ enable_profiling
 enable_coverage
 enable_dtrace
 enable_tap_tests
+enable_injection_points
 with_blocksize
 with_segsize
 with_segsize_blocks
@@ -1532,6 +1534,8 @@ Optional Features:
   --enable-coverage       build with coverage testing instrumentation
   --enable-dtrace         build with DTrace support
   --enable-tap-tests      enable TAP tests (requires Perl and IPC::Run)
+  --enable-injection-points
+                          enable injection points (for testing)
   --enable-depend         turn on automatic dependency tracking
   --enable-cassert        enable assertion checks (for debugging)
   --disable-largefile     omit support for large files
@@ -3682,6 +3686,36 @@ fi
 
 
 
+#
+# Injection points
+#
+
+
+# Check whether --enable-injection-points was given.
+if test "${enable_injection_points+set}" = set; then :
+  enableval=$enable_injection_points;
+  case $enableval in
+    yes)
+
+$as_echo "#define USE_INJECTION_POINTS 1" >>confdefs.h
+
+      ;;
+    no)
+      :
+      ;;
+    *)
+      as_fn_error $? "no argument expected for --enable-injection-points option" "$LINENO" 5
+      ;;
+  esac
+
+else
+  enable_injection_points=no
+
+fi
+
+
+
+
 #
 # Block size
 #
diff --git a/configure.ac b/configure.ac
index e49de9e4f0..1507de55b7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,13 @@ PGAC_ARG_BOOL(enable, tap-tests, no,
               [enable TAP tests (requires Perl and IPC::Run)])
 AC_SUBST(enable_tap_tests)
 
+#
+# Injection points
+#
+PGAC_ARG_BOOL(enable, injection-points, no, [enable injection points (for testing)],
+              [AC_DEFINE([USE_INJECTION_POINTS], 1, [Define to 1 to build with injection points. (--enable-injection-points)])])
+AC_SUBST(enable_injection_points)
+
 #
 # Block size
 #
diff --git a/meson.build b/meson.build
index 52c2a37c41..cae865f640 100644
--- a/meson.build
+++ b/meson.build
@@ -431,6 +431,7 @@ meson_bin = find_program(meson_binpath, native: true)
 ###############################################################
 
 cdata.set('USE_ASSERT_CHECKING', get_option('cassert') ? 1 : false)
+cdata.set('USE_INJECTION_POINTS', get_option('injection_points') ? 1 : false)
 
 blocksize = get_option('blocksize').to_int() * 1024
 
diff --git a/meson_options.txt b/meson_options.txt
index be1b327f54..7a5102df1a 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,9 @@ option('cassert', type: 'boolean', value: false,
 option('tap_tests', type: 'feature', value: 'auto',
   description: 'Enable TAP tests')
 
+option('injection_points', type: 'boolean', value: false,
+  description: 'Enable injection points')
+
 option('PG_TEST_EXTRA', type: 'string', value: '',
   description: 'Enable selected extra tests')
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 104e5de0fe..7c7fd77b01 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -203,6 +203,7 @@ enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
 enable_dtrace	= @enable_dtrace@
 enable_coverage	= @enable_coverage@
+enable_injection_points = @enable_injection_points@
 enable_tap_tests	= @enable_tap_tests@
 
 python_includespec	= @python_includespec@
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ba41149b88..4e495d45fa 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,8 @@ IdentLine
 IdentifierLookup
 IdentifySystemCmd
 IfStackElem
+InjectionPointCacheEntry
+InjectionPointEntry
 ImportForeignSchemaStmt
 ImportForeignSchemaType
 ImportForeignSchema_function
-- 
2.43.0


From 1d1d27e9b696a01d3468acf51d13aca92b629e59 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 12 Dec 2023 11:40:50 +0100
Subject: [PATCH v6 2/4] Add test module test_injection_points

This is a test facility aimed at providing basic coverage for the code
routines of injection points.  This will be extended with more tests.
---
 src/test/modules/Makefile                     |   7 ++
 src/test/modules/meson.build                  |   1 +
 .../modules/test_injection_points/.gitignore  |   4 +
 .../modules/test_injection_points/Makefile    |  22 ++++
 .../expected/test_injection_points.out        | 117 ++++++++++++++++++
 .../modules/test_injection_points/meson.build |  37 ++++++
 .../sql/test_injection_points.sql             |  33 +++++
 .../test_injection_points--1.0.sql            |  36 ++++++
 .../test_injection_points.c                   |  91 ++++++++++++++
 .../test_injection_points.control             |   4 +
 10 files changed, 352 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/.gitignore
 create mode 100644 src/test/modules/test_injection_points/Makefile
 create mode 100644 src/test/modules/test_injection_points/expected/test_injection_points.out
 create mode 100644 src/test/modules/test_injection_points/meson.build
 create mode 100644 src/test/modules/test_injection_points/sql/test_injection_points.sql
 create mode 100644 src/test/modules/test_injection_points/test_injection_points--1.0.sql
 create mode 100644 src/test/modules/test_injection_points/test_injection_points.c
 create mode 100644 src/test/modules/test_injection_points/test_injection_points.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..9a55a6924e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -37,6 +37,13 @@ SUBDIRS = \
 		  worker_spi \
 		  xid_wraparound
 
+
+ifeq ($(enable_injection_points),yes)
+SUBDIRS += test_injection_points
+else
+ALWAYS_SUBDIRS += test_injection_points
+endif
+
 ifeq ($(with_ssl),openssl)
 SUBDIRS += ssl_passphrase_callback
 else
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index b76f588559..dc0048a6e4 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -17,6 +17,7 @@ subdir('test_ddl_deparse')
 subdir('test_dsa')
 subdir('test_extensions')
 subdir('test_ginpostinglist')
+subdir('test_injection_points')
 subdir('test_integerset')
 subdir('test_lfind')
 subdir('test_misc')
diff --git a/src/test/modules/test_injection_points/.gitignore b/src/test/modules/test_injection_points/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_injection_points/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
new file mode 100644
index 0000000000..65bcdde782
--- /dev/null
+++ b/src/test/modules/test_injection_points/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/test_injection_points/Makefile
+
+MODULE_big = test_injection_points
+OBJS = \
+	$(WIN32RES) \
+	test_injection_points.o
+PGFILEDESC = "test_injection_points - test injection points"
+
+EXTENSION = test_injection_points
+DATA = test_injection_points--1.0.sql
+REGRESS = test_injection_points
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_injection_points
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_injection_points/expected/test_injection_points.out b/src/test/modules/test_injection_points/expected/test_injection_points.out
new file mode 100644
index 0000000000..a8ddae0aad
--- /dev/null
+++ b/src/test/modules/test_injection_points/expected/test_injection_points.out
@@ -0,0 +1,117 @@
+CREATE EXTENSION test_injection_points;
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+ERROR:  incorrect mode "booh" for injection point creation
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+ test_injection_points_detach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+ test_injection_points_detach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+ERROR:  injection point "TestInjectionLog" not found
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
new file mode 100644
index 0000000000..7509a102ef
--- /dev/null
+++ b/src/test/modules/test_injection_points/meson.build
@@ -0,0 +1,37 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+  subdir_done()
+endif
+
+test_injection_points_sources = files(
+  'test_injection_points.c',
+)
+
+if host_system == 'windows'
+  test_injection_points_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_injection_points',
+    '--FILEDESC', 'test_injection_points - test injection points',])
+endif
+
+test_injection_points = shared_module('test_injection_points',
+  test_injection_points_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_injection_points
+
+test_install_data += files(
+  'test_injection_points.control',
+  'test_injection_points--1.0.sql',
+)
+
+tests += {
+  'name': 'test_injection_points',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'test_injection_points',
+    ],
+  },
+}
diff --git a/src/test/modules/test_injection_points/sql/test_injection_points.sql b/src/test/modules/test_injection_points/sql/test_injection_points.sql
new file mode 100644
index 0000000000..8f23f4c044
--- /dev/null
+++ b/src/test/modules/test_injection_points/sql/test_injection_points.sql
@@ -0,0 +1,33 @@
+CREATE EXTENSION test_injection_points;
+
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
new file mode 100644
index 0000000000..1c0a689ae2
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -0,0 +1,36 @@
+/* src/test/modules/test_injection_points/test_injection_points--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_injection_points" to load this file. \quit
+
+--
+-- test_injection_points_attach()
+--
+-- Attaches an injection point using callbacks from one of the predefined
+-- modes.
+--
+CREATE FUNCTION test_injection_points_attach(IN point_name TEXT,
+    IN mode text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_attach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_run()
+--
+-- Executes an injection point.
+--
+CREATE FUNCTION test_injection_points_run(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_run'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_detach()
+--
+-- Detaches an injection point.
+--
+CREATE FUNCTION test_injection_points_detach(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_detach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
new file mode 100644
index 0000000000..efb2c74c47
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -0,0 +1,91 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_injection_points.c
+ *		Code for testing injection points.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_injection_points/test_injection_points.c
+ *
+ * Injection points are able to trigger user-defined callbacks in pre-defined
+ * code paths.
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "utils/builtins.h"
+#include "utils/injection_point.h"
+
+PG_MODULE_MAGIC;
+
+extern PGDLLEXPORT void test_injection_error(const char *name);
+extern PGDLLEXPORT void test_injection_notice(const char *name);
+
+/* Set of callbacks available at point creation */
+void
+test_injection_error(const char *name)
+{
+	elog(ERROR, "error triggered for injection point %s", name);
+}
+
+void
+test_injection_notice(const char *name)
+{
+	elog(NOTICE, "notice triggered for injection point %s", name);
+}
+
+/*
+ * SQL function for creating an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_attach);
+Datum
+test_injection_points_attach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *mode = text_to_cstring(PG_GETARG_TEXT_PP(1));
+	char	   *function;
+
+	if (strcmp(mode, "error") == 0)
+		function = "test_injection_error";
+	else if (strcmp(mode, "notice") == 0)
+		function = "test_injection_notice";
+	else
+		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+
+	InjectionPointAttach(name, "test_injection_points", function);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for triggering an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_run);
+Datum
+test_injection_points_run(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	INJECTION_POINT(name);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for dropping an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_detach);
+Datum
+test_injection_points_detach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	InjectionPointDetach(name);
+
+	PG_RETURN_VOID();
+}
diff --git a/src/test/modules/test_injection_points/test_injection_points.control b/src/test/modules/test_injection_points/test_injection_points.control
new file mode 100644
index 0000000000..a13657cfc6
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.control
@@ -0,0 +1,4 @@
+comment = 'Test code for injection points'
+default_version = '1.0'
+module_pathname = '$libdir/test_injection_points'
+relocatable = true
-- 
2.43.0


From aaf4b21dffcabd55f7dd5fb9d1f35f13ba434153 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 16 Nov 2023 14:28:22 +0900
Subject: [PATCH v6 3/4] Add regression test to show snapbuild consistency

Reverting 409f9ca44713 causes the test to fail.  The test added here
relies on the existing callbacks in test_injection_points.
---
 src/backend/replication/logical/snapbuild.c   |  3 ++
 .../modules/test_injection_points/Makefile    |  2 +
 .../modules/test_injection_points/meson.build |  5 ++
 .../t/001_snapshot_status.pl                  | 47 +++++++++++++++++++
 4 files changed, 57 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/t/001_snapshot_status.pl

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index fec190a8b2..3491e5a872 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
 #include "storage/procarray.h"
 #include "storage/standby.h"
 #include "utils/builtins.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/snapshot.h"
@@ -654,6 +655,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	snap->xcnt = newxcnt;
 	snap->xip = newxip;
 
+	INJECTION_POINT("SnapBuildInitialSnapshot");
+
 	return snap;
 }
 
diff --git a/src/test/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
index 65bcdde782..4696c1b013 100644
--- a/src/test/modules/test_injection_points/Makefile
+++ b/src/test/modules/test_injection_points/Makefile
@@ -10,6 +10,8 @@ EXTENSION = test_injection_points
 DATA = test_injection_points--1.0.sql
 REGRESS = test_injection_points
 
+TAP_TESTS = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 7509a102ef..6006b38f3d 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -34,4 +34,9 @@ tests += {
       'test_injection_points',
     ],
   },
+  'tap': {
+    'tests': [
+      't/001_snapshot_status.pl',
+    ],
+  }
 }
diff --git a/src/test/modules/test_injection_points/t/001_snapshot_status.pl b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
new file mode 100644
index 0000000000..ca5c6cc7a4
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
@@ -0,0 +1,47 @@
+# Test consistent of initial snapshot data.
+
+# This requires a node with wal_level=logical combined with an injection
+# point that forces a failure when a snapshot is initially built with a
+# logical slot created.
+#
+# See bug https://postgr.es/m/CAFiTN-s0zA1Kj0ozGHwkYkHwa5U0zUE94RSc_g81WrpcETB5=w@mail.gmail.com.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION test_injection_points;');
+$node->safe_psql('postgres',
+  "SELECT test_injection_points_attach('SnapBuildInitialSnapshot', 'error');");
+
+my $node_host = $node->host;
+my $node_port = $node->port;
+my $connstr_common = "host=$node_host port=$node_port";
+my $connstr_db = "$connstr_common replication=database dbname=postgres";
+
+# This requires a single session, with two commands.
+my $psql_session =
+  $node->background_psql('postgres', on_error_stop => 0,
+			 extra_params => [ '-d', $connstr_db ]);
+my ($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok($ret != 0, "First CREATE_REPLICATION_SLOT fails on injected error");
+
+# Now remove the injected error and check that the second command works.
+$node->safe_psql('postgres',
+  "SELECT test_injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+print "BOO" . substr($output, 0, 4) . "\n";
+ok(substr($output, 0, 4) eq 'slot',
+   "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
-- 
2.43.0


From f184ace1b21c70c09b41bc54ebeb5f341098a96c Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 16 Nov 2023 14:42:31 +0900
Subject: [PATCH v6 4/4] Add basic test for promotion and restart race
 condition

This test fails after 7863ee4def65 is reverted.  test_injection_points
is extended so as it is possible to add condition variables to wait for
in the point callbacks, with a SQL function to broadcast condition
variables that may be sleeping.

I guess that this should be extended so as there is more than one
condition variable stored in shmem for this module, controlling which
variable to wait for directly in the callback itself, but that's not
really necessary now.
---
 src/backend/access/transam/xlog.c             |   7 +
 .../modules/test_injection_points/meson.build |   1 +
 .../t/002_invalid_checkpoint_after_promote.pl | 132 ++++++++++++++++++
 .../test_injection_points--1.0.sql            |  10 ++
 .../test_injection_points.c                   |  73 ++++++++++
 5 files changed, 223 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 01e0484584..ece31bb2a6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -99,6 +99,7 @@
 #include "storage/sync.h"
 #include "utils/guc_hooks.h"
 #include "utils/guc_tables.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
 #include "utils/relmapper.h"
@@ -7345,6 +7346,12 @@ CreateRestartPoint(int flags)
 
 	CheckPointGuts(lastCheckPoint.redo, flags);
 
+	/*
+	 * This location is important to be after CheckPointGuts() to ensure
+	 * that some work has happened.
+	 */
+	INJECTION_POINT("CreateRestartPoint");
+
 	/*
 	 * Remember the prior checkpoint's redo ptr for
 	 * UpdateCheckPointDistanceEstimate()
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 6006b38f3d..6ebdc728b7 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -37,6 +37,7 @@ tests += {
   'tap': {
     'tests': [
       't/001_snapshot_status.pl',
+      't/002_invalid_checkpoint_after_promote.pl',
     ],
   }
 }
diff --git a/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..2da243e871
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,132 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep nanosleep);
+use Test::More;
+
+# initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('master');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+	'postgresql.conf', q[
+checkpoint_timeout = 30s
+log_checkpoints = on
+restart_after_crash = on
+]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# setup a standby
+my $node_standby = PostgreSQL::Test::Cluster->new('standby1');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# dummy table for the upcoming tests.
+$node_primary->safe_psql('postgres', 'checkpoint');
+$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);');
+
+# Register a injection point on the standby so as the follow-up
+# restart point running on it will wait.
+$node_primary->safe_psql('postgres', 'CREATE EXTENSION test_injection_points;');
+# Wait until the extension has been created on the standby
+$node_primary->wait_for_replay_catchup($node_standby);
+# This causes a restartpoint to wait on a standby.
+$node_standby->safe_psql('postgres',
+  "SELECT test_injection_points_attach('CreateRestartPoint', 'wait');");
+
+# Execute a restart point on the standby, that will be waited on.
+# This needs to be in the background as we'll wait on it.
+my $logstart = -s $node_standby->logfile;
+my $psql_session =
+  $node_standby->background_psql('postgres', on_error_stop => 0);
+$psql_session->query_until(qr/starting_checkpoint/, q(
+   \echo starting_checkpoint
+   CHECKPOINT;
+));
+
+# Switch one WAL segment to make the restartpoint remove it.
+$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);');
+$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();');
+$node_primary->wait_for_replay_catchup($node_standby);
+
+# Wait until the checkpointer is in the middle of the restartpoint
+# processing.
+ok( $node_standby->poll_query_until(
+	'postgres',
+	qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'test_injection_wait' ;],
+	'1'),
+    'checkpointer is waiting at restart point'
+    ) or die "Timed out while waiting for checkpointer to run restartpoint";
+
+
+# Restartpoint should have started on standby.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+	$checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
+
+# promote during restartpoint
+$node_primary->stop;
+$node_standby->promote;
+
+# Update the start position before waking up the checkpointer!
+$logstart = -s $node_standby->logfile;
+
+# Now wake up the checkpointer
+$node_standby->safe_psql('postgres',
+  "SELECT test_injection_points_wake();");
+
+# wait until checkpoint completes on the newly-promoted standby.
+my $checkpoint_complete = 0;
+for (my $i = 0; $i < 3000; $i++)
+{
+	my $log = slurp_file($node_standby->logfile, $logstart);
+	if ($log =~ m/restartpoint complete/)
+	{
+		$checkpoint_complete = 1;
+		last;
+	}
+	usleep(100_000);
+}
+is($checkpoint_complete, 1, 'restartpoint has completed');
+
+# kill SIGKILL a backend, and all backend will restart. Note that previous checkpoint has not completed.
+my $psql_timeout = IPC::Run::timer(3600);
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, 'killed process with KILL');
+my $stdout;
+my $stderr;
+
+# after recovery, the server will not start, and log PANIC: could not locate a valid checkpoint record
+for (my $i = 0; $i < 30; $i++)
+{
+    ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1');
+    last if $ret == 0;
+	sleep(1);
+}
+is($ret, 0, "psql connect success");
+is($stdout, 1, "psql select 1");
+
+done_testing();
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
index 1c0a689ae2..05f97f0982 100644
--- a/src/test/modules/test_injection_points/test_injection_points--1.0.sql
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -25,6 +25,16 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'test_injection_points_run'
 LANGUAGE C STRICT PARALLEL UNSAFE;
 
+--
+-- test_injection_points_wake()
+--
+-- Wakes a condition variable executed in an injection point.
+--
+CREATE FUNCTION test_injection_points_wake()
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_wake'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
 --
 -- test_injection_points_detach()
 --
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
index efb2c74c47..8b837d85d3 100644
--- a/src/test/modules/test_injection_points/test_injection_points.c
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -18,13 +18,56 @@
 #include "postgres.h"
 
 #include "fmgr.h"
+#include "storage/condition_variable.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
 #include "utils/builtins.h"
 #include "utils/injection_point.h"
+#include "utils/wait_event.h"
 
 PG_MODULE_MAGIC;
 
+/* Shared state information for injection points. */
+typedef struct TestInjectionPointSharedState
+{
+	/*
+	 * Wait variable that can be registered at a given point, and that can be
+	 * awakened via SQL.
+	 */
+	ConditionVariable	wait_point;
+} TestInjectionPointSharedState;
+
+/* Pointer to shared-memory state. */
+static TestInjectionPointSharedState *inj_state = NULL;
+
+/* Wait event when waiting on condition variable */
+static uint32 test_injection_wait_event = 0;
+
 extern PGDLLEXPORT void test_injection_error(const char *name);
 extern PGDLLEXPORT void test_injection_notice(const char *name);
+extern PGDLLEXPORT void test_injection_wait(const char *name);
+
+
+static void
+test_injection_init_shmem(void)
+{
+	bool		found;
+
+	if (inj_state != NULL)
+		return;
+
+	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+	inj_state = ShmemInitStruct("test_injection_points",
+								sizeof(TestInjectionPointSharedState),
+								&found);
+	if (!found)
+	{
+		/* First time through ... */
+		MemSet(inj_state, 0, sizeof(TestInjectionPointSharedState));
+		ConditionVariableInit(&inj_state->wait_point);
+	}
+	LWLockRelease(AddinShmemInitLock);
+}
 
 /* Set of callbacks available at point creation */
 void
@@ -39,6 +82,20 @@ test_injection_notice(const char *name)
 	elog(NOTICE, "notice triggered for injection point %s", name);
 }
 
+void
+test_injection_wait(const char *name)
+{
+	if (inj_state == NULL)
+		test_injection_init_shmem();
+	if (test_injection_wait_event == 0)
+		test_injection_wait_event = WaitEventExtensionNew("test_injection_wait");
+
+	/* And sleep.. */
+	ConditionVariablePrepareToSleep(&inj_state->wait_point);
+	ConditionVariableSleep(&inj_state->wait_point, test_injection_wait_event);
+	ConditionVariableCancelSleep();
+}
+
 /*
  * SQL function for creating an injection point.
  */
@@ -54,6 +111,8 @@ test_injection_points_attach(PG_FUNCTION_ARGS)
 		function = "test_injection_error";
 	else if (strcmp(mode, "notice") == 0)
 		function = "test_injection_notice";
+	else if (strcmp(mode, "wait") == 0)
+		function = "test_injection_wait";
 	else
 		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
 
@@ -76,6 +135,20 @@ test_injection_points_run(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_wake);
+Datum
+test_injection_points_wake(PG_FUNCTION_ARGS)
+{
+	if (inj_state == NULL)
+		test_injection_init_shmem();
+
+	ConditionVariableBroadcast(&inj_state->wait_point);
+	PG_RETURN_VOID();
+}
+
 /*
  * SQL function for dropping an injection point.
  */
-- 
2.43.0



Attachments:

  [text/plain] v6-0001-Add-backend-facility-for-injection-points.patch (22.1K, ../../[email protected]/2-v6-0001-Add-backend-facility-for-injection-points.patch)
  download | inline diff:
From 1e11c0400bf802834792f3e9c897b17a3d14bca1 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 12 Dec 2023 11:35:24 +0100
Subject: [PATCH v6 1/4] Add backend facility for injection points

This adds a set of routines allowing developers to attach, detach and
run custom code based on arbitrary code paths set with a centralized
macro called INJECTION_POINT().  Injection points are registered in a
shared hash table.  Processes also use a local cache to over loading
callbacks more than necessary, cleaning up their cache if a callback has
found to be removed.
---
 src/include/pg_config.h.in                    |   3 +
 src/include/utils/injection_point.h           |  36 ++
 src/backend/storage/ipc/ipci.c                |   3 +
 src/backend/storage/lmgr/lwlocknames.txt      |   1 +
 .../utils/activity/wait_event_names.txt       |   1 +
 src/backend/utils/misc/Makefile               |   1 +
 src/backend/utils/misc/injection_point.c      | 317 ++++++++++++++++++
 src/backend/utils/misc/meson.build            |   1 +
 doc/src/sgml/installation.sgml                |  30 ++
 doc/src/sgml/xfunc.sgml                       |  56 ++++
 configure                                     |  34 ++
 configure.ac                                  |   7 +
 meson.build                                   |   1 +
 meson_options.txt                             |   3 +
 src/Makefile.global.in                        |   1 +
 src/tools/pgindent/typedefs.list              |   2 +
 16 files changed, 497 insertions(+)
 create mode 100644 src/include/utils/injection_point.h
 create mode 100644 src/backend/utils/misc/injection_point.c

diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 5f16918243..288bb9cb42 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -698,6 +698,9 @@
 /* Define to build with ICU support. (--with-icu) */
 #undef USE_ICU
 
+/* Define to 1 to build with injection points. (--enable-injection-points) */
+#undef USE_INJECTION_POINTS
+
 /* Define to 1 to build with LDAP support. (--with-ldap) */
 #undef USE_LDAP
 
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
new file mode 100644
index 0000000000..6335260fea
--- /dev/null
+++ b/src/include/utils/injection_point.h
@@ -0,0 +1,36 @@
+/*-------------------------------------------------------------------------
+ * injection_point.h
+ *	  Definitions related to injection points.
+ *
+ * Copyright (c) 2001-2023, PostgreSQL Global Development Group
+ *
+ * src/include/utils/injection_point.h
+ * ----------
+ */
+#ifndef INJECTION_POINT_H
+#define INJECTION_POINT_H
+
+/*
+ * Injections points require --enable-injection-points.
+ */
+#ifdef USE_INJECTION_POINTS
+#define INJECTION_POINT(name) InjectionPointRun(name)
+#else
+#define INJECTION_POINT(name) ((void) name)
+#endif
+
+/*
+ * Typedef for callback function launched by an injection point.
+ */
+typedef void (*InjectionPointCallback) (const char *name);
+
+extern Size InjectionPointShmemSize(void);
+extern void InjectionPointShmemInit(void);
+
+extern void InjectionPointAttach(const char *name,
+								 const char *library,
+								 const char *function);
+extern void InjectionPointRun(const char *name);
+extern void InjectionPointDetach(const char *name);
+
+#endif							/* INJECTION_POINT_H */
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 0e0ac22bdd..81799c5688 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -49,6 +49,7 @@
 #include "storage/sinvaladt.h"
 #include "storage/spin.h"
 #include "utils/guc.h"
+#include "utils/injection_point.h"
 #include "utils/snapmgr.h"
 #include "utils/wait_event.h"
 
@@ -147,6 +148,7 @@ CalculateShmemSize(int *num_semaphores)
 	size = add_size(size, AsyncShmemSize());
 	size = add_size(size, StatsShmemSize());
 	size = add_size(size, WaitEventExtensionShmemSize());
+	size = add_size(size, InjectionPointShmemSize());
 #ifdef EXEC_BACKEND
 	size = add_size(size, ShmemBackendArraySize());
 #endif
@@ -348,6 +350,7 @@ CreateOrAttachShmemStructs(void)
 	AsyncShmemInit();
 	StatsShmemInit();
 	WaitEventExtensionShmemInit();
+	InjectionPointShmemInit();
 }
 
 /*
diff --git a/src/backend/storage/lmgr/lwlocknames.txt b/src/backend/storage/lmgr/lwlocknames.txt
index f72f2906ce..42a048746d 100644
--- a/src/backend/storage/lmgr/lwlocknames.txt
+++ b/src/backend/storage/lmgr/lwlocknames.txt
@@ -54,3 +54,4 @@ XactTruncationLock					44
 WrapLimitsVacuumLock				46
 NotifyQueueTailLock					47
 WaitEventExtensionLock				48
+InjectionPointLock				49
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index d7995931bd..5631d29138 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -319,6 +319,7 @@ XactTruncation	"Waiting to execute <function>pg_xact_status</function> or update
 WrapLimitsVacuum	"Waiting to update limits on transaction id and multixact consumption."
 NotifyQueueTail	"Waiting to update limit on <command>NOTIFY</command> message storage."
 WaitEventExtension	"Waiting to read or update custom wait events information for extensions."
+InjectionPoint	"Waiting to read or update information related to injection points."
 
 XactBuffer	"Waiting for I/O on a transaction status SLRU buffer."
 CommitTsBuffer	"Waiting for I/O on a commit timestamp SLRU buffer."
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index c2971c7678..d9f59785b9 100644
--- a/src/backend/utils/misc/Makefile
+++ b/src/backend/utils/misc/Makefile
@@ -21,6 +21,7 @@ OBJS = \
 	guc_funcs.o \
 	guc_tables.o \
 	help_config.o \
+	injection_point.o \
 	pg_config.o \
 	pg_controldata.o \
 	pg_rusage.o \
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
new file mode 100644
index 0000000000..6bc349e7c7
--- /dev/null
+++ b/src/backend/utils/misc/injection_point.c
@@ -0,0 +1,317 @@
+/*-------------------------------------------------------------------------
+ *
+ * injection_point.c
+ *	  Routines to control and run injection points in the code.
+ *
+ * Injection points can be used to call arbitrary callbacks in specific
+ * places of the code, registering callbacks that would be run in the code
+ * paths where a named injection point exists.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ *	  src/backend/utils/misc/injection_point.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <sys/stat.h>
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "port/pg_bitutils.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
+#include "utils/hsearch.h"
+#include "utils/injection_point.h"
+#include "utils/memutils.h"
+
+#ifdef USE_INJECTION_POINTS
+
+/*
+ * Hash table for storing injection points.
+ *
+ * InjectionPointHash is used to find an injection point by name.
+ */
+static HTAB *InjectionPointHash;	/* find points from names */
+
+/* Field sizes */
+#define INJ_NAME_MAXLEN		64
+#define INJ_LIB_MAXLEN		128
+#define INJ_FUNC_MAXLEN		128
+
+typedef struct InjectionPointEntry
+{
+	char		name[INJ_NAME_MAXLEN];	/* hash key */
+	char		library[INJ_LIB_MAXLEN];	/* library */
+	char		function[INJ_FUNC_MAXLEN];	/* function */
+} InjectionPointEntry;
+
+#define INJECTION_POINT_HASH_INIT_SIZE	16
+#define INJECTION_POINT_HASH_MAX_SIZE	128
+
+/*
+ * Local cache of injection callbacks already loaded, stored in
+ * TopMemoryContext.
+ */
+typedef struct InjectionPointArrayEntry
+{
+	char		name[INJ_NAME_MAXLEN];
+	InjectionPointCallback callback;
+} InjectionPointCacheEntry;
+
+static HTAB *InjectionPointCache = NULL;
+
+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+						  InjectionPointCallback callback)
+{
+	InjectionPointCacheEntry *entry;
+	bool		found;
+
+	/* If first time, initialize */
+	if (InjectionPointCache == NULL)
+	{
+		HASHCTL		hash_ctl;
+
+		hash_ctl.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+		hash_ctl.entrysize = sizeof(InjectionPointCacheEntry);
+		hash_ctl.hcxt = TopMemoryContext;
+
+		InjectionPointCache = hash_create("InjectionPoint cache hash",
+										  INJECTION_POINT_HASH_MAX_SIZE,
+										  &hash_ctl,
+										  HASH_ELEM | HASH_STRINGS | HASH_CONTEXT);
+	}
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+	if (!found)
+	{
+		memcpy(entry->name, name, strlen(name));
+		entry->callback = callback;
+	}
+}
+
+/*
+ * Remove entry from the local cache.  Note that this leaks a callback
+ * loaded but removed later on, which should have no consequence from
+ * a testing perspective.
+ */
+static void
+injection_point_cache_remove(const char *name)
+{
+	/* Leave if no cache */
+	if (InjectionPointCache == NULL)
+		return;
+
+	(void) hash_search(InjectionPointCache, name, HASH_REMOVE, NULL);
+}
+
+static InjectionPointCallback
+injection_point_cache_get(const char *name)
+{
+	bool		found;
+	InjectionPointCacheEntry *entry;
+
+	/* no callback if no cache yet */
+	if (InjectionPointCache == NULL)
+		return NULL;
+
+	entry = (InjectionPointCacheEntry *)
+		hash_search(InjectionPointCache, name, HASH_FIND, &found);
+
+	if (found)
+		return entry->callback;
+
+	return NULL;
+}
+#endif							/* USE_INJECTION_POINTS */
+
+/*
+ * Return the space for dynamic shared hash table.
+ */
+Size
+InjectionPointShmemSize(void)
+{
+#ifdef USE_INJECTION_POINTS
+	Size		sz = 0;
+
+	sz = add_size(sz, hash_estimate_size(INJECTION_POINT_HASH_MAX_SIZE,
+										 sizeof(InjectionPointEntry)));
+	return sz;
+#else
+	return 0;
+#endif
+}
+
+/*
+ * Allocate shmem space for dynamic shared hash.
+ */
+void
+InjectionPointShmemInit(void)
+{
+#ifdef USE_INJECTION_POINTS
+	HASHCTL		info;
+
+	/* key is a NULL-terminated string */
+	info.keysize = sizeof(char[INJ_NAME_MAXLEN]);
+	info.entrysize = sizeof(InjectionPointEntry);
+	InjectionPointHash = ShmemInitHash("InjectionPoint hash",
+									   INJECTION_POINT_HASH_INIT_SIZE,
+									   INJECTION_POINT_HASH_MAX_SIZE,
+									   &info,
+									   HASH_ELEM | HASH_STRINGS);
+#endif
+}
+
+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)
+{
+	struct stat st;
+
+	Assert(name != NULL);
+	if (stat(name, &st) == 0)
+		return !S_ISDIR(st.st_mode);
+	else if (!(errno == ENOENT || errno == ENOTDIR))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not access file \"%s\": %m", name)));
+	return false;
+}
+#endif
+
+/*
+ * Attach a new injection point.
+ */
+void
+InjectionPointAttach(const char *name,
+					 const char *library,
+					 const char *function)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+
+	if (strlen(name) >= INJ_NAME_MAXLEN)
+		elog(ERROR, "injection point name %s too long", name);
+	if (strlen(library) >= INJ_LIB_MAXLEN)
+		elog(ERROR, "injection point library %s too long", library);
+	if (strlen(function) >= INJ_FUNC_MAXLEN)
+		elog(ERROR, "injection point function %s too long", function);
+
+	/*
+	 * Allocate and register a new injection point.  A new point should not
+	 * exist.  For testing purposes this should be fine.
+	 */
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_ENTER, &found);
+	if (found)
+	{
+		LWLockRelease(InjectionPointLock);
+		elog(ERROR, "injection point \"%s\" already defined", name);
+	}
+
+	/* Save the entry */
+	memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+	entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+	entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+	memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+	entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+
+	LWLockRelease(InjectionPointLock);
+
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Detach an existing injection point.
+ */
+void
+InjectionPointDetach(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	bool		found;
+
+	LWLockAcquire(InjectionPointLock, LW_EXCLUSIVE);
+	hash_search(InjectionPointHash, name, HASH_REMOVE, &found);
+	LWLockRelease(InjectionPointLock);
+
+	if (!found)
+		elog(ERROR, "injection point \"%s\" not found", name);
+
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
+
+/*
+ * Execute an injection point, if defined.
+ *
+ * Check first the shared hash table, and adapt the local cache
+ * depending on that as it could be possible that an entry to run
+ * has been removed.
+ */
+void
+InjectionPointRun(const char *name)
+{
+#ifdef USE_INJECTION_POINTS
+	InjectionPointEntry *entry_by_name;
+	bool		found;
+	InjectionPointCallback injection_callback;
+
+	LWLockAcquire(InjectionPointLock, LW_SHARED);
+	entry_by_name = (InjectionPointEntry *)
+		hash_search(InjectionPointHash, name,
+					HASH_FIND, &found);
+	LWLockRelease(InjectionPointLock);
+
+	/*
+	 * If not found, do nothing and remove it from the local cache if it
+	 * existed there.
+	 */
+	if (!found)
+	{
+		injection_point_cache_remove(name);
+		return;
+	}
+
+	/*
+	 * Check if the callback exists in the local cache, to avoid unnecessary
+	 * external loads.
+	 */
+	injection_callback = injection_point_cache_get(name);
+	if (injection_callback == NULL)
+	{
+		char		path[MAXPGPATH];
+
+		/* Found, so just run the callback registered */
+		snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+				 entry_by_name->library, DLSUFFIX);
+
+		if (!file_exists(path))
+			elog(ERROR, "could not find injection library \"%s\"", path);
+
+		injection_callback = (InjectionPointCallback)
+			load_external_function(path, entry_by_name->function, true, NULL);
+
+		/* add it to the local cache when found */
+		injection_point_cache_add(name, injection_callback);
+	}
+
+	injection_callback(name);
+#else
+	elog(ERROR, "Injection points are not supported by this build");
+#endif
+}
diff --git a/src/backend/utils/misc/meson.build b/src/backend/utils/misc/meson.build
index f719c97c05..1438859b69 100644
--- a/src/backend/utils/misc/meson.build
+++ b/src/backend/utils/misc/meson.build
@@ -6,6 +6,7 @@ backend_sources += files(
   'guc_funcs.c',
   'guc_tables.c',
   'help_config.c',
+  'injection_point.c',
   'pg_config.c',
   'pg_controldata.c',
   'pg_rusage.c',
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index b23b35cd8e..d5a2fcb084 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -1676,6 +1676,21 @@ build-postgresql:
        </listitem>
       </varlistentry>
 
+      <varlistentry id="configure-option-enable-injection-points">
+       <term><option>--enable-injection-points</option></term>
+       <listitem>
+        <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+        </para>
+       </listitem>
+      </varlistentry>
+
       <varlistentry id="configure-option-with-segsize-blocks">
        <term><option>--with-segsize-blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
@@ -3184,6 +3199,21 @@ ninja install
       </listitem>
      </varlistentry>
 
+     <varlistentry id="configure-injection-points-meson">
+      <term><option>-Dinjection_points={ true | false }</option></term>
+      <listitem>
+       <para>
+        Compiles <productname>PostgreSQL</productname> with support for
+        injection points in the server.  This is valuable to inject
+        user-defined code to force specific conditions to happen on the
+        server in pre-defined code paths.  This option is disabled by default.
+        See <xref linkend="xfunc-addin-injection-points"/> for more details.
+        This option is only for developers to test specific concurrency
+        scenarios.
+       </para>
+      </listitem>
+     </varlistentry>
+
       <varlistentry id="configure-segsize-blocks-meson">
        <term><option>-Dsegsize_blocks=SEGSIZE_BLOCKS</option></term>
        <listitem>
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 89116ae74c..66cc94b03b 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3510,6 +3510,62 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
     </para>
    </sect2>
 
+   <sect2 id="xfunc-addin-injection-points">
+    <title>Injection Points</title>
+
+    <para>
+     Add-ins can define injection points, that can register callbacks
+     to run user-defined code when going through a specific code path,
+     by calling:
+<programlisting>
+extern void InjectionPointAttach(const char *name,
+                                 const char *library,
+                                 const char *function);
+</programlisting>
+
+     <literal>name</literal> is the name of the injection point, that
+     will execute the <literal>function</literal> loaded from
+     <literal>library</library>.
+     Injection points are saved in a hash table in shared memory, and
+     last until the server is shut down.
+    </para>
+
+    <para>
+     Here is an example of callback for
+     <literal>InjectionPointCallback</literal>:
+<programlisting>
+static void
+custom_injection_callback(const char *name)
+{
+    elog(NOTICE, "%s: executed custom callback", name);
+}
+</programlisting>
+    </para>
+
+    <para>
+     Once an injection point is defined, running it requires to use
+     the following macro to trigger the callback given in a wanted code
+     path:
+<programlisting>
+INJECTION_POINT(name);
+</programlisting>
+    </para>
+
+    <para>
+     Optionally, it is possible to detach injection points by calling:
+<programlisting>
+extern void InjectionPointDetach(const char *name);
+</programlisting>
+    </para>
+
+    <para>
+     Enabling injections points requires
+     <option>--enable-injection-points</option> from
+     <command>configure</command> or <option>-Dinjection_points=true</option>
+     from <application>Meson</application>.
+    </para>
+   </sect2>
+
    <sect2 id="extend-cpp">
     <title>Using C++ for Extensibility</title>
 
diff --git a/configure b/configure
index 217704e9ca..fbac8dd23f 100755
--- a/configure
+++ b/configure
@@ -759,6 +759,7 @@ CPPFLAGS
 LDFLAGS
 CFLAGS
 CC
+enable_injection_points
 enable_tap_tests
 enable_dtrace
 DTRACEFLAGS
@@ -839,6 +840,7 @@ enable_profiling
 enable_coverage
 enable_dtrace
 enable_tap_tests
+enable_injection_points
 with_blocksize
 with_segsize
 with_segsize_blocks
@@ -1532,6 +1534,8 @@ Optional Features:
   --enable-coverage       build with coverage testing instrumentation
   --enable-dtrace         build with DTrace support
   --enable-tap-tests      enable TAP tests (requires Perl and IPC::Run)
+  --enable-injection-points
+                          enable injection points (for testing)
   --enable-depend         turn on automatic dependency tracking
   --enable-cassert        enable assertion checks (for debugging)
   --disable-largefile     omit support for large files
@@ -3682,6 +3686,36 @@ fi
 
 
 
+#
+# Injection points
+#
+
+
+# Check whether --enable-injection-points was given.
+if test "${enable_injection_points+set}" = set; then :
+  enableval=$enable_injection_points;
+  case $enableval in
+    yes)
+
+$as_echo "#define USE_INJECTION_POINTS 1" >>confdefs.h
+
+      ;;
+    no)
+      :
+      ;;
+    *)
+      as_fn_error $? "no argument expected for --enable-injection-points option" "$LINENO" 5
+      ;;
+  esac
+
+else
+  enable_injection_points=no
+
+fi
+
+
+
+
 #
 # Block size
 #
diff --git a/configure.ac b/configure.ac
index e49de9e4f0..1507de55b7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -250,6 +250,13 @@ PGAC_ARG_BOOL(enable, tap-tests, no,
               [enable TAP tests (requires Perl and IPC::Run)])
 AC_SUBST(enable_tap_tests)
 
+#
+# Injection points
+#
+PGAC_ARG_BOOL(enable, injection-points, no, [enable injection points (for testing)],
+              [AC_DEFINE([USE_INJECTION_POINTS], 1, [Define to 1 to build with injection points. (--enable-injection-points)])])
+AC_SUBST(enable_injection_points)
+
 #
 # Block size
 #
diff --git a/meson.build b/meson.build
index 52c2a37c41..cae865f640 100644
--- a/meson.build
+++ b/meson.build
@@ -431,6 +431,7 @@ meson_bin = find_program(meson_binpath, native: true)
 ###############################################################
 
 cdata.set('USE_ASSERT_CHECKING', get_option('cassert') ? 1 : false)
+cdata.set('USE_INJECTION_POINTS', get_option('injection_points') ? 1 : false)
 
 blocksize = get_option('blocksize').to_int() * 1024
 
diff --git a/meson_options.txt b/meson_options.txt
index be1b327f54..7a5102df1a 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -43,6 +43,9 @@ option('cassert', type: 'boolean', value: false,
 option('tap_tests', type: 'feature', value: 'auto',
   description: 'Enable TAP tests')
 
+option('injection_points', type: 'boolean', value: false,
+  description: 'Enable injection points')
+
 option('PG_TEST_EXTRA', type: 'string', value: '',
   description: 'Enable selected extra tests')
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 104e5de0fe..7c7fd77b01 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -203,6 +203,7 @@ enable_nls	= @enable_nls@
 enable_debug	= @enable_debug@
 enable_dtrace	= @enable_dtrace@
 enable_coverage	= @enable_coverage@
+enable_injection_points = @enable_injection_points@
 enable_tap_tests	= @enable_tap_tests@
 
 python_includespec	= @python_includespec@
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index ba41149b88..4e495d45fa 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1145,6 +1145,8 @@ IdentLine
 IdentifierLookup
 IdentifySystemCmd
 IfStackElem
+InjectionPointCacheEntry
+InjectionPointEntry
 ImportForeignSchemaStmt
 ImportForeignSchemaType
 ImportForeignSchema_function
-- 
2.43.0



  [text/plain] v6-0002-Add-test-module-test_injection_points.patch (14.5K, ../../[email protected]/3-v6-0002-Add-test-module-test_injection_points.patch)
  download | inline diff:
From 1d1d27e9b696a01d3468acf51d13aca92b629e59 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 12 Dec 2023 11:40:50 +0100
Subject: [PATCH v6 2/4] Add test module test_injection_points

This is a test facility aimed at providing basic coverage for the code
routines of injection points.  This will be extended with more tests.
---
 src/test/modules/Makefile                     |   7 ++
 src/test/modules/meson.build                  |   1 +
 .../modules/test_injection_points/.gitignore  |   4 +
 .../modules/test_injection_points/Makefile    |  22 ++++
 .../expected/test_injection_points.out        | 117 ++++++++++++++++++
 .../modules/test_injection_points/meson.build |  37 ++++++
 .../sql/test_injection_points.sql             |  33 +++++
 .../test_injection_points--1.0.sql            |  36 ++++++
 .../test_injection_points.c                   |  91 ++++++++++++++
 .../test_injection_points.control             |   4 +
 10 files changed, 352 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/.gitignore
 create mode 100644 src/test/modules/test_injection_points/Makefile
 create mode 100644 src/test/modules/test_injection_points/expected/test_injection_points.out
 create mode 100644 src/test/modules/test_injection_points/meson.build
 create mode 100644 src/test/modules/test_injection_points/sql/test_injection_points.sql
 create mode 100644 src/test/modules/test_injection_points/test_injection_points--1.0.sql
 create mode 100644 src/test/modules/test_injection_points/test_injection_points.c
 create mode 100644 src/test/modules/test_injection_points/test_injection_points.control

diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 5d33fa6a9a..9a55a6924e 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -37,6 +37,13 @@ SUBDIRS = \
 		  worker_spi \
 		  xid_wraparound
 
+
+ifeq ($(enable_injection_points),yes)
+SUBDIRS += test_injection_points
+else
+ALWAYS_SUBDIRS += test_injection_points
+endif
+
 ifeq ($(with_ssl),openssl)
 SUBDIRS += ssl_passphrase_callback
 else
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index b76f588559..dc0048a6e4 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -17,6 +17,7 @@ subdir('test_ddl_deparse')
 subdir('test_dsa')
 subdir('test_extensions')
 subdir('test_ginpostinglist')
+subdir('test_injection_points')
 subdir('test_integerset')
 subdir('test_lfind')
 subdir('test_misc')
diff --git a/src/test/modules/test_injection_points/.gitignore b/src/test/modules/test_injection_points/.gitignore
new file mode 100644
index 0000000000..5dcb3ff972
--- /dev/null
+++ b/src/test/modules/test_injection_points/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/src/test/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
new file mode 100644
index 0000000000..65bcdde782
--- /dev/null
+++ b/src/test/modules/test_injection_points/Makefile
@@ -0,0 +1,22 @@
+# src/test/modules/test_injection_points/Makefile
+
+MODULE_big = test_injection_points
+OBJS = \
+	$(WIN32RES) \
+	test_injection_points.o
+PGFILEDESC = "test_injection_points - test injection points"
+
+EXTENSION = test_injection_points
+DATA = test_injection_points--1.0.sql
+REGRESS = test_injection_points
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_injection_points
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_injection_points/expected/test_injection_points.out b/src/test/modules/test_injection_points/expected/test_injection_points.out
new file mode 100644
index 0000000000..a8ddae0aad
--- /dev/null
+++ b/src/test/modules/test_injection_points/expected/test_injection_points.out
@@ -0,0 +1,117 @@
+CREATE EXTENSION test_injection_points;
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+ERROR:  incorrect mode "booh" for injection point creation
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+ test_injection_points_attach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR:  error triggered for injection point TestInjectionError
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+ test_injection_points_detach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+ test_injection_points_detach 
+------------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+ERROR:  injection point "TestInjectionLog" not found
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+NOTICE:  notice triggered for injection point TestInjectionLog2
+ test_injection_points_run 
+---------------------------
+ 
+(1 row)
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
new file mode 100644
index 0000000000..7509a102ef
--- /dev/null
+++ b/src/test/modules/test_injection_points/meson.build
@@ -0,0 +1,37 @@
+# Copyright (c) 2022-2023, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+  subdir_done()
+endif
+
+test_injection_points_sources = files(
+  'test_injection_points.c',
+)
+
+if host_system == 'windows'
+  test_injection_points_sources += rc_lib_gen.process(win32ver_rc, extra_args: [
+    '--NAME', 'test_injection_points',
+    '--FILEDESC', 'test_injection_points - test injection points',])
+endif
+
+test_injection_points = shared_module('test_injection_points',
+  test_injection_points_sources,
+  kwargs: pg_test_mod_args,
+)
+test_install_libs += test_injection_points
+
+test_install_data += files(
+  'test_injection_points.control',
+  'test_injection_points--1.0.sql',
+)
+
+tests += {
+  'name': 'test_injection_points',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'regress': {
+    'sql': [
+      'test_injection_points',
+    ],
+  },
+}
diff --git a/src/test/modules/test_injection_points/sql/test_injection_points.sql b/src/test/modules/test_injection_points/sql/test_injection_points.sql
new file mode 100644
index 0000000000..8f23f4c044
--- /dev/null
+++ b/src/test/modules/test_injection_points/sql/test_injection_points.sql
@@ -0,0 +1,33 @@
+CREATE EXTENSION test_injection_points;
+
+SELECT test_injection_points_attach('TestInjectionBooh', 'booh');
+SELECT test_injection_points_attach('TestInjectionError', 'error');
+SELECT test_injection_points_attach('TestInjectionLog', 'notice');
+SELECT test_injection_points_attach('TestInjectionLog2', 'notice');
+
+SELECT test_injection_points_run('TestInjectionBooh'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Re-load and run again.
+\c
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- error
+
+-- Remove one entry and check the other one.
+SELECT test_injection_points_detach('TestInjectionError'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- notice
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+-- All entries removed, nothing happens
+SELECT test_injection_points_detach('TestInjectionLog'); -- ok
+SELECT test_injection_points_run('TestInjectionLog'); -- nothing
+SELECT test_injection_points_run('TestInjectionError'); -- nothing
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+SELECT test_injection_points_detach('TestInjectionLog'); -- fails
+
+SELECT test_injection_points_run('TestInjectionLog2'); -- notice
+
+DROP EXTENSION test_injection_points;
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
new file mode 100644
index 0000000000..1c0a689ae2
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -0,0 +1,36 @@
+/* src/test/modules/test_injection_points/test_injection_points--1.0.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION test_injection_points" to load this file. \quit
+
+--
+-- test_injection_points_attach()
+--
+-- Attaches an injection point using callbacks from one of the predefined
+-- modes.
+--
+CREATE FUNCTION test_injection_points_attach(IN point_name TEXT,
+    IN mode text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_attach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_run()
+--
+-- Executes an injection point.
+--
+CREATE FUNCTION test_injection_points_run(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_run'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
+--
+-- test_injection_points_detach()
+--
+-- Detaches an injection point.
+--
+CREATE FUNCTION test_injection_points_detach(IN point_name TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_detach'
+LANGUAGE C STRICT PARALLEL UNSAFE;
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
new file mode 100644
index 0000000000..efb2c74c47
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -0,0 +1,91 @@
+/*--------------------------------------------------------------------------
+ *
+ * test_injection_points.c
+ *		Code for testing injection points.
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_injection_points/test_injection_points.c
+ *
+ * Injection points are able to trigger user-defined callbacks in pre-defined
+ * code paths.
+ *
+ * -------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "utils/builtins.h"
+#include "utils/injection_point.h"
+
+PG_MODULE_MAGIC;
+
+extern PGDLLEXPORT void test_injection_error(const char *name);
+extern PGDLLEXPORT void test_injection_notice(const char *name);
+
+/* Set of callbacks available at point creation */
+void
+test_injection_error(const char *name)
+{
+	elog(ERROR, "error triggered for injection point %s", name);
+}
+
+void
+test_injection_notice(const char *name)
+{
+	elog(NOTICE, "notice triggered for injection point %s", name);
+}
+
+/*
+ * SQL function for creating an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_attach);
+Datum
+test_injection_points_attach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+	char	   *mode = text_to_cstring(PG_GETARG_TEXT_PP(1));
+	char	   *function;
+
+	if (strcmp(mode, "error") == 0)
+		function = "test_injection_error";
+	else if (strcmp(mode, "notice") == 0)
+		function = "test_injection_notice";
+	else
+		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
+
+	InjectionPointAttach(name, "test_injection_points", function);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for triggering an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_run);
+Datum
+test_injection_points_run(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	INJECTION_POINT(name);
+
+	PG_RETURN_VOID();
+}
+
+/*
+ * SQL function for dropping an injection point.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_detach);
+Datum
+test_injection_points_detach(PG_FUNCTION_ARGS)
+{
+	char	   *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+	InjectionPointDetach(name);
+
+	PG_RETURN_VOID();
+}
diff --git a/src/test/modules/test_injection_points/test_injection_points.control b/src/test/modules/test_injection_points/test_injection_points.control
new file mode 100644
index 0000000000..a13657cfc6
--- /dev/null
+++ b/src/test/modules/test_injection_points/test_injection_points.control
@@ -0,0 +1,4 @@
+comment = 'Test code for injection points'
+default_version = '1.0'
+module_pathname = '$libdir/test_injection_points'
+relocatable = true
-- 
2.43.0



  [text/plain] v6-0003-Add-regression-test-to-show-snapbuild-consistency.patch (4.2K, ../../[email protected]/4-v6-0003-Add-regression-test-to-show-snapbuild-consistency.patch)
  download | inline diff:
From aaf4b21dffcabd55f7dd5fb9d1f35f13ba434153 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 16 Nov 2023 14:28:22 +0900
Subject: [PATCH v6 3/4] Add regression test to show snapbuild consistency

Reverting 409f9ca44713 causes the test to fail.  The test added here
relies on the existing callbacks in test_injection_points.
---
 src/backend/replication/logical/snapbuild.c   |  3 ++
 .../modules/test_injection_points/Makefile    |  2 +
 .../modules/test_injection_points/meson.build |  5 ++
 .../t/001_snapshot_status.pl                  | 47 +++++++++++++++++++
 4 files changed, 57 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/t/001_snapshot_status.pl

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index fec190a8b2..3491e5a872 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -141,6 +141,7 @@
 #include "storage/procarray.h"
 #include "storage/standby.h"
 #include "utils/builtins.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/snapmgr.h"
 #include "utils/snapshot.h"
@@ -654,6 +655,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	snap->xcnt = newxcnt;
 	snap->xip = newxip;
 
+	INJECTION_POINT("SnapBuildInitialSnapshot");
+
 	return snap;
 }
 
diff --git a/src/test/modules/test_injection_points/Makefile b/src/test/modules/test_injection_points/Makefile
index 65bcdde782..4696c1b013 100644
--- a/src/test/modules/test_injection_points/Makefile
+++ b/src/test/modules/test_injection_points/Makefile
@@ -10,6 +10,8 @@ EXTENSION = test_injection_points
 DATA = test_injection_points--1.0.sql
 REGRESS = test_injection_points
 
+TAP_TESTS = 1
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 7509a102ef..6006b38f3d 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -34,4 +34,9 @@ tests += {
       'test_injection_points',
     ],
   },
+  'tap': {
+    'tests': [
+      't/001_snapshot_status.pl',
+    ],
+  }
 }
diff --git a/src/test/modules/test_injection_points/t/001_snapshot_status.pl b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
new file mode 100644
index 0000000000..ca5c6cc7a4
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/001_snapshot_status.pl
@@ -0,0 +1,47 @@
+# Test consistent of initial snapshot data.
+
+# This requires a node with wal_level=logical combined with an injection
+# point that forces a failure when a snapshot is initially built with a
+# logical slot created.
+#
+# See bug https://postgr.es/m/CAFiTN-s0zA1Kj0ozGHwkYkHwa5U0zUE94RSc_g81WrpcETB5=w@mail.gmail.com.
+
+use strict;
+use warnings;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init(allows_streaming => 'logical');
+$node->start;
+
+$node->safe_psql('postgres', 'CREATE EXTENSION test_injection_points;');
+$node->safe_psql('postgres',
+  "SELECT test_injection_points_attach('SnapBuildInitialSnapshot', 'error');");
+
+my $node_host = $node->host;
+my $node_port = $node->port;
+my $connstr_common = "host=$node_host port=$node_port";
+my $connstr_db = "$connstr_common replication=database dbname=postgres";
+
+# This requires a single session, with two commands.
+my $psql_session =
+  $node->background_psql('postgres', on_error_stop => 0,
+			 extra_params => [ '-d', $connstr_db ]);
+my ($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+ok($ret != 0, "First CREATE_REPLICATION_SLOT fails on injected error");
+
+# Now remove the injected error and check that the second command works.
+$node->safe_psql('postgres',
+  "SELECT test_injection_points_detach('SnapBuildInitialSnapshot');");
+
+($output, $ret) = $psql_session->query(
+    'CREATE_REPLICATION_SLOT "slot" LOGICAL "pgoutput";');
+print "BOO" . substr($output, 0, 4) . "\n";
+ok(substr($output, 0, 4) eq 'slot',
+   "Second CREATE_REPLICATION_SLOT passes");
+
+done_testing();
-- 
2.43.0



  [text/plain] v6-0004-Add-basic-test-for-promotion-and-restart-race-con.patch (10.8K, ../../[email protected]/5-v6-0004-Add-basic-test-for-promotion-and-restart-race-con.patch)
  download | inline diff:
From f184ace1b21c70c09b41bc54ebeb5f341098a96c Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Thu, 16 Nov 2023 14:42:31 +0900
Subject: [PATCH v6 4/4] Add basic test for promotion and restart race
 condition

This test fails after 7863ee4def65 is reverted.  test_injection_points
is extended so as it is possible to add condition variables to wait for
in the point callbacks, with a SQL function to broadcast condition
variables that may be sleeping.

I guess that this should be extended so as there is more than one
condition variable stored in shmem for this module, controlling which
variable to wait for directly in the callback itself, but that's not
really necessary now.
---
 src/backend/access/transam/xlog.c             |   7 +
 .../modules/test_injection_points/meson.build |   1 +
 .../t/002_invalid_checkpoint_after_promote.pl | 132 ++++++++++++++++++
 .../test_injection_points--1.0.sql            |  10 ++
 .../test_injection_points.c                   |  73 ++++++++++
 5 files changed, 223 insertions(+)
 create mode 100644 src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 01e0484584..ece31bb2a6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -99,6 +99,7 @@
 #include "storage/sync.h"
 #include "utils/guc_hooks.h"
 #include "utils/guc_tables.h"
+#include "utils/injection_point.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
 #include "utils/relmapper.h"
@@ -7345,6 +7346,12 @@ CreateRestartPoint(int flags)
 
 	CheckPointGuts(lastCheckPoint.redo, flags);
 
+	/*
+	 * This location is important to be after CheckPointGuts() to ensure
+	 * that some work has happened.
+	 */
+	INJECTION_POINT("CreateRestartPoint");
+
 	/*
 	 * Remember the prior checkpoint's redo ptr for
 	 * UpdateCheckPointDistanceEstimate()
diff --git a/src/test/modules/test_injection_points/meson.build b/src/test/modules/test_injection_points/meson.build
index 6006b38f3d..6ebdc728b7 100644
--- a/src/test/modules/test_injection_points/meson.build
+++ b/src/test/modules/test_injection_points/meson.build
@@ -37,6 +37,7 @@ tests += {
   'tap': {
     'tests': [
       't/001_snapshot_status.pl',
+      't/002_invalid_checkpoint_after_promote.pl',
     ],
   }
 }
diff --git a/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
new file mode 100644
index 0000000000..2da243e871
--- /dev/null
+++ b/src/test/modules/test_injection_points/t/002_invalid_checkpoint_after_promote.pl
@@ -0,0 +1,132 @@
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Time::HiRes qw(usleep nanosleep);
+use Test::More;
+
+# initialize primary node
+my $node_primary = PostgreSQL::Test::Cluster->new('master');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+	'postgresql.conf', q[
+checkpoint_timeout = 30s
+log_checkpoints = on
+restart_after_crash = on
+]);
+$node_primary->start;
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+
+# setup a standby
+my $node_standby = PostgreSQL::Test::Cluster->new('standby1');
+$node_standby->init_from_backup($node_primary, $backup_name, has_streaming => 1);
+$node_standby->start;
+
+# dummy table for the upcoming tests.
+$node_primary->safe_psql('postgres', 'checkpoint');
+$node_primary->safe_psql('postgres', 'CREATE TABLE prim_tab (a int);');
+
+# Register a injection point on the standby so as the follow-up
+# restart point running on it will wait.
+$node_primary->safe_psql('postgres', 'CREATE EXTENSION test_injection_points;');
+# Wait until the extension has been created on the standby
+$node_primary->wait_for_replay_catchup($node_standby);
+# This causes a restartpoint to wait on a standby.
+$node_standby->safe_psql('postgres',
+  "SELECT test_injection_points_attach('CreateRestartPoint', 'wait');");
+
+# Execute a restart point on the standby, that will be waited on.
+# This needs to be in the background as we'll wait on it.
+my $logstart = -s $node_standby->logfile;
+my $psql_session =
+  $node_standby->background_psql('postgres', on_error_stop => 0);
+$psql_session->query_until(qr/starting_checkpoint/, q(
+   \echo starting_checkpoint
+   CHECKPOINT;
+));
+
+# Switch one WAL segment to make the restartpoint remove it.
+$node_primary->safe_psql('postgres', 'INSERT INTO prim_tab VALUES (1);');
+$node_primary->safe_psql('postgres', 'SELECT pg_switch_wal();');
+$node_primary->wait_for_replay_catchup($node_standby);
+
+# Wait until the checkpointer is in the middle of the restartpoint
+# processing.
+ok( $node_standby->poll_query_until(
+	'postgres',
+	qq[SELECT count(*) FROM pg_stat_activity
+           WHERE backend_type = 'checkpointer' AND wait_event = 'test_injection_wait' ;],
+	'1'),
+    'checkpointer is waiting at restart point'
+    ) or die "Timed out while waiting for checkpointer to run restartpoint";
+
+
+# Restartpoint should have started on standby.
+my $log = slurp_file($node_standby->logfile, $logstart);
+my $checkpoint_start = 0;
+if ($log =~ m/restartpoint starting: immediate wait/)
+{
+	$checkpoint_start = 1;
+}
+is($checkpoint_start, 1, 'restartpoint has started');
+
+# promote during restartpoint
+$node_primary->stop;
+$node_standby->promote;
+
+# Update the start position before waking up the checkpointer!
+$logstart = -s $node_standby->logfile;
+
+# Now wake up the checkpointer
+$node_standby->safe_psql('postgres',
+  "SELECT test_injection_points_wake();");
+
+# wait until checkpoint completes on the newly-promoted standby.
+my $checkpoint_complete = 0;
+for (my $i = 0; $i < 3000; $i++)
+{
+	my $log = slurp_file($node_standby->logfile, $logstart);
+	if ($log =~ m/restartpoint complete/)
+	{
+		$checkpoint_complete = 1;
+		last;
+	}
+	usleep(100_000);
+}
+is($checkpoint_complete, 1, 'restartpoint has completed');
+
+# kill SIGKILL a backend, and all backend will restart. Note that previous checkpoint has not completed.
+my $psql_timeout = IPC::Run::timer(3600);
+my ($killme_stdin, $killme_stdout, $killme_stderr) = ('', '', '');
+my $killme = IPC::Run::start(
+	[ 'psql', '-XAtq', '-v', 'ON_ERROR_STOP=1', '-f', '-', '-d', $node_standby->connstr('postgres') ],
+	'<',
+	\$killme_stdin,
+	'>',
+	\$killme_stdout,
+	'2>',
+	\$killme_stderr,
+	$psql_timeout);
+$killme_stdin .= q[
+SELECT pg_backend_pid();
+];
+$killme->pump until $killme_stdout =~ /[[:digit:]]+[\r\n]$/;
+my $pid = $killme_stdout;
+chomp($pid);
+my $ret = PostgreSQL::Test::Utils::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, 'killed process with KILL');
+my $stdout;
+my $stderr;
+
+# after recovery, the server will not start, and log PANIC: could not locate a valid checkpoint record
+for (my $i = 0; $i < 30; $i++)
+{
+    ($ret, $stdout, $stderr) = $node_standby->psql('postgres', 'select 1');
+    last if $ret == 0;
+	sleep(1);
+}
+is($ret, 0, "psql connect success");
+is($stdout, 1, "psql select 1");
+
+done_testing();
diff --git a/src/test/modules/test_injection_points/test_injection_points--1.0.sql b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
index 1c0a689ae2..05f97f0982 100644
--- a/src/test/modules/test_injection_points/test_injection_points--1.0.sql
+++ b/src/test/modules/test_injection_points/test_injection_points--1.0.sql
@@ -25,6 +25,16 @@ RETURNS void
 AS 'MODULE_PATHNAME', 'test_injection_points_run'
 LANGUAGE C STRICT PARALLEL UNSAFE;
 
+--
+-- test_injection_points_wake()
+--
+-- Wakes a condition variable executed in an injection point.
+--
+CREATE FUNCTION test_injection_points_wake()
+RETURNS void
+AS 'MODULE_PATHNAME', 'test_injection_points_wake'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
 --
 -- test_injection_points_detach()
 --
diff --git a/src/test/modules/test_injection_points/test_injection_points.c b/src/test/modules/test_injection_points/test_injection_points.c
index efb2c74c47..8b837d85d3 100644
--- a/src/test/modules/test_injection_points/test_injection_points.c
+++ b/src/test/modules/test_injection_points/test_injection_points.c
@@ -18,13 +18,56 @@
 #include "postgres.h"
 
 #include "fmgr.h"
+#include "storage/condition_variable.h"
+#include "storage/lwlock.h"
+#include "storage/shmem.h"
 #include "utils/builtins.h"
 #include "utils/injection_point.h"
+#include "utils/wait_event.h"
 
 PG_MODULE_MAGIC;
 
+/* Shared state information for injection points. */
+typedef struct TestInjectionPointSharedState
+{
+	/*
+	 * Wait variable that can be registered at a given point, and that can be
+	 * awakened via SQL.
+	 */
+	ConditionVariable	wait_point;
+} TestInjectionPointSharedState;
+
+/* Pointer to shared-memory state. */
+static TestInjectionPointSharedState *inj_state = NULL;
+
+/* Wait event when waiting on condition variable */
+static uint32 test_injection_wait_event = 0;
+
 extern PGDLLEXPORT void test_injection_error(const char *name);
 extern PGDLLEXPORT void test_injection_notice(const char *name);
+extern PGDLLEXPORT void test_injection_wait(const char *name);
+
+
+static void
+test_injection_init_shmem(void)
+{
+	bool		found;
+
+	if (inj_state != NULL)
+		return;
+
+	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+	inj_state = ShmemInitStruct("test_injection_points",
+								sizeof(TestInjectionPointSharedState),
+								&found);
+	if (!found)
+	{
+		/* First time through ... */
+		MemSet(inj_state, 0, sizeof(TestInjectionPointSharedState));
+		ConditionVariableInit(&inj_state->wait_point);
+	}
+	LWLockRelease(AddinShmemInitLock);
+}
 
 /* Set of callbacks available at point creation */
 void
@@ -39,6 +82,20 @@ test_injection_notice(const char *name)
 	elog(NOTICE, "notice triggered for injection point %s", name);
 }
 
+void
+test_injection_wait(const char *name)
+{
+	if (inj_state == NULL)
+		test_injection_init_shmem();
+	if (test_injection_wait_event == 0)
+		test_injection_wait_event = WaitEventExtensionNew("test_injection_wait");
+
+	/* And sleep.. */
+	ConditionVariablePrepareToSleep(&inj_state->wait_point);
+	ConditionVariableSleep(&inj_state->wait_point, test_injection_wait_event);
+	ConditionVariableCancelSleep();
+}
+
 /*
  * SQL function for creating an injection point.
  */
@@ -54,6 +111,8 @@ test_injection_points_attach(PG_FUNCTION_ARGS)
 		function = "test_injection_error";
 	else if (strcmp(mode, "notice") == 0)
 		function = "test_injection_notice";
+	else if (strcmp(mode, "wait") == 0)
+		function = "test_injection_wait";
 	else
 		elog(ERROR, "incorrect mode \"%s\" for injection point creation", mode);
 
@@ -76,6 +135,20 @@ test_injection_points_run(PG_FUNCTION_ARGS)
 	PG_RETURN_VOID();
 }
 
+/*
+ * SQL function for waking a condition variable.
+ */
+PG_FUNCTION_INFO_V1(test_injection_points_wake);
+Datum
+test_injection_points_wake(PG_FUNCTION_ARGS)
+{
+	if (inj_state == NULL)
+		test_injection_init_shmem();
+
+	ConditionVariableBroadcast(&inj_state->wait_point);
+	PG_RETURN_VOID();
+}
+
 /*
  * SQL function for dropping an injection point.
  */
-- 
2.43.0



  [application/pgp-signature] signature.asc (833B, ../../[email protected]/6-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: Adding facility for injection points (or probe points?) for more advanced tests
@ 2024-01-02 10:06  Ashutosh Bapat <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 6+ messages in thread

From: Ashutosh Bapat @ 2024-01-02 10:06 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Dilip Kumar <[email protected]>; Postgres hackers <[email protected]>

On Tue, Dec 12, 2023 at 4:15 PM Michael Paquier <[email protected]> wrote:
>
> On Tue, Dec 12, 2023 at 10:27:09AM +0530, Dilip Kumar wrote:
> > Oops, I only included the code changes where I am adding injection
> > points and some comments to verify that, but missed the actual test
> > file. Attaching it here.
>
> I see.  Interesting that this requires persistent connections to work.
> That's something I've found clunky to rely on when the scenarios a
> test needs to deal with are rather complex.  That's an area that could
> be made easier to use outside of this patch..  Something got proposed
> by Andrew Dunstan to make the libpq routines usable through a perl
> module, for example.
>
> > Note:  I think the latest patches are conflicting with the head, can you rebase?
>
> Indeed, as per the recent manipulations in ipci.c for the shmem
> initialization areas.  Here goes a v6.

Sorry for replying late here. Another minor conflict has risen again.
It's minor enough to be ignored for a review.

On Tue, Nov 21, 2023 at 6:56 AM Michael Paquier <[email protected]> wrote:
>
> On Mon, Nov 20, 2023 at 04:53:45PM +0530, Ashutosh Bapat wrote:
> > On Wed, Oct 25, 2023 at 9:43 AM Michael Paquier <[email protected]> wrote:
> >> I have added some documentation to explain that, as well.  I am not
> >> wedded to the name proposed in the patch, so if you feel there is
> >> better, feel free to propose ideas.
> >
> > Actually with Attach and Detach terminology, INJECTION_POINT becomes
> > the place where we "declare" the injection point. So the documentation
> > needs to first explain INJECTION_POINT and then explain the other
> > operations.
>
> Sure.

This discussion has not been addressed in v6. I think the interface
needs to be documented in the order below
INJECTION_POINT - this declares an injection point - i.e. a place in
code where an external code can be injected (and run).
InjectionPointAttach() - this is used to associate ("attach") external
code to an injection point.
InjectionPointDetach() - this is used to disassociate ("detach")
external code from an injection point.

Specifying that InjectionPointAttach() "defines" an injection point
gives an impression that the injection point will be "somehow" added
to the code by calling InjectionPointAttach() which is not true. For
InjectionPointAttach() to be useful, the first argument to it should
be something already "declared" in the code using INJECTION_POINT().
Hence INJECTION_POINT needs to be mentioned in the documentation
first, followed by Attach and detach. The documentation needs to be
rephrased to use terms "declare", "attach" and "detach" instead of
"define", "run". The first set is aligned with the functionality
whereas the second set is aligned with the implementation.

Even if an INJECTION_POINT is not "declared" attach would succeed but
doesn't do anything. I think this needs to be made clear in the
documentation. Better if we could somehow make Attach() fail if the
specified injection point is not "declared" using INJECTION_POINT. Of
course we don't want to bloat the hash table with all "declared"
injection points even if they aren't being attached to and hence not
used. I think, exposing the current injection point strings as
#defines and encouraging users to use these macros instead of string
literals will be a good start.

With the current implementation it's possible to "declare" injection
point with same name at multiple places. It's useful but is it
intended?

/* Field sizes */
#define INJ_NAME_MAXLEN 64
#define INJ_LIB_MAXLEN 128
#define INJ_FUNC_MAXLEN 128
I think these limits should be either documented or specified in the
error messages for users to fix their code in case of
errors/unexpected behaviour.

Here are some code level comments on 0001

+typedef struct InjectionPointArrayEntry

This is not an array entry anymore. I think we should rename
InjectionPointEntry as SharedInjectionPointEntry and InjectionPointArrayEntry
as LocalInjectionPointEntry.

+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+ InjectionPointCallback callback)
+{
... snip ...
+
+ entry = (InjectionPointCacheEntry *)
+ hash_search(InjectionPointCache, name, HASH_ENTER, &found);
+
+ if (!found)

The function is called only when the injection point is not found in the local
cache. So this condition will always be true. An Assert will help to make it
clear and also prevent an unintended callback replacement.

+#ifdef USE_INJECTION_POINTS
+static bool
+file_exists(const char *name)

There's similar function in jit.c and dfmgr.c. Can we not reuse that code?

+ /* Save the entry */
+ memcpy(entry_by_name->name, name, sizeof(entry_by_name->name));
+ entry_by_name->name[INJ_NAME_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->library, library, sizeof(entry_by_name->library));
+ entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
+ memcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
+ entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';

Most of the code is using strncpy instead of memcpy. Why is this code different?

+ injection_callback = injection_point_cache_get(name);
+ if (injection_callback == NULL)
+ {
+ char path[MAXPGPATH];
+
+ /* Found, so just run the callback registered */

The condition indicates that the callback was not found. Comment looks wrong.

+ snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
+ entry_by_name->library, DLSUFFIX);
+
+ if (!file_exists(path))
+ elog(ERROR, "could not find injection library \"%s\"", path);
+
+ injection_callback = (InjectionPointCallback)
+ load_external_function(path, entry_by_name->function, true, NULL);
+
+ /* add it to the local cache when found */
+ injection_point_cache_add(name, injection_callback);
+ }
+

Consider case

Backend 2
InjectionPointAttach("xyz", "abc", "pqr");

Backend 1
INJECTION_POINT("xyz");

Backend 2
InjectionPointDetach("xyz");
InjectionPointAttach("xyz", "uvw", "lmn");

Backend 1
INJECTION_POINT("xyz");

IIUC, the last INJECTION_POINT would run abc.pqr instead of uvw.lmn.
Am I correct?

To fix this, we have to a. either save qualified name of the function in local
cache too OR resolve the function name every time INJECTION_POINT is invoked
and is found in the shared hash table. The first one option is cheaper I think.
But it will be good if we can invalidate the local entry when the global entry
changes. To keep code simple, we may choose to ignore close race conditions
where INJECTION_POINT is run while InjectionPointAttach or InjectionPointDetach
is happening. But this way we don't have to look up shared hash table every
time INJECTION_POINT is invoked thus improving performance.

I will look at 0002 next.

-- 
Best Wishes,
Ashutosh Bapat






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: Adding facility for injection points (or probe points?) for more advanced tests
@ 2024-01-04 12:52  Ashutosh Bapat <[email protected]>
  parent: Ashutosh Bapat <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Ashutosh Bapat @ 2024-01-04 12:52 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Dilip Kumar <[email protected]>; Postgres hackers <[email protected]>

On Tue, Jan 2, 2024 at 3:36 PM Ashutosh Bapat
<[email protected]> wrote:

>
> I will look at 0002 next.

One more comment on 0001
InjectionPointAttach() doesn't test whether the given function exists
in the given library. Even if InjectionPointAttach() succeeds,
INJECTION_POINT might throw error because the function doesn't exist.
This can be seen as an unwanted behaviour. I think
InjectionPointAttach() should test whether the function exists and
possibly load it as well by adding it to the local cache.

0002 comments
--- /dev/null
+++ b/src/test/modules/test_injection_points/expected/test_injection_points.out

When built without injection point support, this test fails. We should
add an alternate output file for such a build so that the behaviour
with and without injection point support is tested. Or set up things
such that the test is not run under make check in that directory. I
will prefer the first option.

+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Re-load and run again.

What's getting Re-loaded here? \c will create a new connection and
thus a new backend. Maybe the comment should say "test in a fresh
backend" or something of that sort?

+
+SELECT test_injection_points_run('TestInjectionError'); -- error
+ERROR: error triggered for injection point TestInjectionError
+-- Remove one entry and check the other one.

Looks confusing to me, we are testing the one removed as well. Am I
missing something?

+(1 row)
+
+-- All entries removed, nothing happens

We aren't removing all entries TestInjectionLog2 is still there. Am I
missing something?

0003 looks mostly OK.

0004 comments

+
+# after recovery, the server will not start, and log PANIC: could not
locate a valid checkpoint record

IIUC the comment describes the behaviour with 7863ee4def65 reverted.
But the test after this comment is written for the behaviour with
7863ee4def65. That's confusing. Is the intent to describe both
behaviours in the comment?

+
+ /* And sleep.. */
+ ConditionVariablePrepareToSleep(&inj_state->wait_point);
+ ConditionVariableSleep(&inj_state->wait_point, test_injection_wait_event);
+ ConditionVariableCancelSleep();

According to the prologue of ConditionVariableSleep(), that function
should be called in a loop checking for the desired condition. All the
callers that I examined follow that pattern. I think we need to follow
that pattern here as well.

Below comment from ConditionVariableTimedSleep() makes me think that
the caller of ConditionVariableSleep() can be woken up even if the
condition variable was not signaled. That's why the while() loop
around ConditionVariableSleep().

* If we're still in the wait list, then the latch must have been set
* by something other than ConditionVariableSignal; though we don't
* guarantee not to return spuriously, we'll avoid this obvious case.
*/.

That's all I have for now.

-- 
Best Wishes,
Ashutosh Bapat






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: Adding facility for injection points (or probe points?) for more advanced tests
@ 2024-01-05 09:30  Dilip Kumar <[email protected]>
  parent: Michael Paquier <[email protected]>
  1 sibling, 1 reply; 6+ messages in thread

From: Dilip Kumar @ 2024-01-05 09:30 UTC (permalink / raw)
  To: Michael Paquier <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Postgres hackers <[email protected]>

On Tue, Dec 12, 2023 at 4:15 PM Michael Paquier <[email protected]> wrote:
>
> On Tue, Dec 12, 2023 at 10:27:09AM +0530, Dilip Kumar wrote:
> > Oops, I only included the code changes where I am adding injection
> > points and some comments to verify that, but missed the actual test
> > file. Attaching it here.
>
> I see.  Interesting that this requires persistent connections to work.
> That's something I've found clunky to rely on when the scenarios a
> test needs to deal with are rather complex.  That's an area that could
> be made easier to use outside of this patch..  Something got proposed
> by Andrew Dunstan to make the libpq routines usable through a perl
> module, for example.
>
> > Note:  I think the latest patches are conflicting with the head, can you rebase?
>
> Indeed, as per the recent manipulations in ipci.c for the shmem
> initialization areas.  Here goes a v6.

Some comments in 0001, mostly cosmetics

1.
+/* utilities to handle the local array cache */
+static void
+injection_point_cache_add(const char *name,
+   InjectionPointCallback callback)

I think the comment for this function should be more specific about
adding an entry to the local injection_point_cache_add.  And add
comments for other functions as well e.g. injection_point_cache_get


2.
+typedef struct InjectionPointEntry
+{
+ char name[INJ_NAME_MAXLEN]; /* hash key */
+ char library[INJ_LIB_MAXLEN]; /* library */
+ char function[INJ_FUNC_MAXLEN]; /* function */
+} InjectionPointEntry;

Some comments would be good for the structure

3.

+static bool
+file_exists(const char *name)
+{
+ struct stat st;
+
+ Assert(name != NULL);
+ if (stat(name, &st) == 0)
+ return !S_ISDIR(st.st_mode);
+ else if (!(errno == ENOENT || errno == ENOTDIR))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not access file \"%s\": %m", name)));
+ return false;
+}

dfmgr.c has a similar function so can't we reuse it by making that
function external?

4.
+ if (found)
+ {
+ LWLockRelease(InjectionPointLock);
+ elog(ERROR, "injection point \"%s\" already defined", name);
+ }
+
...
+#else
+ elog(ERROR, "Injection points are not supported by this build");

Better to use similar formatting for error output, Injection vs
injection (better not to capitalize the first letter for consistency
pov)

5.
+ * Check first the shared hash table, and adapt the local cache
+ * depending on that as it could be possible that an entry to run
+ * has been removed.
+ */

What if the entry is removed after we have released the
InjectionPointLock? Or this would not cause any harm?


0004:

I think
test_injection_points_wake() and test_injection_wait() can be moved as
part of 0002



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: Adding facility for injection points (or probe points?) for more advanced tests
@ 2024-01-09 01:09  Michael Paquier <[email protected]>
  parent: Dilip Kumar <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Michael Paquier @ 2024-01-09 01:09 UTC (permalink / raw)
  To: Dilip Kumar <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Postgres hackers <[email protected]>

On Fri, Jan 05, 2024 at 03:00:25PM +0530, Dilip Kumar wrote:
> Some comments in 0001, mostly cosmetics
> 
> 1.
> +/* utilities to handle the local array cache */
> +static void
> +injection_point_cache_add(const char *name,
> +   InjectionPointCallback callback)
> 
> I think the comment for this function should be more specific about
> adding an entry to the local injection_point_cache_add.  And add
> comments for other functions as well e.g. injection_point_cache_get

And it is not an array anymore.  Note InjectionPointArrayEntry that
still existed.

> 2.
> +typedef struct InjectionPointEntry
> +{
> + char name[INJ_NAME_MAXLEN]; /* hash key */
> + char library[INJ_LIB_MAXLEN]; /* library */
> + char function[INJ_FUNC_MAXLEN]; /* function */
> +} InjectionPointEntry;
> 
> Some comments would be good for the structure

Sure.  I've spent more time documenting things in injection_point.c,
addressing any inconsistencies.

> 3.
> 
> +static bool
> +file_exists(const char *name)
> +{
> + struct stat st;
> +
> + Assert(name != NULL);
> + if (stat(name, &st) == 0)
> + return !S_ISDIR(st.st_mode);
> + else if (!(errno == ENOENT || errno == ENOTDIR))
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not access file \"%s\": %m", name)));
> + return false;
> +}
> 
> dfmgr.c has a similar function so can't we reuse it by making that
> function external?

Yes.  Note that jit.c has an extra copy of it.  I was holding on the
refactoring, but let's bite the bullet and have a single routine.
I've moved that into a 0001 that builds on top of the rest.

> 4.
> + if (found)
> + {
> + LWLockRelease(InjectionPointLock);
> + elog(ERROR, "injection point \"%s\" already defined", name);
> + }
> +
> ...
> +#else
> + elog(ERROR, "Injection points are not supported by this build");
> 
> Better to use similar formatting for error output, Injection vs
> injection (better not to capitalize the first letter for consistency
> pov)

Fixed.

> 5.
> + * Check first the shared hash table, and adapt the local cache
> + * depending on that as it could be possible that an entry to run
> + * has been removed.
> + */
> 
> What if the entry is removed after we have released the
> InjectionPointLock? Or this would not cause any harm?

With an entry found in the shmem table?  I don't really think that we
need to care about such cases, TBH, because the injection point would
have been found in the table to start with.  This comes down to if we
should try to hold InjectionPointLock while calling the callback, and
that may not be a good idea in some cases if you'd expect a high
concurrency on the callback running.

> 0004:
> 
> I think
> test_injection_points_wake() and test_injection_wait() can be moved as
> part of 0002

Nah.  I intend to keep the introduction of this API where it becomes
relevant.  Perhaps this could also use an isolation test?  This could
always be polished once we agree on 0001 and 0002.

(I'll post a v6 a bit later, there are more comments posted here and
there.)
--
Michael


Attachments:

  [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
  download

^ permalink  raw  reply  [nested|flat] 6+ messages in thread

end of thread, other threads:[~2024-01-09 01:09 UTC | newest]

Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 01:35 [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]>
2023-12-12 10:44 Re: Adding facility for injection points (or probe points?) for more advanced tests Michael Paquier <[email protected]>
2024-01-02 10:06 ` Re: Adding facility for injection points (or probe points?) for more advanced tests Ashutosh Bapat <[email protected]>
2024-01-04 12:52   ` Re: Adding facility for injection points (or probe points?) for more advanced tests Ashutosh Bapat <[email protected]>
2024-01-05 09:30 ` Re: Adding facility for injection points (or probe points?) for more advanced tests Dilip Kumar <[email protected]>
2024-01-09 01:09   ` Re: Adding facility for injection points (or probe points?) for more advanced tests Michael Paquier <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox