Fix deadlock when ddl gets triggered during autovacuum

.github/workflows/ci.yml

@@ -152,6 +152,12 @@ jobs:
           make PG_CONFIG=$PG_CONFIG
           sudo make PG_CONFIG=$PG_CONFIG install
 
+      # Separate module, never installed: catalog_ael_boundary loads it into
+      # the *source* cluster to reproduce VACUUM's lock lifetime deterministically
+      - name: Build walshadow test-only PG module
+        working-directory: walshadow/pgext/test
+        run: make PG_CONFIG=/usr/lib/postgresql/${{ matrix.pg-major }}/bin/pg_config
+
       # Each e2e test stages a source + shadow PG cluster on disk; the
       # runner's root fs is small, so point TMPDIR at the larger /mnt volume.
       - name: Set TMPDIR to /mnt

pgext/overlay.c

@@ -10,9 +10,16 @@
  *
  * Never opens the target relation. The replaying transaction holds
  * AccessExclusiveLock on it and standby lock replay is driven by the startup
- * process, so relation_open would block against recovery. Catalogs only, and
- * standby lock replay records AccessExclusiveLocks alone, so AccessShareLock
- * on a catalog never conflicts with the DDL in flight.
+ * process, so relation_open would block against recovery. Catalogs only.
+ *
+ * AccessShareLock on a catalog does not conflict with the DDL in flight, but
+ * that is not the only lock replay can be holding: any unrelated source
+ * transaction that took AccessExclusiveLock on the catalog itself — VACUUM
+ * truncating a bloated pg_type is the one seen in practice — leaves the
+ * startup process holding it until that transaction's commit record is
+ * replayed, and the caller is withholding exactly that record. A pinned scan
+ * therefore takes the lock only if it is free, and reads without it otherwise;
+ * see `ws_overlay_scan`.
  *
  * An invalid top xid asks the same scan for the committed view, which is what
  * the daemon captures at a catalog commit. Same projections, same assembly on
@@ -36,6 +43,7 @@
 #include "catalog/pg_namespace.h"
 #include "catalog/pg_type.h"
 #include "libpq/pqformat.h"
+#include "storage/lmgr.h"
 #include "storage/procarray.h"
 #include "utils/fmgroids.h"
 #include "utils/fmgrprotos.h"
@@ -412,12 +420,78 @@ ws_scan_emit(Relation rel, const WsCatalogPlan *plan, Oid key,
 	systable_endscan(scan);
 }
 
+/* qsort/bsearch comparator over Oid */
+static int
+ws_oid_cmp(const void *a, const void *b)
+{
+	Oid			x = *(const Oid *) a;
+	Oid			y = *(const Oid *) b;
+
+	if (x < y)
+		return -1;
+	return (x > y) ? 1 : 0;
+}
+
+/*
+ * One heap pass, emitting rows whose key attribute is in `sorted` (or every
+ * row when `nsorted` is 0). Used only when the catalog's lock was unavailable:
+ * `systable_beginscan` with an index would `index_open` it under
+ * AccessShareLock, which is the wait this path exists to avoid, so the index
+ * is skipped and the oid list is applied to the heap tuple instead.
+ *
+ * `scanned` counts rows that passed the oid filter, so it stays comparable to
+ * the indexed path.
+ */
+static void
+ws_scan_emit_lockfree(Relation rel, const WsCatalogPlan *plan,
+					  const Oid *sorted, int nsorted, TransactionId top,
+					  StringInfo out, WsScanStats *stats)
+{
+	SysScanDesc scan;
+	ScanKeyData skey[1];
+	int			nkeys = 0;
+	HeapTuple	tup;
+	TupleDesc	desc = RelationGetDescr(rel);
+	bool		scoped = nsorted > 0;
+
+	if (plan->min_attnum != 0)
+		ScanKeyInit(&skey[nkeys++], Anum_pg_attribute_attnum,
+					BTGreaterEqualStrategyNumber, F_INT2GE,
+					Int16GetDatum(plan->min_attnum));
+
+	scan = systable_beginscan(rel, InvalidOid, false, SnapshotAny, nkeys, skey);
+	while (HeapTupleIsValid(tup = systable_getnext(scan)))
+	{
+		if (scoped)
+		{
+			Datum		key;
+			bool		isnull;
+			Oid			keyoid;
+
+			key = heap_getattr(tup, plan->keyattno, desc, &isnull);
+			if (isnull)
+				continue;
+			keyoid = DatumGetObjectId(key);
+			if (bsearch(&keyoid, sorted, (size_t) nsorted, sizeof(Oid),
+						ws_oid_cmp) == NULL)
+				continue;
+		}
+		stats->scanned++;
+		if (!ws_tuple_visible(tup->t_data, top, scoped, stats))
+			continue;
+		stats->emitted++;
+		plan->emit(out, tup, desc);
+	}
+	systable_endscan(scan);
+}
+
 void
 ws_overlay_scan(WsCatalog cat, TransactionId top, const Oid *oids, int noids,
-				StringInfo out, WsScanStats *stats)
+				WsScanLock lock, StringInfo out, WsScanStats *stats)
 {
 	WsCatalogPlan plan;
 	bool		rel_scoped;
+	bool		locked;
 	Relation	rel;
 
 	if (!ws_catalog_plan(cat, &plan))
@@ -429,9 +503,50 @@ ws_overlay_scan(WsCatalog cat, TransactionId top, const Oid *oids, int noids,
 	 * pg_namespace and pg_type have. The lock argument comes with the list, so
 	 * losing the list loses the argument too */
 	rel_scoped = AttributeNumberIsValid(plan.keyattno) && noids > 0;
-	rel = table_open(plan.relid, AccessShareLock);
 
-	if (rel_scoped)
+	/*
+	 * See WsScanLock. A caller that named a replay position never waits here:
+	 * the release for a lock replay is holding can be in the WAL that caller
+	 * is withholding, so waiting deadlocks the daemon against its own shadow.
+	 * Reading without the lock is licensed only once that position is known to
+	 * be where replay is, which is what holds these pages still — together
+	 * with the shadow being read-only, so no local backend can write either.
+	 */
+	if (lock == WS_SCAN_LOCK_WAIT)
+	{
+		LockRelationOid(plan.relid, AccessShareLock);
+		locked = true;
+	}
+	else
+	{
+		locked = ConditionalLockRelationOid(plan.relid, AccessShareLock);
+		if (!locked && lock == WS_SCAN_LOCK_NOWAIT)
+			ereport(ERROR,
+					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
+					 errmsg("walshadow: %u is locked and replay is not at the position the scan named",
+							plan.relid),
+					 errdetail("Reading without the lock needs that position to hold; waiting for it can deadlock against withheld WAL.")));
+		if (!locked)
+			elog(DEBUG1,
+				 "walshadow: pinned scan of %u read without AccessShareLock",
+				 plan.relid);
+	}
+	rel = table_open(plan.relid, NoLock);
+
+	if (!locked)
+	{
+		Oid		   *sorted = NULL;
+
+		if (rel_scoped)
+		{
+			sorted = palloc_array(Oid, noids);
+			memcpy(sorted, oids, sizeof(Oid) * (size_t) noids);
+			qsort(sorted, (size_t) noids, sizeof(Oid), ws_oid_cmp);
+		}
+		ws_scan_emit_lockfree(rel, &plan, sorted, rel_scoped ? noids : 0,
+							  top, out, stats);
+	}
+	else if (rel_scoped)
 	{
 		int			i;
 
@@ -441,5 +556,7 @@ ws_overlay_scan(WsCatalog cat, TransactionId top, const Oid *oids, int noids,
 	else
 		ws_scan_emit(rel, &plan, InvalidOid, top, false, out, stats);
 
-	table_close(rel, AccessShareLock);
+	table_close(rel, NoLock);
+	if (locked)
+		UnlockRelationOid(plan.relid, AccessShareLock);
 }

pgext/test/Makefile

@@ -0,0 +1,8 @@
+# Test-only loadable module; see wstest.c. Built by the integration tests
+# that need VACUUM's lock lifetime without waiting on autovacuum.
+#
+#   make -C pgext/test
+MODULES = wstest
+PG_CONFIG ?= pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)

pgext/test/wstest.c

@@ -0,0 +1,30 @@
+/*
+ * Test-only helper. Not part of the walshadow module: built and loaded only
+ * by the daemon's integration tests, never by a deployed shadow.
+ *
+ * Reproduces VACUUM's lock lifetime (vacuumlazy.c, lazy_truncate_heap):
+ * acquire AccessExclusiveLock on a relation, then release it source-side
+ * while the surrounding transaction stays open. The acquire emits
+ * XLOG_STANDBY_LOCK; the release emits nothing. A standby therefore keeps
+ * the replayed lock until the transaction's commit record arrives, which is
+ * the asymmetry tests need to drive deterministically.
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "storage/lmgr.h"
+
+PG_MODULE_MAGIC;
+
+PG_FUNCTION_INFO_V1(ws_test_lock_unlock_relation);
+
+Datum
+ws_test_lock_unlock_relation(PG_FUNCTION_ARGS)
+{
+	Oid			relid = PG_GETARG_OID(0);
+
+	LockRelationOid(relid, AccessExclusiveLock);
+	UnlockRelationOid(relid, AccessExclusiveLock);
+
+	PG_RETURN_VOID();
+}

pgext/walshadow.h

@@ -8,7 +8,7 @@
 
 /* Bumped when a request or response layout changes, or when an op's reading
  * of an unchanged layout changes */
-#define WS_PROTO_VERSION		2
+#define WS_PROTO_VERSION		3
 /* Bumped when any catalog projection changes shape */
 #define WS_PROJECTION_VERSION	1
 
@@ -61,12 +61,34 @@ typedef struct WsScanStats
 
 extern int	ws_overlay_ncols(WsCatalog cat);
 
+/*
+ * How a scan may treat the target catalog's lock.
+ *
+ * A caller that named a replay position has withheld every successor byte to
+ * park the shadow there, so a lock this scan waits for can have its release
+ * sitting in that withheld WAL — waiting deadlocks the daemon against its own
+ * shadow. Such callers therefore never wait. Reading *without* the lock needs
+ * more than that: the position the caller named must be where replay actually
+ * is, which is what makes the pages stable.
+ */
+typedef enum WsScanLock
+{
+	/* No position named. Ordinary locking, and it may wait */
+	WS_SCAN_LOCK_WAIT = 0,
+	/* Position named but replay is elsewhere, so the caller's guarantee does
+	 * not hold: take the lock if free, error rather than wait or read past it */
+	WS_SCAN_LOCK_NOWAIT = 1,
+	/* Position named and verified: take the lock if free, read without it
+	 * otherwise */
+	WS_SCAN_LOCK_PINNED = 2,
+} WsScanLock;
+
 /*
  * `top` invalid reads the committed view; an empty `oids` reads the whole
  * catalog, which is the only mode pg_namespace and pg_type have.
  */
 extern void ws_overlay_scan(WsCatalog cat, TransactionId top,
-							const Oid *oids, int noids,
+							const Oid *oids, int noids, WsScanLock lock,
 							StringInfo out, WsScanStats *stats);
 
 #endif							/* WALSHADOW_H */

pgext/worker.c

@@ -312,13 +312,16 @@ ws_handle_scan(StringInfo req, StringInfo resp)
 {
 	WsCatalog	cat = (WsCatalog) pq_getmsgbyte(req);
 	TransactionId top = (TransactionId) pq_getmsgint(req, 4);
+	/* Replay position the caller parked at, 0 when it has not parked one */
+	uint64		boundary = pq_getmsgint64(req);
 	uint32		noids = pq_getmsgint(req, 4);
 	int			ncols = ws_overlay_ncols(cat);
 	Oid		   *oids = NULL;
 	StringInfoData rows;
 	WsScanStats stats = {0, 0, 0};
 	uint64		lsn_start;
 	uint64		lsn_end;
+	WsScanLock	lock;
 	uint32		i;
 
 	if (ncols < 0)
@@ -340,12 +343,22 @@ ws_handle_scan(StringInfo req, StringInfo resp)
 
 	initStringInfo(&rows);
 	/*
-	 * Caller asserts both LSNs equal the boundary it parked replay at. Equal
-	 * but wrong is impossible: replay cannot rewind, and the daemon holds the
-	 * successor bytes.
+	 * Both LSNs go back for the caller to check against the boundary it parked
+	 * replay at. Equal but wrong is impossible: replay cannot rewind, and the
+	 * daemon holds the successor bytes.
+	 *
+	 * The pre-scan sample also decides how the scan may treat the catalog's
+	 * lock: reading without it needs replay to actually be where the caller
+	 * says, and that cannot be judged from the response.
 	 */
 	lsn_start = (uint64) GetXLogReplayRecPtr(NULL);
-	ws_overlay_scan(cat, top, oids, (int) noids, &rows, &stats);
+	if (boundary == 0)
+		lock = WS_SCAN_LOCK_WAIT;
+	else if (lsn_start == boundary)
+		lock = WS_SCAN_LOCK_PINNED;
+	else
+		lock = WS_SCAN_LOCK_NOWAIT;
+	ws_overlay_scan(cat, top, oids, (int) noids, lock, &rows, &stats);
 	lsn_end = (uint64) GetXLogReplayRecPtr(NULL);
 
 	pq_sendbyte(resp, WS_STATUS_OK);