Subject: Bug in ALTER SUBSCRIPTION ... SERVER / ... CONNECTION with broken old server

From: Chao Li <[email-masked]>
Date: Wed, 22 Apr 2026 09:51:36 +0800

Hi,

I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and found an issue: once the old server becomes broken, the subscription cannot be recovered by switching it to a good server.

Here is a repro:
```
evantest=# CREATE EXTENSION postgres_fdw;
CREATE EXTENSION

# Create two servers
evantest=# CREATE SERVER old_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE SERVER new_srv FOREIGN DATA WRAPPER postgres_fdw
evantest-# OPTIONS (host '127.0.0.1', dbname 'postgres', port '5432');
CREATE SERVER
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
evantest=# CREATE USER MAPPING FOR CURRENT_USER SERVER new_srv
evantest-# OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING

# Add old_server to a subscription
evantest=# CREATE SUBSCRIPTION sub_bug SERVER old_srv
evantest-# PUBLICATION pub_does_not_matter
evantest-# WITH (connect = false, slot_name = NONE);
WARNING:  subscription was created, but is not connected
HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
CREATE SUBSCRIPTION

# Break old_srv
evantest=# DROP USER MAPPING FOR CURRENT_USER SERVER old_srv;
DROP USER MAPPING

# Fail to switch to a good server because old_srv is broken
evantest=# ALTER SUBSCRIPTION sub_bug SERVER new_srv;
ERROR:  user mapping not found for user "chaol", server "old_srv"
evantest=# ALTER SUBSCRIPTION sub_bug CONNECTION 'host=127.0.0.1 dbname=postgres port=5432 user=dummy password=dummy';
ERROR:  user mapping not found for user "chaol", server "old_srv"
```

In AlterSubscription(), this comment made me think this is a bug:
```
	/*
	 * Skip ACL checks on the subscription's foreign server, if any. If
	 * changing the server (or replacing it with a raw connection), then the
	 * old one will be removed anyway. If changing something unrelated,
	 * there's no need to do an additional ACL check here; that will be done
	 * by the subscription worker anyway.
	 */
	sub = GetSubscription(subid, false, false);
```

The comment explicitly says to skip ACL checks on the old server because it will be removed anyway.

But after looking into GetSubscription(), I found that when the aclcheck parameter is false, it still calls ForeignServerConnectionString(). I think that is the root cause of the bug.

To fix this, I worked out a solution that stores the server OID in Subscription, and only calls ForeignServerConnectionString()lazily when sub->conninfo is actually needed. I also added a new test case to cover this scenario. Without the fix, the new test fails.
See attached patch for details.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 22 Apr 2026 12:35:02 +0000

Dear Chao,

> I tested the v19 new feature CREATE SUBSCRIPTION ... SERVER yesterday, and
> found an issue: once the old server becomes broken, the subscription cannot be
> recovered by switching it to a good server.

Thanks for testing. I could reproduce the same issue. In addition to yours, I found
DROP SUBSCRIPTION cannot be done anymore. To switch the connection or drop it,
I had to create the same user mapping must be created again.

```
postgres=# DROP SUBSCRIPTION sub_bug ;
ERROR:  user mapping not found for user "postgres", server "old_srv"
postgres=# CREATE USER MAPPING FOR CURRENT_USER SERVER old_srv
OPTIONS (user 'dummy', password 'dummy');
CREATE USER MAPPING
postgres=# DROP SUBSCRIPTION sub_bug ;
DROP SUBSCRIPTION
```

Before deep dive to your fix, I'm unclear why dropping the active USER MAPPING is
allowed. Personally, it should be avoided anyway. Do you know why it's not restricted?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Chao Li <[email-masked]>
Date: Thu, 23 Apr 2026 11:37:42 +0800

> On Apr 22, 2026, at 20:35, Hayato Kuroda (Fujitsu) <[email-masked]> wrote:
> 
> Dear Chao,
> 

Hi Hayato-san,

There is an existing test case in subscription.sql:
```
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
```

So, I guess that’s an intentional behavior. You have to fix the broken server or switch to a good one before dropping the subscription. That’s my understanding from the test cases.

Best regards,

---

From: Ajin Cherian <[email-masked]>
Date: Wed, 29 Apr 2026 14:44:47 +1000

> See attached patch for details.
Hi Li,

Thanks for the patch.
Some comments:

1.
             if (aclresult != ACLCHECK_OK)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                         errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
-                                GetUserNameFromId(subform->subowner, false),
-                                server->servername)));
+                        errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                        errmsg("subscription owner \"%s\" does not
have permission on foreign server \"%s\"",
+                               GetUserNameFromId(subform->subowner, false),
+                               server->servername));
+            sub->conninfo = ForeignServerConnectionString(subform->subowner,
+                                                          server);
         }

Add a new line before the call to ForeignServerConnectionString(),
also I think you should put the if condition within curly brackets
because it spans more than one line and might confuse developers while
adding new code.

2. I think you should add a comment in the function header above
GetSubscription() stating that if aclcheck is false, then the conninfo
will be set to null and users need to call GetSubscriptionConnInfo to
get the conninfo.

3.
 Datum
 test_fdw_connection(PG_FUNCTION_ARGS)
 {
+    GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
     PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist
user=doesnotexist password=secret"));
 }

Add a comment above this change describing why it's required.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Chao Li <[email-masked]>
Date: Thu, 30 Apr 2026 12:11:47 +0800

> On Apr 29, 2026, at 12:44, Ajin Cherian <[email-masked]> wrote:
> 

Thanks for your review.

> Add a new line before the call to ForeignServerConnectionString(),

Okay.

> because it spans more than one line and might confuse developers while

I don’t think curly brackets are needed as there is only one statement under the if though the statement spans multiple lines. There are plenty of examples in the existing code, for example:
```
	if (!wrconn)
		ereport(ERROR,
				errcode(ERRCODE_CONNECTION_FAILURE),
				errmsg("subscription \"%s\" could not connect to the publisher: %s",
					   sub->name, err));
```

> will be set to null and users need to call GetSubscriptionConnInfo to

Updated the header comment.

> Add a comment above this change describing why it's required.

Added the comment.

Best regards,

---

From: Ajin Cherian <[email-masked]>
Date: Fri, 1 May 2026 13:58:51 +1000

> Updated the header comment.

 /*
  * Fetch the subscription from the syscache.
+ *
+ * If missing_ok is false, throw an error if the subscription is not found.
+ * If true, return NULL in that case.
+ *
+ * If aclcheck is true, check whether the subscription owner has permission on
+ * the subscription's foreign server, and load the connection string from the
+ * foreign server. Later, GetSubscriptionConnInfo() should be called to get
+ * the connection string.
  */
 Subscription *
 GetSubscription(Oid subid, bool missing_ok, bool aclcheck)

I don't think this comment is right. If aclcheck is true, users need
not call GetSubscriptionConnInfo() to get the connection string, as it
is populated in this function itself. It is only if aclecheck is
false, do callers need to do that.
Isn't that the case? There are multiple places in the apply worker
where GetSubscription() is called with aclcheck is true and
GetSubscriptionConnInfo() is not called there.

regards,
Ajin Cherian
Fujitsu Australia

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 5 May 2026 21:53:20 +0100

Hello

- server = GetForeignServer(subform->subserver);
+ server = GetForeignServer(sub->serverid);

Couldn't we also move this inside the if?

+/*
+ * Return the subscription's connection string, loading it into the
+ * subscription memory context if necessary.
+ *
+ * GetSubscription must be called earlier to set sub->serverid, because ACL
+ * checks are performed there.
+ */
+char *
+GetSubscriptionConnInfo(Subscription *sub)

This is related to Ajin's comment earlier, the part about ACL check
seems incorrect to me.

---

From: Chao Li <[email-masked]>
Date: Wed, 6 May 2026 15:47:21 +0800

> On May 1, 2026, at 11:58, Ajin Cherian <[email-masked]> wrote:
> 

Hi Ajin,

Thank you for the comment. After rereading that part, I agree the wording is not clear.

What I meant is that GetSubscriptionConnInfo() is a safe accessor, if sub->conninfo has already been resolved, it just returns it; otherwise, it resolves it on demand. So the intended usage is that callers can consistently use GetSubscriptionConnInfo() when they need the connection string.

I also missed doing a broader search for direct uses of sub->conninfo and replacing them with GetSubscriptionConnInfo() where appropriate. Sorry about that.

I will address both issues in v3.

Best regards,

---

From: Chao Li <[email-masked]>
Date: Wed, 6 May 2026 15:57:51 +0800

> On May 6, 2026, at 04:53, Zsolt Parragi <[email-masked]> wrote:
> 
> Hello
> 

Ah, true. Both aclresult and server can be moved into the if.

> This is related to Ajin's comment earlier, the part about ACL check

Yes, see my reply to Ajin in the previous email.

PFA v3 - addressed Ajin and Zsolt’s comments.

Best regards,

---

From: Jeff Davis <[email-masked]>
Date: Fri, 08 May 2026 18:01:28 -0700

> PFA v3 - addressed Ajin and Zsolt’s comments.

Thank you for the report!

The proposed patch seems unnecessarily complex, though. It seems too
easy to add GetSubscriptionConninfo() in the wrong place and end up
with another problem that's not easily detected.

Can't we just do something like the attached? It's easy to explain at
the call site that, when changing to a different server or using
CONNECTION instead, that we don't need the old conninfo at all.

I included your test case in my patch, and it passes.

Also, Hayato Kuroda's report was an issue also because the error could
be thrown even if slotname was NULL. Patch attached for that, as well.
Thank you, also!

Regards,
	Jeff Davis

From 3430b5c0e29351ebc19c6763411ca1b29b401fec Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Fri, 8 May 2026 16:23:00 -0700
Subject: [PATCH v4 1/2] Avoid errors during ALTER SUBSCRIPTION.

Previously, when retrieving the old Subscription object, constructing
the conninfo could encounter an error during
ForeignServerConnectionString(). ACL errors were handled properly, but
other errors could interfere with a user fixing the problem with ALTER
SUBSCRIPTION.

Add parameter to GetSubscrpition() that allows the caller to bypass
generating the conninfo, which is unneeded in these cases because it's
about to be replaced anyway.

Test case from Chao Li.

Reported-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/catalog/pg_subscription.c      | 64 ++++++++++++----------
 src/backend/commands/subscriptioncmds.c    | 22 ++++++--
 src/backend/replication/logical/worker.c   |  4 +-
 src/include/catalog/pg_subscription.h      |  3 +-
 src/test/regress/expected/subscription.out | 16 ++++++
 src/test/regress/regress.c                 |  3 +
 src/test/regress/sql/subscription.sql      | 19 +++++++
 7 files changed, 92 insertions(+), 39 deletions(-)

diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..79739f0354b 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -74,7 +74,8 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
  * Fetch the subscription from the syscache.
  */
 Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
+GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
+				bool conninfo_aclcheck)
 {
 	HeapTuple	tup;
 	Subscription *sub;
@@ -100,7 +101,7 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
 
 	subform = (Form_pg_subscription) GETSTRUCT(tup);
 
-	sub = palloc_object(Subscription);
+	sub = palloc0_object(Subscription);
 	sub->cxt = cxt;
 	sub->oid = subid;
 	sub->dbid = subform->subdbid;
@@ -120,37 +121,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
 	sub->retentionactive = subform->subretentionactive;
 
 	/* Get conninfo */
-	if (OidIsValid(subform->subserver))
+	if (conninfo_needed)
 	{
-		AclResult	aclresult;
-		ForeignServer *server;
-
-		server = GetForeignServer(subform->subserver);
-
-		/* recheck ACL if requested */
-		if (aclcheck)
+		if (OidIsValid(subform->subserver))
 		{
-			aclresult = object_aclcheck(ForeignServerRelationId,
-										subform->subserver,
-										subform->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(subform->subowner, false),
-								server->servername)));
+			AclResult	aclresult;
+			ForeignServer *server;
+
+			server = GetForeignServer(subform->subserver);
+
+			/* recheck ACL if requested */
+			if (conninfo_aclcheck)
+			{
+				aclresult = object_aclcheck(ForeignServerRelationId,
+											subform->subserver,
+											subform->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(subform->subowner, false),
+									server->servername)));
+			}
+
+			sub->conninfo = ForeignServerConnectionString(subform->subowner,
+														  server);
+		}
+		else
+		{
+			datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
+										   tup,
+										   Anum_pg_subscription_subconninfo);
+			sub->conninfo = TextDatumGetCString(datum);
 		}
-
-		sub->conninfo = ForeignServerConnectionString(subform->subowner,
-													  server);
-	}
-	else
-	{
-		datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
-									   tup,
-									   Anum_pg_subscription_subconninfo);
-		sub->conninfo = TextDatumGetCString(datum);
 	}
 
 	/* Get slotname */
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 1e10d9d9a58..21b27991d3c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1424,6 +1424,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	bool		update_failover = false;
 	bool		update_two_phase = false;
 	bool		check_pub_rdt = false;
+	bool		conninfo_needed = true;
 	bool		retain_dead_tuples;
 	int			max_retention;
 	bool		retention_active;
@@ -1454,13 +1455,22 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 					   stmt->subname);
 
 	/*
-	 * Skip ACL checks on the subscription's foreign server, if any. If
-	 * changing the server (or replacing it with a raw connection), then the
-	 * old one will be removed anyway. If changing something unrelated,
-	 * there's no need to do an additional ACL check here; that will be done
-	 * by the subscription worker anyway.
+	 * If we're replacing the connection information, we don't need the old
+	 * conninfo at all. Trying to construct it could encounter errors, which
+	 * would prevent the user from fixing the problem.
 	 */
-	sub = GetSubscription(subid, false, false);
+	if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+		stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+		conninfo_needed = false;
+
+	/*
+	 * Skip ACL checks when constructing the existing connection information;
+	 * the ACL check will be performed on the new connection information, if
+	 * any. If changing something unrelated to conninfo, there's no need to do
+	 * an additional ACL check on the foreign server here; that will be done
+	 * by the subscription worker.
+	 */
+	sub = GetSubscription(subid, false, conninfo_needed, false);
 
 	retain_dead_tuples = sub->retaindeadtuples;
 	origin = sub->origin;
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dd6fc38a41e..909bd47f0a9 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5059,7 +5059,7 @@ maybe_reread_subscription(void)
 		started_tx = true;
 	}
 
-	newsub = GetSubscription(MyLogicalRepWorker->subid, true, true);
+	newsub = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
 
 	if (newsub)
 	{
@@ -5808,7 +5808,7 @@ InitializeLogRepWorker(void)
 	LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid, 0,
 					 AccessShareLock);
 
-	MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true);
+	MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
 
 	if (MySubscription)
 	{
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..48944201889 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -213,7 +213,8 @@ typedef struct Subscription
 #endif							/* EXPOSE_TO_CLIENT_CODE */
 
 extern Subscription *GetSubscription(Oid subid, bool missing_ok,
-									 bool aclcheck);
+									 bool conninfo_needed,
+									 bool conninfo_aclcheck);
 extern void DisableSubscription(Oid subid);
 
 extern int	CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..128f44eaacd 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,32 @@ WARNING:  subscription was created, but is not connected
 HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
 DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+RESET SESSION AUTHORIZATION;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 -- fail, must connect but lacks USAGE on server, as well as user mapping
 DROP SUBSCRIPTION regress_testsub6;
 ERROR:  could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign 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.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6;
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
 DROP SERVER test_server;
 -- fail, FDW is dependent
 DROP FUNCTION test_fdw_connection(oid, oid, internal);
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
 #include "executor/executor.h"
 #include "executor/functions.h"
 #include "executor/spi.h"
+#include "foreign/foreign.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
 Datum
 test_fdw_connection(PG_FUNCTION_ARGS)
 {
+	/* Ensure the test fails if no valid user mapping exists. */
+	GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
 	PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
 }
 
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..c90f8a03ed8 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
 CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
 
 DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+RESET SESSION AUTHORIZATION;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
 RESET SESSION AUTHORIZATION;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,25 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
 -- fail, must connect but lacks USAGE on server, as well as user mapping
 DROP SUBSCRIPTION regress_testsub6;
 
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6;
 
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
 
+DROP SERVER test_server2;
 DROP SERVER test_server;
 
 -- fail, FDW is dependent
-- 
2.43.0

From 39b2a13ee7904256fc100a0f92104c056386712e Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Fri, 8 May 2026 17:16:50 -0700
Subject: [PATCH v4 2/2] Avoid errors during DROP SUBSCRIPTION.

Previously, an error during ForeignServerConnectionString() could
prevent a subscription from being dropped, even if slot_name was
NULL. ACL check errors were handled, but not other errors, such as a
dropped user mapping.

Change to only construct conninfo if slotname is defined. Errors
(aside from ACL check failures) will not benefit from the more helpful
ReportSlotConnectionError(), but at least they will not interfere with
a user fixing the problem.

Reported-by: Hayato Kuroda (Fujitsu) <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/subscriptioncmds.c | 71 +++++++++++++------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 21b27991d3c..498a102200e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2193,7 +2193,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 	Datum		datum;
 	bool		isnull;
 	char	   *subname;
-	char	   *conninfo;
+	char	   *conninfo = NULL;
 	char	   *slotname;
 	List	   *subworkers;
 	ListCell   *lc;
@@ -2256,39 +2256,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 								   Anum_pg_subscription_subname);
 	subname = pstrdup(NameStr(*DatumGetName(datum)));
 
-	/* Get conninfo */
-	if (OidIsValid(form->subserver))
-	{
-		AclResult	aclresult;
-		ForeignServer *server;
-
-		server = GetForeignServer(form->subserver);
-		aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
-									form->subowner, ACL_USAGE);
-		if (aclresult != ACLCHECK_OK)
-		{
-			/*
-			 * 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(form->subowner, false),
-						   server->servername);
-			conninfo = NULL;
-		}
-		else
-			conninfo = ForeignServerConnectionString(form->subowner,
-													 server);
-	}
-	else
-	{
-		datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
-									   Anum_pg_subscription_subconninfo);
-		conninfo = TextDatumGetCString(datum);
-	}
-
 	/* Get slotname */
 	datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
 							Anum_pg_subscription_subslotname, &isnull);
@@ -2297,6 +2264,42 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 	else
 		slotname = NULL;
 
+	/* conninfo only needed if slotname is valid */
+	if (slotname)
+	{
+		if (OidIsValid(form->subserver))
+		{
+			AclResult	aclresult;
+			ForeignServer *server;
+
+			server = GetForeignServer(form->subserver);
+			aclresult = object_aclcheck(ForeignServerRelationId, form->subserver,
+										form->subowner, ACL_USAGE);
+			if (aclresult != ACLCHECK_OK)
+			{
+				/*
+				 * Unable to generate connection string because permissions on
+				 * the foreign server have been removed. Follow the same logic
+				 * as an unusable subconninfo and ReportSlotConnectionError()
+				 * for a more helpful error.
+				 */
+				err = psprintf(_("subscription owner \"%s\" does not have permission on foreign server \"%s\""),
+							   GetUserNameFromId(form->subowner, false),
+							   server->servername);
+				conninfo = NULL;
+			}
+			else
+				conninfo = ForeignServerConnectionString(form->subowner,
+														 server);
+		}
+		else
+		{
+			datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
+										   Anum_pg_subscription_subconninfo);
+			conninfo = TextDatumGetCString(datum);
+		}
+	}
+
 	/*
 	 * Since dropping a replication slot is not transactional, the replication
 	 * slot stays dropped even if the transaction rolls back.  So we cannot
-- 
2.43.0

---

From: Chao Li <[email-masked]>
Date: Sat, 9 May 2026 11:08:28 +0800

> On May 9, 2026, at 09:01, Jeff Davis <[email-masked]> wrote:
> 
> <v4-0001-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v4-0002-Avoid-errors-during-DROP-SUBSCRIPTION.patch>

Ah, I see. You added a new conninfo_needed parameter to GetSubscription(), which separates the decision of building conninfo from the ACL check. Cool, I believe this is a better approach.

So 0001 looks good to me. nitpick is that conninfo_aclcheck is now only meaningful when conninfo_needed is true. I wonder if we should mention that briefly in the function header comment, or add an assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to avoid possible misuse of conninfo_aclcheck in the future.

For 0002, I have a doubt. Now conninfo is built only when slotname is not NULL. But after reading through DropSubscription(), I am not sure conninfo is strictly tied to slotname.

For example, this fast path returns only when both slotname is NULL and rstates is NIL:
```
	/*
	 * If there is no slot associated with the subscription, we can finish
	 * here.
	 */
	if (!slotname && rstates == NIL)
	{
		table_close(rel, NoLock);
		return;
	}
```

That seems to imply that even when slotname is NULL, rstates might still be not NIL.

Later, if conninfo is not NULL, the code connects to the publisher and does some cleanup work for tablesync slots:
```
	if (conninfo)
		wrconn = walrcv_connect(conninfo, true, true, must_use_password,
								subname, &err);
        ...
			/*
			 * Drop the tablesync slots associated with removed tables.
			 *
			 * For SYNCDONE/READY states, the tablesync slot is known to have
			 * already been dropped by the tablesync worker.
			 *
			 * For other states, there is no certainty, maybe the slot does
			 * not exist yet. Also, if we fail after removing some of the
			 * slots, next time, it will again try to drop already dropped
			 * slots and fail. For these reasons, we allow missing_ok = true
			 * for the drop.
			 */
			if (rstate->state != SUBREL_STATE_SYNCDONE)
			{
				char		syncslotname[NAMEDATALEN] = {0};

				ReplicationSlotNameForTablesync(subid, relid, syncslotname,
												sizeof(syncslotname));
				ReplicationSlotDropAtPubNode(wrconn, syncslotname, true);
			}
```

So with 0002, if slotname is NULL but rstates is not NIL, it looks possible that we no longer build conninfo and therefore skip the cleanup on the publisher side.

Best reagards,

---

From: Jeff Davis <[email-masked]>
Date: Thu, 14 May 2026 14:45:10 -0700

Hi,

I added a fix to the series: v5-0001 fixes check_pub_rdt for the
foreign server case.

On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
> Ah, I see. You added a new conninfo_needed parameter to
> GetSubscription(), which separates the decision of building conninfo
> from the ACL check. Cool, I believe this is a better approach.
> 
> So 0001 looks good to me. nitpick is that conninfo_aclcheck is now
> only meaningful when conninfo_needed is true. I wonder if we should
> mention that briefly in the function header comment, or add an
> assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to
> avoid possible misuse of conninfo_aclcheck in the future.

I refactored the fix in v5-0002 to do this in a more organized way: now
all option parsing happens first, so I can more precisely decide which
paths need conninfo and which ones don't.

> So with 0002, if slotname is NULL but rstates is not NIL, it looks
> possible that we no longer build conninfo and therefore skip the
> cleanup on the publisher side.

I separated this into two patches:

v5-0003 just moves the connection string building after the early exit,
so that if slotname is NONE and rstates is NIL, then it won't try to
build the connection string at all (and therefore won't get an error
while doing so).

v5-0004 fixes the remaining issue when slotname is NONE and rstates is
*not* NIL. It uses a subtransaction to catch the error, so that the
DROP TRANSACTION will still succeed even though it can't connect to the
publisher to drop the tablesync slots. This feels a bit over-
engineered, but it does maintain the expected behavior in this case. It
also routes errors inside of ForeignServerConnectionString() through
ReportSlotConnectionError(), which adds a helpful hint.

Regards,
	Jeff Davis

From f0edc45856a4c6d2ddb9f83d53bd1c50978801d9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Tue, 12 May 2026 14:57:31 -0700
Subject: [PATCH v5 1/4] Check retain_dead_tuples for ALTER SUBSCRIPTION ...
 SERVER.

Previously, the subscription setting retain_dead_tuples didn't cause
ALTER SUBSCRIPTION ... SERVER to check the publisher. And if the
publisher was checked for some other reason, then it would use the old
conninfo.

Fix ALTER SUBSCRIPTION ... SERVER to always check the publisher when
retain_dead_tuples is set, and to use the new connection info, like
ALTER SUBSCRIPTION ... CONNECTION.
---
 src/backend/commands/subscriptioncmds.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 7818f667edf..523959ba0ce 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1427,6 +1427,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	bool		retain_dead_tuples;
 	int			max_retention;
 	bool		retention_active;
+	char	   *new_conninfo = NULL;
 	char	   *origin;
 	Subscription *sub;
 	Form_pg_subscription form;
@@ -1810,7 +1811,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				ForeignServer *new_server;
 				ObjectAddress referenced;
 				AclResult	aclresult;
-				char	   *conninfo;
 
 				/*
 				 * Remove what was there before, either another foreign server
@@ -1846,13 +1846,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				/* make sure a user mapping exists */
 				GetUserMapping(form->subowner, new_server->serverid);
 
-				conninfo = ForeignServerConnectionString(form->subowner,
-														 new_server);
+				new_conninfo = ForeignServerConnectionString(form->subowner,
+															 new_server);
 
 				/* Load the library providing us libpq calls. */
 				load_file("libpqwalreceiver", false);
 				/* Check the connection info string. */
-				walrcv_check_conninfo(conninfo,
+				walrcv_check_conninfo(new_conninfo,
 									  sub->passwordrequired && !sub->ownersuperuser);
 
 				values[Anum_pg_subscription_subserver - 1] = ObjectIdGetDatum(new_server->serverid);
@@ -1863,6 +1863,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 				update_tuple = true;
 			}
+
+			/*
+			 * Since the remote server configuration might have changed,
+			 * perform a check to ensure it permits enabling
+			 * retain_dead_tuples.
+			 */
+			check_pub_rdt = sub->retaindeadtuples;
 			break;
 
 		case ALTER_SUBSCRIPTION_CONNECTION:
@@ -1877,10 +1884,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				replaces[Anum_pg_subscription_subserver - 1] = true;
 			}
 
+			new_conninfo = stmt->conninfo;
+
 			/* Load the library providing us libpq calls. */
 			load_file("libpqwalreceiver", false);
 			/* Check the connection info string. */
-			walrcv_check_conninfo(stmt->conninfo,
+			walrcv_check_conninfo(new_conninfo,
 								  sub->passwordrequired && !sub->ownersuperuser);
 
 			values[Anum_pg_subscription_subconninfo - 1] =
@@ -2129,7 +2138,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 		 * available.
 		 */
 		must_use_password = sub->passwordrequired && !sub->ownersuperuser;
-		wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+		wrconn = walrcv_connect(new_conninfo ? new_conninfo : sub->conninfo,
 								true, true, must_use_password, sub->name,
 								&err);
 		if (!wrconn)
-- 
2.43.0

From 6ab85d7b22204dd40562157096e6989f04932ae2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Tue, 12 May 2026 14:10:07 -0700
Subject: [PATCH v5 2/4] Avoid errors during ALTER SUBSCRIPTION.

Reported-by: Chao Li <[email-masked]>
Reviewed-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/catalog/pg_subscription.c      |  72 ++++++++------
 src/backend/commands/subscriptioncmds.c    | 110 +++++++++++++++------
 src/backend/replication/logical/worker.c   |   4 +-
 src/include/catalog/pg_subscription.h      |   3 +-
 src/test/regress/expected/subscription.out |  28 +++++-
 src/test/regress/regress.c                 |   3 +
 src/test/regress/sql/subscription.sql      |  34 ++++++-
 7 files changed, 181 insertions(+), 73 deletions(-)

diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..82fc91e0810 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -72,9 +72,15 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
 
 /*
  * Fetch the subscription from the syscache.
+ *
+ * If conninfo_needed is true, conninfo will be constructed, possibly
+ * encountering errors in ForeignServerConnectionString(). Callers not
+ * expecting such errors should pass false, in which case conninfo will be
+ * NULL.
  */
 Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
+GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
+				bool conninfo_aclcheck)
 {
 	HeapTuple	tup;
 	Subscription *sub;
@@ -84,6 +90,8 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
 	MemoryContext cxt;
 	MemoryContext oldcxt;
 
+	Assert(conninfo_needed || !conninfo_aclcheck);
+
 	tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
 
 	if (!HeapTupleIsValid(tup))
@@ -100,7 +108,7 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)

-	sub = palloc_object(Subscription);
+	sub = palloc0_object(Subscription);
 	sub->cxt = cxt;
 	sub->oid = subid;
 	sub->dbid = subform->subdbid;
@@ -119,38 +127,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
 	sub->maxretention = subform->submaxretention;
 	sub->retentionactive = subform->subretentionactive;
 
-	/* Get conninfo */
 
 	/* Get slotname */
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 523959ba0ce..01e992f656e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1420,6 +1420,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	Datum		values[Natts_pg_subscription];
 	HeapTuple	tup;
 	Oid			subid;
+	bool		conninfo_needed = true;
 	bool		update_tuple = false;
 	bool		update_failover = false;
 	bool		update_two_phase = false;
@@ -1454,14 +1455,88 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 		aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION,
 					   stmt->subname);
 
+	/* parse and check options */
+	switch (stmt->kind)
+	{
+		case ALTER_SUBSCRIPTION_OPTIONS:
+			supported_opts = (SUBOPT_SLOT_NAME |
+							  SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+							  SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
+							  SUBOPT_DISABLE_ON_ERR |
+							  SUBOPT_PASSWORD_REQUIRED |
+							  SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
+							  SUBOPT_RETAIN_DEAD_TUPLES |
+							  SUBOPT_MAX_RETENTION_DURATION |
+							  SUBOPT_WAL_RECEIVER_TIMEOUT |
+							  SUBOPT_ORIGIN);
+			break;
+
+		case ALTER_SUBSCRIPTION_ENABLED:
+			supported_opts = SUBOPT_ENABLED;
+			break;
+
+		case ALTER_SUBSCRIPTION_SET_PUBLICATION:
+			supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
+			break;
+
+		case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+		case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+			supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
+			break;
+
+		case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION:
+			supported_opts = SUBOPT_COPY_DATA;
+			break;
+
+		case ALTER_SUBSCRIPTION_SKIP:
+			supported_opts = SUBOPT_LSN;
+			break;
+
+		default:
+			supported_opts = 0;
+			break;
+	}
+
+	if (supported_opts > 0)
+		parse_subscription_options(pstate, stmt->options, supported_opts, &opts);
+
+	/*
+	 * Ensure that ALTER SUBSCRIPTION commands that could be used to fix a
+	 * broken connection or prepare to drop a broken subscription don't
+	 * attempt to construct the conninfo. Otherwise, we might encounter the
+	 * error the user is trying to fix.
+	 *
+	 * Specifically, ALTER SUBSCRIPTION ... SERVER, ALTER SUBSCRIPTION ...
+	 * CONNECTION, or ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+	 *
+	 * NB: if the user specifies multiple SET options, then we may still need
+	 * to construct conninfo even if slot_name is set to NONE.
+	 */
+	if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+		stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+	{
+		conninfo_needed = false;
+	}
+	else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS)
+	{
+		if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME) && !opts.slot_name)
+			conninfo_needed = false;
+
+		/* for these options, we still need conninfo */
+		if (IsSet(opts.specified_opts, SUBOPT_FAILOVER) ||
+			IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT) ||
+			IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES))
+			conninfo_needed = true;
+	}
+
 	/*
 	 * Skip ACL checks on the subscription's foreign server, if any. If
 	 * changing the server (or replacing it with a raw connection), then the
 	 * old one will be removed anyway. If changing something unrelated,
 	 * there's no need to do an additional ACL check here; that will be done
-	 * by the subscription worker anyway.
+	 * by the subscription worker.
 	 */
-	sub = GetSubscription(subid, false, false);
+	sub = GetSubscription(subid, false, conninfo_needed, false);
 
 	retain_dead_tuples = sub->retaindeadtuples;
 	origin = sub->origin;
@@ -1492,20 +1567,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	{
 		case ALTER_SUBSCRIPTION_OPTIONS:
 			{
-				supported_opts = (SUBOPT_SLOT_NAME |
-								  SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
-								  SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
-								  SUBOPT_DISABLE_ON_ERR |
-								  SUBOPT_PASSWORD_REQUIRED |
-								  SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
-								  SUBOPT_RETAIN_DEAD_TUPLES |
-								  SUBOPT_MAX_RETENTION_DURATION |
-								  SUBOPT_WAL_RECEIVER_TIMEOUT |
-								  SUBOPT_ORIGIN);
-
-				parse_subscription_options(pstate, stmt->options,
-										   supported_opts, &opts);
-
 				if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
 				{
 					/*
@@ -1769,8 +1830,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 		case ALTER_SUBSCRIPTION_ENABLED:
 			{
-				parse_subscription_options(pstate, stmt->options,
-										   SUBOPT_ENABLED, &opts);
 				Assert(IsSet(opts.specified_opts, SUBOPT_ENABLED));
 
 				if (!sub->slotname && opts.enabled)
@@ -1907,10 +1966,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 		case ALTER_SUBSCRIPTION_SET_PUBLICATION:
 			{
-				supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
-				parse_subscription_options(pstate, stmt->options,
-										   supported_opts, &opts);
-
 				values[Anum_pg_subscription_subpublications - 1] =
 					publicationListToArray(stmt->publication);
 				replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1954,10 +2009,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				List	   *publist;
 				bool		isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
 
-				supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
-				parse_subscription_options(pstate, stmt->options,
-										   supported_opts, &opts);
-
 				publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
 				values[Anum_pg_subscription_subpublications - 1] =
 					publicationListToArray(publist);
@@ -2015,9 +2066,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 							 errmsg("%s is not allowed for disabled subscriptions",
 									"ALTER SUBSCRIPTION ... REFRESH PUBLICATION")));
 
-				parse_subscription_options(pstate, stmt->options,
-										   SUBOPT_COPY_DATA, &opts);
-
 				/*
 				 * The subscription option "two_phase" requires that
 				 * replication has passed the initial table synchronization
@@ -2063,8 +2111,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 		case ALTER_SUBSCRIPTION_SKIP:
 			{
-				parse_subscription_options(pstate, stmt->options, SUBOPT_LSN, &opts);
-
 				/* ALTER SUBSCRIPTION ... SKIP supports only LSN option */
 				Assert(IsSet(opts.specified_opts, SUBOPT_LSN));
 
@@ -2130,6 +2176,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 		char	   *err;
 		WalReceiverConn *wrconn;
 
+		Assert(conninfo_needed);
+
 		/* Load the library providing us libpq calls. */
 		load_file("libpqwalreceiver", false);

 
 	if (newsub)
 	{

 	if (MySubscription)
 	{

 extern int	CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..3e44232eb23 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -169,19 +169,39 @@ DETAIL:  Foreign data wrapper must be defined with CONNECTION specified.
 RESET SESSION AUTHORIZATION;
 ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
 SET SESSION AUTHORIZATION regress_subscription_user3;
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+  PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
 WARNING:  subscription was created, but is not connected
 HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 RESET SESSION AUTHORIZATION;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+-- fails, cannot drop slot
 DROP SUBSCRIPTION regress_testsub6;
 ERROR:  could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign 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.
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+  PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+WARNING:  subscription was created, but is not connected
+HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+-- ok, test_server lacks user mapping, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub6; --ok
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
 DROP SERVER test_server;
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 2bcb5559a45..598635b6558 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
 #include "executor/executor.h"
 #include "executor/functions.h"
 #include "executor/spi.h"
+#include "foreign/foreign.h"
 #include "funcapi.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
@@ -735,6 +736,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
 Datum
 test_fdw_connection(PG_FUNCTION_ARGS)
 {
+	/* Ensure the test fails if no valid user mapping exists. */
+	GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
 	PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
 }
 
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..f610d6ade18 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -121,18 +121,44 @@ RESET SESSION AUTHORIZATION;
 ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+  PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
 
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 RESET SESSION AUTHORIZATION;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+
+-- fails, cannot drop slot
 DROP SUBSCRIPTION regress_testsub6;
 
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+  PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+
+-- ok, test_server lacks user mapping, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+
+ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
+DROP SUBSCRIPTION regress_testsub6; --ok
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;

From 42bd12a6338d09d6832e4af1e69f85c2790dad21 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v5 3/4] Avoid errors during DROP SUBSCRIPTION when slot_name
 is NONE.

Previously, if the subscription used a server,
ForeignServerConnectionString() could raise an error (e.g. missing
user mapping) during DROP SUBSCRIPTION even if the conninfo wasn't
needed at all.

Construct conninfo after the early return, so that if slot_name is
NONE and rstates is NIL, the DROP SUBSCRIPTION will succeed even if
ForeignServerConnectionString() raises an error (e.g. missing user
mapping).

If slot_name is NONE and rstates is not NIL, DROP SUBSCRIPTION may
still encounter an error from ForeignServerConnectionString().

Reported-by: Hayato Kuroda (Fujitsu) <[email-masked]>
Reviewed-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/subscriptioncmds.c    | 86 +++++++++++++---------
 src/test/regress/expected/subscription.out |  5 +-
 src/test/regress/sql/subscription.sql      |  5 +-
 3 files changed, 57 insertions(+), 39 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 01e992f656e..8a349f7e0be 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"
 
 /*
@@ -2226,6 +2227,43 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	return myself;
 }
 
+/*
+ * 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.
+ */
+static char *
+construct_subserver_conninfo(Oid subserver, Oid subowner, char **err)
+{
+	AclResult		 aclresult;
+	ForeignServer	*server;
+
+	*err = NULL;
+
+	server = GetForeignServer(subserver);
+
+	aclresult = object_aclcheck(ForeignServerRelationId, subserver,
+								subowner, ACL_USAGE);
+	if (aclresult != ACLCHECK_OK)
+	{
+		/*
+		 * 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;
+	}
+
+	return ForeignServerConnectionString(subowner, server);
+}
+
 /*
  * Drop a subscription
  */
@@ -2237,10 +2275,12 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 	HeapTuple	tup;
 	Oid			subid;
 	Oid			subowner;
+	Oid			subserver;
+	char	   *subconninfo = NULL;
 	Datum		datum;
 	bool		isnull;
 	char	   *subname;
-	char	   *conninfo;
+	char	   *conninfo = NULL;
 	char	   *slotname;
 	List	   *subworkers;
 	ListCell   *lc;
@@ -2279,9 +2319,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 		return;
 	}
 
+	datum = SysCacheGetAttr(SUBSCRIPTIONOID, tup,
+							Anum_pg_subscription_subconninfo, &isnull);
+	if (!isnull)
+		subconninfo = TextDatumGetCString(datum);
+
 	form = (Form_pg_subscription) GETSTRUCT(tup);
 	subid = form->oid;
 	subowner = form->subowner;
+	subserver = form->subserver;
 	must_use_password = !superuser_arg(subowner) && form->subpasswordrequired;
 
 	/* must be owner */
@@ -2303,39 +2349,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 								   Anum_pg_subscription_subname);
 	subname = pstrdup(NameStr(*DatumGetName(datum)));
 
-	/* Get conninfo */
@@ -2472,6 +2485,11 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 	 */
 	load_file("libpqwalreceiver", false);
 
+	if (OidIsValid(subserver))
+		conninfo = construct_subserver_conninfo(subserver, subowner, &err);
+	else
+		conninfo = subconninfo;
+
 	if (conninfo)
 		wrconn = walrcv_connect(conninfo, true, true, must_use_password,
 								subname, &err);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 3e44232eb23..41bc265edfb 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -198,10 +198,11 @@ DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 BEGIN;
 ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
 ABORT;
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
+ERROR:  user mapping not found for user "regress_subscription_user3", server "test_server"
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
 DROP SERVER test_server;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index f610d6ade18..576ee17dfd4 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -153,13 +153,12 @@ BEGIN;
 ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
 ABORT;
 
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+-- fails, cannot drop slot
+DROP SUBSCRIPTION regress_testsub6;
 
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
 
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
-
 SET SESSION AUTHORIZATION regress_subscription_user;
 REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
 
-- 
2.43.0

From 56d29ab1c09839fd24dd1c947bf22069534554a0 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Thu, 14 May 2026 10:59:53 -0700
Subject: [PATCH v5 4/4] DROP SUBSCRIPTION: improve error message.

Previously, when constructing the conninfo for a subscription using a
server, errors from ForeignServerConnectionString() were raised
immediately, losing the helpful HINT as well as warnings about
tablesync slots. ACL errors were handled by explicitly formatting an
error message and passing it to ReportSlotConnectionError().

Use a subtransaction to capture ACL errors and
ForeignServerConnectionString() errors and report with
ReportSlotConnectionError() consistently.

Reported-by: Hayato Kuroda (Fujitsu) <[email-masked]>
Reviewed-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/subscriptioncmds.c    | 65 +++++++++++++++-------
 src/test/regress/expected/subscription.out |  3 +-
 2 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8a349f7e0be..7b05b54f9a4 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2231,37 +2231,62 @@ 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);
+		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 41bc265edfb..1af5aec878f 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -200,7 +200,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 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok
 SET SESSION AUTHORIZATION regress_subscription_user;
-- 
2.43.0

---

From: Chao Li <[email-masked]>
Date: Fri, 15 May 2026 15:18:31 +0800

> On May 15, 2026, at 05:45, Jeff Davis <[email-masked]> wrote:
> 
> Hi,
> 
> <v5-0001-Check-retain_dead_tuples-for-ALTER-SUBSCRIPTION-..patch><v5-0002-Avoid-errors-during-ALTER-SUBSCRIPTION.patch><v5-0003-Avoid-errors-during-DROP-SUBSCRIPTION-when-slot_n.patch><v5-0004-DROP-SUBSCRIPTION-improve-error-message.patch>

I have just one comment on v5:

In 0002, for both ALTER_SUBSCRIPTION_SERVER and ALTER_SUBSCRIPTION_CONNECTION, conninfo_needed is false:
```
	if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
		stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
	{
		conninfo_needed = false;
	}
```

0001 adds "check_pub_rdt = sub->retaindeadtuples;" to the both paths:

0002 adds this Assert:
```
	if (update_failover || update_two_phase || check_pub_rdt)
	{
		bool		must_use_password;
		char	   *err;
		WalReceiverConn *wrconn;

		Assert(conninfo_needed);
```

So, for those two paths, if check_pub_rdt is true, then the Assert will be fired, is that intentional?

Best regards,

---

From: Chao Li <[email-masked]>
Date: Mon, 18 May 2026 13:28:32 +0800

> On May 16, 2026, at 07:18, Jeff Davis <[email-masked]> wrote:
> 
> On Fri, 2026-05-15 at 15:18 +0800, Chao Li wrote:
>> 0002 adds this Assert:
>> ```
> I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
> 
> Also, the code above was missing the case of SUBOPT_ORIGIN which could
> set check_pub_rdt. I changed it to be more conservative and set
> orig_conninfo_needed=false when one of:
> 
>  ALTER SUBSCRIPTION ... SERVER
>  ALTER SUBSCRIPTION ... CONNECTION
>  ALTER SUBSCRIPTION ... SET (slot_name=NONE)
> 
> and not try to be precise about which other settings might need
> check_pub_rdt or not.

Yep, this part looks good now.

> 
> What do you think of v6-0003? Is it over-engineered? Should the
> subtransaction happen at a lower level? Is there an alternative to
> using a subtransaction?
> 

For the reason you described in the commit message, catching the error and later reporting it through ReportSlotConnectionError(), I don't think this is over-engineered. I also think keeping the subtransaction inside construct_subserver_conninfo() is reasonable, because this error-capturing behavior seems specific to the DROP SUBSCRIPTION path.

As for whether the HINT itself really helps, I would reserve my opinion. As the test output shows:
```
DROP SUBSCRIPTION regress_testsub6;
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.
```

The error message already says that the problem is “user mapping not found”, so fixing the user mapping could also be a solution. So, the HINT is still useful, but it might not be the most direct fix in some case.

I got another small comment. Now construct_subserver_conninfo() has some duplicate code block of getting foreign server, aclcheck and ForeignServerConnectionString as what GetSubscription() has, maybe we can wrap that piece of code into a helper function.

Best regards,

---

From: Amit Kapila <[email-masked]>
Date: Tue, 26 May 2026 17:44:29 -0700

> The error message already says that the problem is “user mapping not found”, so fixing the user mapping could also be a solution. So, the HINT is still useful, but it might not be the most direct f…

Right, the hint doesn't sound to be a right solution for the problem reported.

-- 
With Regards,
Amit Kapila.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Jun 2026 13:26:51 -0700

> allowed. Personally, it should be avoided anyway. Do you know why

We don't record dependencies on user mappings because technically it's
not dependent on a specific user mapping. That is, GetUserMapping() can
fall back to the public user mapping.

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Jun 2026 16:10:41 -0700

> > Yep, this part looks good now.

Thank you. I committed 0001 with one additional fix: make sure that
ALTER SUBSCRIPTION DISABLE doesn't try to construct the old conninfo.

I plan to commit 0002 soon.

> > 
> > For the reason you described in the commit message, catching the
> > error and later reporting it through ReportSlotConnectionError(), I
> > don't think this is over-engineered. I also think keeping the
> > subtransaction inside construct_subserver_conninfo() is reasonable,
> > because this error-capturing behavior seems specific to the DROP
> > SUBSCRIPTION path.

OK.

> > 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.
> > ```
> > 
> > The error message already says that the problem is “user mapping
> > not found”, so fixing the user mapping could also be a solution.
> > So, the HINT is still useful, but it might not be the most direct
> > fix in some case.
> > 
> 

The user might no longer have permissions on the server to create a new
user mapping, so if they want to drop the subscription, then they need
to set slot_name=NONE.

In other words, we can't delete the existing hint language; we can only
add new language about adding the user mapping back. I don't see a
great way to add that language without making it too long and
confusing, but I am open to suggestion.

v7-0003 is more than just an error message change, so I updated the
commit message. It handles the case where the drop path has
rstates!=NIL but slot_name=NONE: without 0003, the drop will fail while
trying to construct the conninfo (without the HINT); with 0003, the
drop will silently succeed without removing the tablesync slots (as it
does with a connection failure when subconninfo is set).

If the use of a subtransaction is fine there, then I think we should
proceed with 0003 and whatever hint message is agreeable.

Regards,
	Jeff Davis

From 0b8958d75d65d32c4c41e784ec17ea344957e353 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v7 2/3] Avoid errors during DROP SUBSCRIPTION when slot_name
 is NONE.

Reported-by: Hayato Kuroda (Fujitsu) <[email-masked]>
Reviewed-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/subscriptioncmds.c    | 85 +++++++++++++---------
 src/test/regress/expected/subscription.out |  5 +-
 src/test/regress/sql/subscription.sql      |  5 +-
 3 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index c9faf68cbc5..ee06a726f42 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2260,6 +2260,43 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	return myself;
 }
 
+/*
+		 * unusable subconninfo (which will result in an ERROR later unless
+		 * slot_name = NONE).
@@ -2271,10 +2308,12 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
@@ -2313,9 +2352,15 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 		return;
 	}

 	/* must be owner */
@@ -2337,39 +2382,6 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 								   Anum_pg_subscription_subname);
 	subname = pstrdup(NameStr(*DatumGetName(datum)));
 
-	/* Get conninfo */
@@ -2506,6 +2518,11 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
 
index 61236672ce4..8dbfac66326 100644
@@ -213,11 +213,12 @@ DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
 ALTER SUBSCRIPTION regress_testsub6 DISABLE;
index 665b510f180..05533d66675 100644
@@ -164,14 +164,13 @@ BEGIN;

 ALTER SUBSCRIPTION regress_testsub6 DISABLE;
 ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
 DROP SUBSCRIPTION regress_testsub6; --ok

-- 
2.43.0

From fc35789170a27db04af0a650058bbe81a546a58f Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email-masked]>
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) <[email-masked]>
Reviewed-by: Chao Li <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/commands/subscriptioncmds.c    | 67 +++++++++++++++-------
 src/test/regress/expected/subscription.out |  3 +-
 2 files changed, 49 insertions(+), 21 deletions(-)

index ee06a726f42..137bfa1a9b0 100644
 
 /*
@@ -2264,37 +2265,63 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 	*err = NULL;

-		 * unusable subconninfo (which will result in an ERROR later unless
-		 * slot_name = NONE).
+		Assert(conninfo != NULL);

 /*
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

---

From: Jeff Davis <[email-masked]>
Date: Fri, 19 Jun 2026 11:40:34 -0700

> proceed with 0003 and whatever hint message is agreeable.

0002 committed.

For 0003, I don't think we can absorb every kind of error that might
happen inside fdwconnection. libpqrcv_connect() doesn't attempt to do
so, so neither should construct_subserver_conninfo().

What 0003 is really trying to avoid is fairly normal kinds of errors
that can happen on a non-broken FDW that get in the way of dropping a
subscription. That includes a missing user mapping or a failed ACL
check. But aside from that, it's hard to think of other examples we'd
clearly want to absorb.

Options:

  * check for those two error codes explicitly
  * try to be more systematic about what kinds of errors should be
absorbed or not
  * add an escape hatch for users to turn off tablesync slots so that
DROP will always succeed
  * consider it an unimportant edge case and leave it the way it is
(with 0001 & 0002 already done), and close the open item

Thoughts?

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Mon, 22 Jun 2026 14:10:25 -0700

>   * add an escape hatch for users to turn off tablesync slots so that

It looks like SLOT_NAME=NONE is already supposed to be this escape
hatch, even for tablesync slots. From the docs:

"To proceed in this situation, first disable the subscription by
executing ALTER SUBSCRIPTION ... DISABLE, and then disassociate it from
the replication slot by executing ALTER SUBSCRIPTION ... SET (slot_name
= NONE). After that, DROP SUBSCRIPTION will no longer attempt any
actions on a remote host."

https://www.postgresql.org/docs/devel/sql-dropsubscription.html

But DropSubscription() only does the early-return if there are no
tablesync slots. If there are tablesync slots, it still tries to
contact the publisher, even if SLOT_NAME=NONE.

> (with 0001 & 0002 already done), and close the open item

I plan to close this open item, and treat the above as a pre-existing
bug, which may require a backport.

Regards,
	Jeff Davis

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 24 Jun 2026 02:18:20 +0000

Dear Jeff,

I went through the 0003 patch. Apart from HINT message, I found that error code
ERRCODE_CONNECTION_FAILURE is always used. Should we report the original errcode
of the root cause? Or it's OK because the observed outcome is the cannot-connect
error?

Another comment:

```

Do we have to consider the case that ForeignServerConnectionString() returns NULL
without any ERRORs? For the production build, the Assert() would be skipped and
the segmentation fault would happen in the `if (!conninfo)` part.
I checked the doc [1] but there are no specifications for it.

[1]: https://www.postgresql.org/docs/devel/sql-createforeigndatawrapper.html#:~:text=ereport(ERROR)%20function.-,CONNECTION%20connection_function,-connection_function%20is%20the

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Jun 2026 09:57:54 +0530

> contact the publisher, even if SLOT_NAME=NONE.

AFAICS, the corresponding DropSubscription() code is as follows:

if (!slotname && rstates == NIL)
{
table_close(rel, NoLock);
return;
}
....
if (OidIsValid(subserver))
conninfo = construct_subserver_conninfo(subserver, subowner, &err);
else
conninfo = subconninfo;

if (conninfo)

if (wrconn == NULL)
{
if (!slotname)
{
/* be tidy */
list_free(rstates);
table_close(rel, NoLock);
return;
}
else
{
ReportSlotConnectionError(rstates, subid, slotname, err);
}
}

So, if the connection is not successful then we simply return without
checking tablesync slots (when slotname is NONE) whereas with FDW
server connection, the ERROR will be raised from
construct_subserver_conninfo->ForeignServerConnectionString(). Isn't
that different from current situation?

> bug, which may require a backport.

As per my understanding it is better to go with your fix idea even
though we don't have better ideas for HINT message. I see your point
of making HINT much longer as we may need to add something like (...
or re-create the user mapping/ACL and retry..), so we can keep the
current one as it is and if we see any real user complaint then we can
consider improving it in the future.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Jun 2026 10:07:00 +0530

On Wed, Jun 24, 2026 at 7:48 AM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> of the root cause?

That could be marginally better but not sure if it is worth the
complexity. I have a few more points regarding 0003:
* Regarding the point: "we can't absorb every kind of error." In the
PG_CATCH, re-throw query-cancel / interrupt-class conditions (if those
are possible) instead of swallowing them, otherwise a SIGINT during
ForeignServerConnectionString() becomes a silent successful DROP (for
slot_name=NONE) or a mislabeled connection error.

*
+ 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));

I am not sure if this is a good idea as the outer catch will anyway
silence this.

-- 
With Regards,
Amit Kapila.

---

From: Jeff Davis <[email-masked]>
Date: Mon, 06 Jul 2026 16:41:34 -0700

> slot_name=NONE) or a mislabeled connection error.

The DROP SUBSCRIPTION documentation says that:

But that stopped being true after commit ce0fdbfe97. There should be
some kind of escape such that we can force a DROP SUBSCRIPTION to
succeed without trying to connect to the remote host. And if we have
that escape, then we don't need to overcomplicate this by using a
subtransaction and trying to guess which errors are recoverable or not.

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Mon, 06 Jul 2026 16:46:10 -0700

...

> that different from current situation?

It's easier to cause an error in ForeignServerConnectionString(); but
walrcv_connect() can throw errors, too. Also, a network connection is a
side-effect that you may not want to have for whatever reason.

I think it's better to have an escape that prevents any connection to
the publisher, as the documentation says, to deal with these kinds of
edge cases.

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Wed, 8 Jul 2026 10:29:15 +0530

> subtransaction and trying to guess which errors are recoverable or not.

Okay, I see your point and we can make it return early when slot_name
is NONE. However, do we need to consider a case where users want to
manually manage the subscription's main slot, so it uses
slot_name=NONE? I mean users want to manually maintain the lifecycle
of the slot and due to that even during the CREATE SUBSCRIPTION, it
uses the specified slot_name instead of the default. But still
tablesync slots need to be removed in such a case as those are
internally created. Actually, this best-case try to connect to
publisher works for such cases. I think this is mostly a theoretical
scenario because ideally these tablesync slots should be removed at
the end of tablesync especially when users are doing drop subscription
as part of planned operation, so we can as well rely on the document
as we are doing now.

-- 
With Regards,
Amit Kapila.

---

From: Jeff Davis <[email-masked]>
Date: Wed, 08 Jul 2026 12:20:45 -0700

On Wed, 2026-07-08 at 10:29 +0530, Amit Kapila wrote:
> internally created.

The problem is that we are overloading slot_name=NONE, and as you point
out, it can mean multiple things.

Can we just have DROP SUBSCRIPTION ... FORCE or DROP SUBSCRIPTION ...
NOCONNECT? That allows the user to clearly express their intent at the
time of the drop, rather than guessing from the state of the
subscription.

If it's too late to do that for 19, we should probably just make the
code match the documentation and don't try to connect if
slot_name=NONE. In most ordinary cases that would not cause a problem,
you'd have to have slot_name=NONE and also drop the subscription before
the table syncs finish.

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Jul 2026 12:47:02 +0530

> time of the drop, rather than guessing from the state of the

That sounds like an appropriate solution or we can use (connect =
false) option as well a way to indicate NOCONNECT.

> If it's too late to do that for 19,
>

I feel it is late for 19.

 we should probably just make the
> you'd have to have slot_name=NONE and also drop the subscription before

Yeah, we can do this but if we want to change this behaviour in the
next version, isn't it better to tweak the doc wording as:"After that,
DROP SUBSCRIPTION will not attempt to drop the subscription's own
replication slot. It may still connect to the publisher to drop
internally-created table synchronization slots if some table
synchronization was left unfinished; if the publisher is unreachable,
those slots (and the main slot, if it still exists) must be dropped
manually.". I think in most cases where we need this is where the
publisher is not available or the network is broken and in those
cases, the current code still works and won't leave any tablesync
slots.

-- 
With Regards,
Amit Kapila.