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