Fix synchronized_standby_slots GUC check hook

alvherre · alvherre · commit 9abdc1841e16 · 2024-12-03T17:50:57.000+01:00
The validate_sync_standby_slots subroutine requires an LWLock, so it cannot run in processes without PGPROC; skip it there to avoid a crash. This replaces the current test for ReplicationSlotCtl being not null, which appears to be a solution for the same problem but less general. I also rewrote a related comment that mentioned ReplicationSlotCtl in StandbySlotsHaveCaughtup. This code came in with commit bf279dd; backpatch to 17. Reported-by: Gabriele Bartolini <gabriele.bartolini@enterprisedb.com> Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com> Discussion: https://postgr.es/m/202411281216.sutbxtr6idnn@alvherre.pgsql
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
@@ -2428,18 +2428,15 @@ validate_sync_standby_slots(char *rawname, List **elemlist)
 	{
 		GUC_check_errdetail("List syntax is invalid.");
 	}
-	else if (!ReplicationSlotCtl)
+	else if (MyProc)
 	{
 		/*
-		 * We cannot validate the replication slot if the replication slots'
-		 * data has not been initialized. This is ok as we will anyway
-		 * validate the specified slot when waiting for them to catch up. See
-		 * StandbySlotsHaveCaughtup() for details.
+		 * Check that each specified slot exist and is physical.
+		 *
+		 * Because we need an LWLock, we cannot do this on processes without a
+		 * PGPROC, so we skip it there; but see comments in
+		 * StandbySlotsHaveCaughtup() as to why that's not a problem.
 		 */
-	}
-	else
-	{
-		/* Check that the specified slots exist and are logical slots */
 		LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
 
 		foreach_ptr(char, name, *elemlist)
@@ -2621,18 +2618,18 @@ StandbySlotsHaveCaughtup(XLogRecPtr wait_for_lsn, int elevel)
 
 		slot = SearchNamedReplicationSlot(name, false);
 
+		/*
+		 * If a slot name provided in synchronized_standby_slots does not
+		 * exist, report a message and exit the loop.
+		 *
+		 * Though validate_sync_standby_slots (the GUC check_hook) tries to
+		 * avoid this, it can nonetheless happen because the user can specify
+		 * a nonexistent slot name before server startup. That function cannot
+		 * validate such a slot during startup, as ReplicationSlotCtl is not
+		 * initialized by then.  Also, the user might have dropped one slot.
+		 */
 		if (!slot)
 		{
-			/*
-			 * If a slot name provided in synchronized_standby_slots does not
-			 * exist, report a message and exit the loop. A user can specify a
-			 * slot name that does not exist just before the server startup.
-			 * The GUC check_hook(validate_sync_standby_slots) cannot validate
-			 * such a slot during startup as the ReplicationSlotCtl shared
-			 * memory is not initialized at that time. It is also possible for
-			 * a user to drop the slot in synchronized_standby_slots
-			 * afterwards.
-			 */
 			ereport(elevel,
 					errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					errmsg("replication slot \"%s\" specified in parameter \"%s\" does not exist",
@@ -2644,16 +2641,9 @@ StandbySlotsHaveCaughtup(XLogRecPtr wait_for_lsn, int elevel)
 			break;
 		}
 
+		/* Same as above: if a slot is not physical, exit the loop. */
 		if (SlotIsLogical(slot))
 		{
-			/*
-			 * If a logical slot name is provided in
-			 * synchronized_standby_slots, report a message and exit the loop.
-			 * Similar to the non-existent case, a user can specify a logical
-			 * slot name in synchronized_standby_slots before the server
-			 * startup, or drop an existing physical slot and recreate a
-			 * logical slot with the same name.
-			 */
 			ereport(elevel,
 					errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 					errmsg("cannot specify logical replication slot \"%s\" in parameter \"%s\"",
