v7-0003-DROP-SUBSCRIPTION-handle-errors-from-fdwconnectio.patch

From fc35789170a27db04af0a650058bbe81a546a58f Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 14 May 2026 10:59:53 -0700
Subject: [PATCH v7 3/3] DROP SUBSCRIPTION: handle errors from fdwconnection.

Previously, when constructing the conninfo for a subscription using a
server, errors from ForeignServerConnectionString() were raised
immediately. That could cause the drop to fail if there were leftover
tablesync slots, even if slot_name was set to NONE. It also lost the
helpful HINT when slot_name was defined. ACL errors were handled, but
arbitrary errors from fdwconnection were not.

Fix by using a subtransaction to capture errors consistently and
either report with ReportSlotConnectionError() if slot_name is defined
or succeed silently if slot_name=NONE.

Reported-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com
---
 src/backend/commands/subscriptioncmds.c    | 67 +++++++++++++++-------
 src/test/regress/expected/subscription.out |  3 +-
 2 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index ee06a726f42..137bfa1a9b0 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -54,6 +54,7 @@
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/pg_lsn.h"
+#include "utils/resowner.h"
 #include "utils/syscache.h"
 
 /*
@@ -2264,37 +2265,63 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
  * Construct conninfo from a subscription's server. Like libpqrcv_connect(),
  * if an error occurs, set *err to the error message and return NULL.
  *
- * However, failures in ForeignServerConnectionString() may ereport(ERROR),
- * and (also like libpqrcv_connect) it's not worth adding the machinery to
- * pass all of those back to the caller just to cover this one case.
+ * Uses a subtransaction so that we can catch arbitrary errors from
+ * ForeignServerConnectionString().
  */
 static char *
 construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
 {
-	AclResult	aclresult;
-	ForeignServer *server;
+	MemoryContext oldcxt = CurrentMemoryContext;
+	ResourceOwner oldresowner = CurrentResourceOwner;
+	ErrorData  *edata = NULL;
+	char	   *conninfo = NULL;
 
 	*err = NULL;
 
-	server = GetForeignServer(subserver);
+	BeginInternalSubTransaction(NULL);
+	MemoryContextSwitchTo(oldcxt);
 
-	aclresult = object_aclcheck(ForeignServerRelationId, subserver,
-								subowner, ACL_USAGE);
-	if (aclresult != ACLCHECK_OK)
+	PG_TRY();
 	{
-		/*
-		 * Unable to generate connection string because permissions on the
-		 * foreign server have been removed. Follow the same logic as an
-		 * unusable subconninfo (which will result in an ERROR later unless
-		 * slot_name = NONE).
-		 */
-		*err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
-						GetUserNameFromId(subowner, false),
-						server->servername);
-		return NULL;
+		AclResult	aclresult;
+		ForeignServer *server;
+
+		server = GetForeignServer(subserver);
+		aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+									subowner, ACL_USAGE);
+		if (aclresult != ACLCHECK_OK)
+			ereport(ERROR,
+					errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+					errmsg("subscription owner \"%s\" does not have permission on foreign server \"%s\"",
+						   GetUserNameFromId(subowner, false),
+						   server->servername));
+
+		conninfo = ForeignServerConnectionString(subowner, server);
+		Assert(conninfo != NULL);
+		ReleaseCurrentSubTransaction();
+		MemoryContextSwitchTo(oldcxt);
+		CurrentResourceOwner = oldresowner;
+	}
+	PG_CATCH();
+	{
+		MemoryContextSwitchTo(oldcxt);
+		edata = CopyErrorData();
+		FlushErrorState();
+		RollbackAndReleaseCurrentSubTransaction();
+		MemoryContextSwitchTo(oldcxt);
+		CurrentResourceOwner = oldresowner;
+
+		conninfo = NULL;
+	}
+	PG_END_TRY();
+
+	if (!conninfo)
+	{
+		*err = pstrdup(edata->message);
+		FreeErrorData(edata);
 	}
 
-	return ForeignServerConnectionString(subowner, server);
+	return conninfo;
 }
 
 /*
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 8dbfac66326..1a4c4d6392e 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -215,7 +215,8 @@ ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist pass
 ABORT;
 -- fails, cannot drop slot
 DROP SUBSCRIPTION regress_testsub6;
-ERROR:  user mapping not found for user "regress_subscription_user3", server "test_server"
+ERROR:  could not connect to publisher when attempting to drop replication slot "dummy": user mapping not found for user "regress_subscription_user3", server "test_server"
+HINT:  Use ALTER SUBSCRIPTION ... DISABLE to disable the subscription, and then use ALTER SUBSCRIPTION ... SET (slot_name = NONE) to disassociate it from the slot.
 ALTER SUBSCRIPTION regress_testsub6 DISABLE;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
-- 
2.43.0