inplace050-tests-inj-v2.patch

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Add an injection_points isolation test suite.
    
    Make the isolation harness recognize injection_points wait events as a
    type of blocked state.  To simplify that, change that wait event naming
    scheme to INJECTION_POINT(name).  Add an initial test for an extant
    inplace-update bug.
    
    Reviewed by FIXME.
    
    Discussion: https://postgr.es/m/20240512232923.aa.nmisch@google.com

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 4be0dee..4eda445 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -63,6 +63,7 @@
 #include "storage/procarray.h"
 #include "storage/standby.h"
 #include "utils/datum.h"
+#include "utils/injection_point.h"
 #include "utils/inval.h"
 #include "utils/relcache.h"
 #include "utils/snapmgr.h"
@@ -6077,6 +6078,7 @@ heap_inplace_update(Relation relation, HeapTuple tuple)
 				(errcode(ERRCODE_INVALID_TRANSACTION_STATE),
 				 errmsg("cannot update tuples during a parallel operation")));
 
+	INJECTION_POINT("inplace-before-pin");
 	buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(&(tuple->t_self)));
 	LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
 	page = (Page) BufferGetPage(buffer);
diff --git a/src/backend/utils/adt/waitfuncs.c b/src/backend/utils/adt/waitfuncs.c
index d9c92c3..b524a8a 100644
--- a/src/backend/utils/adt/waitfuncs.c
+++ b/src/backend/utils/adt/waitfuncs.c
@@ -14,8 +14,13 @@
 
 #include "catalog/pg_type.h"
 #include "storage/predicate_internals.h"
+#include "storage/proc.h"
+#include "storage/procarray.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
+#include "utils/wait_event.h"
+
+#define UINT32_ACCESS_ONCE(var)		 ((uint32)(*((volatile uint32 *)&(var))))
 
 
 /*
@@ -23,8 +28,9 @@
  *
  * Check if specified PID is blocked by any of the PIDs listed in the second
  * argument.  Currently, this looks for blocking caused by waiting for
- * heavyweight locks or safe snapshots.  We ignore blockage caused by PIDs
- * not directly under the isolationtester's control, eg autovacuum.
+ * injection points, heavyweight locks, or safe snapshots.  We ignore blockage
+ * caused by PIDs not directly under the isolationtester's control, eg
+ * autovacuum.
  *
  * This is an undocumented function intended for use by the isolation tester,
  * and may change in future releases as required for testing purposes.
@@ -34,6 +40,8 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 {
 	int			blocked_pid = PG_GETARG_INT32(0);
 	ArrayType  *interesting_pids_a = PG_GETARG_ARRAYTYPE_P(1);
+	PGPROC	   *proc;
+	const char *wait_event;
 	ArrayType  *blocking_pids_a;
 	int32	   *interesting_pids;
 	int32	   *blocking_pids;
@@ -43,6 +51,17 @@ pg_isolation_test_session_is_blocked(PG_FUNCTION_ARGS)
 	int			i,
 				j;
 
+	/* Check if blocked_pid is in injection_wait(). */
+	proc = BackendPidGetProc(blocked_pid);
+	if (proc == NULL)
+		PG_RETURN_BOOL(false);	/* session gone: definitely unblocked */
+	wait_event =
+		pgstat_get_wait_event(UINT32_ACCESS_ONCE(proc->wait_event_info));
+	if (wait_event && strncmp("INJECTION_POINT(",
+							  wait_event,
+							  strlen("INJECTION_POINT(")) == 0)
+		PG_RETURN_BOOL(true);
+
 	/* Validate the passed-in array */
 	Assert(ARR_ELEMTYPE(interesting_pids_a) == INT4OID);
 	if (array_contains_nulls(interesting_pids_a))
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 31bd787..2ffd2f7 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,6 +9,8 @@ PGFILEDESC = "injection_points - facility for injection points"
 REGRESS = injection_points
 REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
+ISOLATION = inplace
+
 # The injection points are cluster-wide, so disable installcheck
 NO_INSTALLCHECK = 1
 
diff --git a/src/test/modules/injection_points/expected/inplace.out b/src/test/modules/injection_points/expected/inplace.out
new file mode 100644
index 0000000..123f45a
--- /dev/null
+++ b/src/test/modules/injection_points/expected/inplace.out
@@ -0,0 +1,43 @@
+Parsed test spec with 3 sessions
+
+starting permutation: vac1 grant2 vac3 mkrels3 read1
+mkrels
+------
+      
+(1 row)
+
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step vac1: VACUUM vactest.orig50;  -- wait during inplace update <waiting ...>
+step grant2: GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC;
+step vac3: VACUUM pg_class;
+step mkrels3: 
+	SELECT vactest.mkrels('intruder', 1, 100);  -- repopulate LP_UNUSED
+	SELECT injection_points_detach('inplace-before-pin');
+	SELECT injection_points_wakeup('inplace-before-pin');
+
+mkrels
+------
+      
+(1 row)
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step vac1: <... completed>
+step read1: 
+	REINDEX TABLE pg_class;  -- look for duplicates
+	SELECT reltuples = -1 AS reltuples_unknown
+	FROM pg_class WHERE oid = 'vactest.orig50'::regclass;
+
+ERROR:  could not create unique index "pg_class_oid_index"
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 5c44625..4193061 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -200,6 +200,7 @@ injection_notice(const char *name, const void *private_data)
 void
 injection_wait(const char *name, const void *private_data)
 {
+	char		event_name[NAMEDATALEN];
 	uint32		old_wait_counts = 0;
 	int			index = -1;
 	uint32		injection_wait_event = 0;
@@ -212,11 +213,11 @@ injection_wait(const char *name, const void *private_data)
 		return;
 
 	/*
-	 * Use the injection point name for this custom wait event.  Note that
-	 * this custom wait event name is not released, but we don't care much for
-	 * testing as this should be short-lived.
+	 * Note that this custom wait event name is not released, but we don't
+	 * care much for testing as this should be short-lived.
 	 */
-	injection_wait_event = WaitEventExtensionNew(name);
+	snprintf(event_name, sizeof(event_name), "INJECTION_POINT(%s)", name);
+	injection_wait_event = WaitEventExtensionNew(event_name);
 
 	/*
 	 * Find a free slot to wait for, and register this injection point's name.
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 8e1b5b4..3c23c14 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -37,4 +37,9 @@ tests += {
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
   },
+  'isolation': {
+    'specs': [
+      'inplace',
+    ],
+  },
 }
diff --git a/src/test/modules/injection_points/specs/inplace.spec b/src/test/modules/injection_points/specs/inplace.spec
new file mode 100644
index 0000000..e957713
--- /dev/null
+++ b/src/test/modules/injection_points/specs/inplace.spec
@@ -0,0 +1,83 @@
+# Test race conditions involving:
+# - s1: VACUUM inplace-updating a pg_class row
+# - s2: GRANT/REVOKE making pg_class rows dead
+# - s3: "VACUUM pg_class" making dead rows LP_UNUSED; DDL reusing them
+
+# Need GRANT to make a non-HOT update.  Otherwise, "VACUUM pg_class" would
+# leave an LP_REDIRECT that persists.  To get non-HOT, make rels so the
+# pg_class row for vactest.orig50 is on a filled page (assuming BLCKSZ=8192).
+# Just to save on filesystem syscalls, use relkind=c for every other rel.
+setup
+{
+	CREATE EXTENSION injection_points;
+	CREATE SCHEMA vactest;
+	CREATE FUNCTION vactest.mkrels(text, int, int) RETURNS void
+		LANGUAGE plpgsql SET search_path = vactest AS $$
+	DECLARE
+		tname text;
+	BEGIN
+		FOR i in $2 .. $3 LOOP
+			tname := $1 || i;
+			EXECUTE FORMAT('CREATE TYPE ' || tname || ' AS ()');
+			RAISE DEBUG '% at %', tname, ctid
+				FROM pg_class WHERE oid = tname::regclass;
+		END LOOP;
+	END
+	$$;
+}
+setup	{ VACUUM FULL pg_class;  -- reduce free space }
+setup
+{
+	SELECT vactest.mkrels('orig', 1, 49);
+	CREATE TABLE vactest.orig50 ();
+	SELECT vactest.mkrels('orig', 51, 100);
+}
+
+# XXX DROP causes an assertion failure; adopt DROP once fixed
+teardown
+{
+	--DROP SCHEMA vactest CASCADE;
+	DO $$BEGIN EXECUTE 'ALTER SCHEMA vactest RENAME TO schema' || oid FROM pg_namespace where nspname = 'vactest'; END$$;
+	DROP EXTENSION injection_points;
+}
+
+# Wait during inplace update, in a VACUUM of vactest.orig50.
+session s1
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('inplace-before-pin', 'wait');
+}
+step vac1	{ VACUUM vactest.orig50;  -- wait during inplace update }
+# One bug scenario leaves two live pg_class tuples for vactest.orig50 and zero
+# live tuples for one of the "intruder" rels.  REINDEX observes the duplicate.
+step read1	{
+	REINDEX TABLE pg_class;  -- look for duplicates
+	SELECT reltuples = -1 AS reltuples_unknown
+	FROM pg_class WHERE oid = 'vactest.orig50'::regclass;
+}
+
+
+# Transactional updates of the tuple vac1 is waiting to inplace-update.
+session s2
+step grant2		{ GRANT SELECT ON TABLE vactest.orig50 TO PUBLIC; }
+
+
+# Non-blocking actions.
+session s3
+step vac3		{ VACUUM pg_class; }
+# Reuse the lp that vac1 is waiting to change.  I've observed reuse at the 1st
+# or 18th CREATE, so create excess.
+step mkrels3	{
+	SELECT vactest.mkrels('intruder', 1, 100);  -- repopulate LP_UNUSED
+	SELECT injection_points_detach('inplace-before-pin');
+	SELECT injection_points_wakeup('inplace-before-pin');
+}
+
+
+# XXX extant bug
+permutation
+	vac1(mkrels3)	# reads pg_class tuple T0 for vactest.orig50, xmax invalid
+	grant2			# T0 becomes eligible for pruning, T1 is successor
+	vac3			# T0 becomes LP_UNUSED
+	mkrels3			# T0 reused; vac1 wakes and overwrites the reused T0
+	read1
diff --git a/src/test/modules/test_misc/t/005_timeouts.pl b/src/test/modules/test_misc/t/005_timeouts.pl
index a792610..18c800c 100644
--- a/src/test/modules/test_misc/t/005_timeouts.pl
+++ b/src/test/modules/test_misc/t/005_timeouts.pl
@@ -50,7 +50,8 @@ $psql_session->query_until(
 
 # Wait until the backend enters the timeout injection point. Will get an error
 # here if anything goes wrong.
-$node->wait_for_event('client backend', 'transaction-timeout');
+$node->wait_for_event('client backend',
+	'INJECTION_POINT(transaction-timeout)');
 
 my $log_offset = -s $node->logfile;
 
@@ -86,7 +87,7 @@ $psql_session->query_until(
 
 # Wait until the backend enters the timeout injection point.
 $node->wait_for_event('client backend',
-	'idle-in-transaction-session-timeout');
+	'INJECTION_POINT(idle-in-transaction-session-timeout)');
 
 $log_offset = -s $node->logfile;
 
@@ -116,7 +117,8 @@ $psql_session->query_until(
 ));
 
 # Wait until the backend enters the timeout injection point.
-$node->wait_for_event('client backend', 'idle-session-timeout');
+$node->wait_for_event('client backend',
+	'INJECTION_POINT(idle-session-timeout)');
 
 $log_offset = -s $node->logfile;
 
diff --git a/src/test/recovery/t/041_checkpoint_at_promote.pl b/src/test/recovery/t/041_checkpoint_at_promote.pl
index 7c30731..538facb 100644
--- a/src/test/recovery/t/041_checkpoint_at_promote.pl
+++ b/src/test/recovery/t/041_checkpoint_at_promote.pl
@@ -78,7 +78,8 @@ $node_primary->wait_for_replay_catchup($node_standby);
 
 # Wait until the checkpointer is in the middle of the restart point
 # processing.
-$node_standby->wait_for_event('checkpointer', 'create-restart-point');
+$node_standby->wait_for_event('checkpointer',
+	'INJECTION_POINT(create-restart-point)');
 
 # Check the logs that the restart point has started on standby.  This is
 # optional, but let's be sure.