On Fri, 2026-05-15 at 15:18 +0800, Chao Li wrote:
> 0002 adds this Assert:
> ```
> if (update_failover || update_two_phase || check_pub_rdt)
> {
> bool must_use_password;
> char *err;
> WalReceiverConn *wrconn;
>
> Assert(conninfo_needed);
> ```
>
> So, for those two paths, if check_pub_rdt is true, then the Assert
> will be fired, is that intentional?
I've fixed it to be Assert(new_conninfo || orig_conninfo_needed).
Also, the code above was missing the case of SUBOPT_ORIGIN which could
set check_pub_rdt. I changed it to be more conservative and set
orig_conninfo_needed=false when one of:
ALTER SUBSCRIPTION ... SERVER
ALTER SUBSCRIPTION ... CONNECTION
ALTER SUBSCRIPTION ... SET (slot_name=NONE)
and not try to be precise about which other settings might need
check_pub_rdt or not.
What do you think of v6-0003? Is it over-engineered? Should the
subtransaction happen at a lower level? Is there an alternative to
using a subtransaction?
Regards,
Jeff Davis
From 70cc6eff0ba070337e40c51d4631f8ea99f51c29 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:10:07 -0700
Subject: [PATCH v6 1/3] Avoid errors during ALTER SUBSCRIPTION.
Previously, when retrieving the old Subscription object, constructing
the conninfo could encounter an error during
ForeignServerConnectionString(). ACL errors were handled properly, but
other errors could interfere with a user fixing the problem with ALTER
SUBSCRIPTION.
Reported-by: Chao Li <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 72 ++++++++------
src/backend/commands/subscriptioncmds.c | 105 +++++++++++++++------
src/backend/replication/logical/worker.c | 4 +-
src/include/catalog/pg_subscription.h | 3 +-
src/test/regress/expected/subscription.out | 28 +++++-
src/test/regress/regress.c | 3 +
src/test/regress/sql/subscription.sql | 34 ++++++-
7 files changed, 176 insertions(+), 73 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..82fc91e0810 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -72,9 +72,15 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
/*
* Fetch the subscription from the syscache.
+ *
+ * If conninfo_needed is true, conninfo will be constructed, possibly
+ * encountering errors in ForeignServerConnectionString(). Callers not
+ * expecting such errors should pass false, in which case conninfo will be
+ * NULL.
*/
Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
+GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
+ bool conninfo_aclcheck)
{
HeapTuple tup;
Subscription *sub;
@@ -84,6 +90,8 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
MemoryContext cxt;
MemoryContext oldcxt;
+ Assert(conninfo_needed || !conninfo_aclcheck);
+
tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
if (!HeapTupleIsValid(tup))
@@ -100,7 +108,7 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
subform = (Form_pg_subscription) GETSTRUCT(tup);
- sub = palloc_object(Subscription);
+ sub = palloc0_object(Subscription);
sub->cxt = cxt;
sub->oid = subid;
sub->dbid = subform->subdbid;
@@ -119,38 +127,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
sub->maxretention = subform->submaxretention;
sub->retentionactive = subform->subretentionactive;
- /* Get conninfo */
- if (OidIsValid(subform->subserver))
+ if (conninfo_needed)
{
- AclResult aclresult;
- ForeignServer *server;
-
- server = GetForeignServer(subform->subserver);
-
- /* recheck ACL if requested */
- if (aclcheck)
+ if (OidIsValid(subform->subserver))
{
- aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
- subform->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(subform->subowner, false),
- server->servername)));
+ AclResult aclresult;
+ ForeignServer *server;
+
+ server = GetForeignServer(subform->subserver);
+
+ if (conninfo_aclcheck)
+ {
+ /* recheck ACL if requested */
+ aclresult = object_aclcheck(ForeignServerRelationId,
+ subform->subserver,
+ subform->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(subform->subowner, false),
+ server->servername)));
+ }
+
+ sub->conninfo = ForeignServerConnectionString(subform->subowner,
+ server);
+ }
+ else
+ {
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
+ tup,
+ Anum_pg_subscription_subconninfo);
+ sub->conninfo = TextDatumGetCString(datum);
}
-
- sub->conninfo = ForeignServerConnectionString(subform->subowner,
- server);
- }
- else
- {
- datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID,
- tup,
- Anum_pg_subscription_subconninfo);
- sub->conninfo = TextDatumGetCString(datum);
}
/* Get slotname */
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 523959ba0ce..12f349e24a2 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1420,6 +1420,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
Datum values[Natts_pg_subscription];
HeapTuple tup;
Oid subid;
+ bool orig_conninfo_needed = true;
bool update_tuple = false;
bool update_failover = false;
bool update_two_phase = false;
@@ -1454,14 +1455,83 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION,
stmt->subname);
+ /* parse and check options */
+ switch (stmt->kind)
+ {
+ case ALTER_SUBSCRIPTION_OPTIONS:
+ supported_opts = (SUBOPT_SLOT_NAME |
+ SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
+ SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
+ SUBOPT_DISABLE_ON_ERR |
+ SUBOPT_PASSWORD_REQUIRED |
+ SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
+ SUBOPT_RETAIN_DEAD_TUPLES |
+ SUBOPT_MAX_RETENTION_DURATION |
+ SUBOPT_WAL_RECEIVER_TIMEOUT |
+ SUBOPT_ORIGIN);
+ break;
+
+ case ALTER_SUBSCRIPTION_ENABLED:
+ supported_opts = SUBOPT_ENABLED;
+ break;
+
+ case ALTER_SUBSCRIPTION_SET_PUBLICATION:
+ supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
+ break;
+
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
+ break;
+
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION:
+ supported_opts = SUBOPT_COPY_DATA;
+ break;
+
+ case ALTER_SUBSCRIPTION_SKIP:
+ supported_opts = SUBOPT_LSN;
+ break;
+
+ default:
+ supported_opts = 0;
+ break;
+ }
+
+ if (supported_opts > 0)
+ parse_subscription_options(pstate, stmt->options, supported_opts, &opts);
+
+ /*
+ * Ensure that ALTER SUBSCRIPTION commands that could be used to fix a
+ * broken connection or prepare to drop a broken subscription don't
+ * attempt to construct the conninfo. Otherwise, we might encounter the
+ * error the user is trying to fix.
+ *
+ * Specifically, ALTER SUBSCRIPTION ... SERVER, ALTER SUBSCRIPTION ...
+ * CONNECTION, or ALTER SUBSCRIPTION ... SET (slot_name = NONE).
+ *
+ * NB: if the user specifies multiple SET options, then we may still need
+ * to construct conninfo even if slot_name is set to NONE.
+ */
+ if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+ stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+ {
+ orig_conninfo_needed = false;
+ }
+ else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS)
+ {
+ /* ... SET (slot_name = NONE) with no other options */
+ if (opts.specified_opts == SUBOPT_SLOT_NAME && !opts.slot_name)
+ orig_conninfo_needed = false;
+ }
+
/*
* Skip ACL checks on the subscription's foreign server, if any. If
* changing the server (or replacing it with a raw connection), then the
* old one will be removed anyway. If changing something unrelated,
* there's no need to do an additional ACL check here; that will be done
- * by the subscription worker anyway.
+ * by the subscription worker.
*/
- sub = GetSubscription(subid, false, false);
+ sub = GetSubscription(subid, false, orig_conninfo_needed, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
@@ -1492,20 +1562,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
{
case ALTER_SUBSCRIPTION_OPTIONS:
{
- supported_opts = (SUBOPT_SLOT_NAME |
- SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
- SUBOPT_STREAMING | SUBOPT_TWOPHASE_COMMIT |
- SUBOPT_DISABLE_ON_ERR |
- SUBOPT_PASSWORD_REQUIRED |
- SUBOPT_RUN_AS_OWNER | SUBOPT_FAILOVER |
- SUBOPT_RETAIN_DEAD_TUPLES |
- SUBOPT_MAX_RETENTION_DURATION |
- SUBOPT_WAL_RECEIVER_TIMEOUT |
- SUBOPT_ORIGIN);
-
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME))
{
/*
@@ -1769,8 +1825,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_ENABLED:
{
- parse_subscription_options(pstate, stmt->options,
- SUBOPT_ENABLED, &opts);
Assert(IsSet(opts.specified_opts, SUBOPT_ENABLED));
if (!sub->slotname && opts.enabled)
@@ -1907,10 +1961,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_SET_PUBLICATION:
{
- supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(stmt->publication);
replaces[Anum_pg_subscription_subpublications - 1] = true;
@@ -1954,10 +2004,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
List *publist;
bool isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
- supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA;
- parse_subscription_options(pstate, stmt->options,
- supported_opts, &opts);
-
publist = merge_publications(sub->publications, stmt->publication, isadd, stmt->subname);
values[Anum_pg_subscription_subpublications - 1] =
publicationListToArray(publist);
@@ -2015,9 +2061,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
errmsg("%s is not allowed for disabled subscriptions",
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION")));
- parse_subscription_options(pstate, stmt->options,
- SUBOPT_COPY_DATA, &opts);
-
/*
* The subscription option "two_phase" requires that
* replication has passed the initial table synchronization
@@ -2063,8 +2106,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
case ALTER_SUBSCRIPTION_SKIP:
{
- parse_subscription_options(pstate, stmt->options, SUBOPT_LSN, &opts);
-
/* ALTER SUBSCRIPTION ... SKIP supports only LSN option */
Assert(IsSet(opts.specified_opts, SUBOPT_LSN));
@@ -2130,6 +2171,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
char *err;
WalReceiverConn *wrconn;
+ Assert(new_conninfo || orig_conninfo_needed);
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index dd6fc38a41e..909bd47f0a9 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5059,7 +5059,7 @@ maybe_reread_subscription(void)
started_tx = true;
}
- newsub = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ newsub = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (newsub)
{
@@ -5808,7 +5808,7 @@ InitializeLogRepWorker(void)
LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid, 0,
AccessShareLock);
- MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true);
+ MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
if (MySubscription)
{
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index a6a2ad1e49c..48944201889 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -213,7 +213,8 @@ typedef struct Subscription
#endif /* EXPOSE_TO_CLIENT_CODE */
extern Subscription *GetSubscription(Oid subid, bool missing_ok,
- bool aclcheck);
+ bool conninfo_needed,
+ bool conninfo_aclcheck);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 7e3cabdb93f..3e44232eb23 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -169,19 +169,39 @@ DETAIL: Foreign data wrapper must be defined with CONNECTION specified.
RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
ERROR: could not connect to publisher when attempting to drop replication slot "dummy": subscription owner "regress_subscription_user3" does not have permission on foreign 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.
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+WARNING: subscription was created, but is not connected
+HINT: To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+-- ok, test_server lacks user mapping, but replacing connection anyway
+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');
+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/regress.c b/src/test/regress/regress.c
index 4927f1ddcbf..d5aafdf370c 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -31,6 +31,7 @@
#include "executor/executor.h"
#include "executor/functions.h"
#include "executor/spi.h"
+#include "foreign/foreign.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -736,6 +737,8 @@ PG_FUNCTION_INFO_V1(test_fdw_connection);
Datum
test_fdw_connection(PG_FUNCTION_ARGS)
{
+ /* Ensure the test fails if no valid user mapping exists. */
+ GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 6c3d9632e8a..f610d6ade18 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -121,18 +121,44 @@ RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
-DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, must connect but lacks USAGE on server, as well as user mapping
+-- ok, lacks USAGE on test_server, but replacing connection anyway
+BEGIN;
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
+ABORT;
+
+-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
+RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
-DROP SUBSCRIPTION regress_testsub6;
+DROP SUBSCRIPTION regress_testsub6; --ok
+
+CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
+ PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+
+-- ok, test_server lacks user mapping, but replacing connection anyway
+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');
+
+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 ceb97ecc26467cbcff24da1484706d6a04b04509 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:29:29 -0700
Subject: [PATCH v6 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 | 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 12f349e24a2..d6422ba5b4a 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"
/*
@@ -2221,6 +2222,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
*/
@@ -2232,10 +2270,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;
@@ -2274,9 +2314,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 */
@@ -2298,39 +2344,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);
@@ -2467,6 +2480,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
From e899c17d50ce467b7d6fdd2e0d05afb956d28118 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
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) <[email protected]>
Reviewed-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/os9pr01mb12149b54dea148108c6fa5667f5...@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
