On Tue, 2026-05-26 at 17:44 -0700, Amit Kapila wrote:
> > 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.
> > 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.
> >
>
> Right, the hint doesn't sound to be a right solution for the problem
> reported.
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 protected]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v7 2/3] 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 protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/os9pr01mb12149b54dea148108c6fa5667f5...@os9pr01mb12149.jpnprd01.prod.outlook.com
---
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;
}
+/*
+ * 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
*/
@@ -2271,10 +2308,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;
@@ -2313,9 +2352,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 */
@@ -2337,39 +2382,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);
@@ -2506,6 +2518,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 61236672ce4..8dbfac66326 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -213,11 +213,12 @@ 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 DISABLE;
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 665b510f180..05533d66675 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -164,14 +164,13 @@ 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 DISABLE;
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 fc35789170a27db04af0a650058bbe81a546a58f Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
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 protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/os9pr01mb12149b54dea148108c6fa5667f5...@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
