Hi,
I added a fix to the series: v5-0001 fixes check_pub_rdt for the
foreign server case.
On Sat, 2026-05-09 at 11:08 +0800, Chao Li wrote:
> Ah, I see. You added a new conninfo_needed parameter to
> GetSubscription(), which separates the decision of building conninfo
> from the ACL check. Cool, I believe this is a better approach.
>
> So 0001 looks good to me. nitpick is that conninfo_aclcheck is now
> only meaningful when conninfo_needed is true. I wonder if we should
> mention that briefly in the function header comment, or add an
> assertion such as: Assert(conninfo_needed || !conninfo_aclcheck); to
> avoid possible misuse of conninfo_aclcheck in the future.
I refactored the fix in v5-0002 to do this in a more organized way: now
all option parsing happens first, so I can more precisely decide which
paths need conninfo and which ones don't.
> So with 0002, if slotname is NULL but rstates is not NIL, it looks
> possible that we no longer build conninfo and therefore skip the
> cleanup on the publisher side.
I separated this into two patches:
v5-0003 just moves the connection string building after the early exit,
so that if slotname is NONE and rstates is NIL, then it won't try to
build the connection string at all (and therefore won't get an error
while doing so).
v5-0004 fixes the remaining issue when slotname is NONE and rstates is
*not* NIL. It uses a subtransaction to catch the error, so that the
DROP TRANSACTION will still succeed even though it can't connect to the
publisher to drop the tablesync slots. This feels a bit over-
engineered, but it does maintain the expected behavior in this case. It
also routes errors inside of ForeignServerConnectionString() through
ReportSlotConnectionError(), which adds a helpful hint.
Regards,
Jeff Davis
From f0edc45856a4c6d2ddb9f83d53bd1c50978801d9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:57:31 -0700
Subject: [PATCH v5 1/4] Check retain_dead_tuples for ALTER SUBSCRIPTION ...
SERVER.
Previously, the subscription setting retain_dead_tuples didn't cause
ALTER SUBSCRIPTION ... SERVER to check the publisher. And if the
publisher was checked for some other reason, then it would use the old
conninfo.
Fix ALTER SUBSCRIPTION ... SERVER to always check the publisher when
retain_dead_tuples is set, and to use the new connection info, like
ALTER SUBSCRIPTION ... CONNECTION.
---
src/backend/commands/subscriptioncmds.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 7818f667edf..523959ba0ce 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1427,6 +1427,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
bool retain_dead_tuples;
int max_retention;
bool retention_active;
+ char *new_conninfo = NULL;
char *origin;
Subscription *sub;
Form_pg_subscription form;
@@ -1810,7 +1811,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ForeignServer *new_server;
ObjectAddress referenced;
AclResult aclresult;
- char *conninfo;
/*
* Remove what was there before, either another foreign server
@@ -1846,13 +1846,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
/* make sure a user mapping exists */
GetUserMapping(form->subowner, new_server->serverid);
- conninfo = ForeignServerConnectionString(form->subowner,
- new_server);
+ new_conninfo = ForeignServerConnectionString(form->subowner,
+ new_server);
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
- walrcv_check_conninfo(conninfo,
+ walrcv_check_conninfo(new_conninfo,
sub->passwordrequired && !sub->ownersuperuser);
values[Anum_pg_subscription_subserver - 1] = ObjectIdGetDatum(new_server->serverid);
@@ -1863,6 +1863,13 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
update_tuple = true;
}
+
+ /*
+ * Since the remote server configuration might have changed,
+ * perform a check to ensure it permits enabling
+ * retain_dead_tuples.
+ */
+ check_pub_rdt = sub->retaindeadtuples;
break;
case ALTER_SUBSCRIPTION_CONNECTION:
@@ -1877,10 +1884,12 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
replaces[Anum_pg_subscription_subserver - 1] = true;
}
+ new_conninfo = stmt->conninfo;
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Check the connection info string. */
- walrcv_check_conninfo(stmt->conninfo,
+ walrcv_check_conninfo(new_conninfo,
sub->passwordrequired && !sub->ownersuperuser);
values[Anum_pg_subscription_subconninfo - 1] =
@@ -2129,7 +2138,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(stmt->conninfo ? stmt->conninfo : sub->conninfo,
+ wrconn = walrcv_connect(new_conninfo ? new_conninfo : sub->conninfo,
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
--
2.43.0
From 6ab85d7b22204dd40562157096e6989f04932ae2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Tue, 12 May 2026 14:10:07 -0700
Subject: [PATCH v5 2/4] 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 | 110 +++++++++++++++------
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, 181 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..01e992f656e 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 conninfo_needed = true;
bool update_tuple = false;
bool update_failover = false;
bool update_two_phase = false;
@@ -1454,14 +1455,88 @@ 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)
+ {
+ conninfo_needed = false;
+ }
+ else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS)
+ {
+ if (IsSet(opts.specified_opts, SUBOPT_SLOT_NAME) && !opts.slot_name)
+ conninfo_needed = false;
+
+ /* for these options, we still need conninfo */
+ if (IsSet(opts.specified_opts, SUBOPT_FAILOVER) ||
+ IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT) ||
+ IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES))
+ conninfo_needed = true;
+ }
+
/*
* 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, conninfo_needed, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
@@ -1492,20 +1567,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 +1830,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 +1966,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 +2009,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 +2066,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 +2111,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 +2176,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
char *err;
WalReceiverConn *wrconn;
+ Assert(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 2bcb5559a45..598635b6558 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"
@@ -735,6 +736,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 42bd12a6338d09d6832e4af1e69f85c2790dad21 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
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) <[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 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
From 56d29ab1c09839fd24dd1c947bf22069534554a0 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
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) <[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 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
