From 56d29ab1c09839fd24dd1c947bf22069534554a0 Mon Sep 17 00:00:00 2001 From: Jeff Davis 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) Reviewed-by: Chao Li Discussion: https://postgr.es/m/OS9PR01MB12149B54DEA148108C6FA5667F52D2@OS9PR01MB12149.jpnprd01.prod.outlook.com --- 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