From e899c17d50ce467b7d6fdd2e0d05afb956d28118 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 14 May 2026 10:59:53 -0700 Subject: [PATCH v6 3/3] 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 d6422ba5b4a..ea37455a257 100644 --- a/src/backend/commands/subscriptioncmds.c +++ b/src/backend/commands/subscriptioncmds.c @@ -2226,37 +2226,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