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