On Wed, 2026-05-06 at 15:57 +0800, Chao Li wrote:
> PFA v3 - addressed Ajin and Zsolt’s comments.
Thank you for the report!
The proposed patch seems unnecessarily complex, though. It seems too
easy to add GetSubscriptionConninfo() in the wrong place and end up
with another problem that's not easily detected.
Can't we just do something like the attached? It's easy to explain at
the call site that, when changing to a different server or using
CONNECTION instead, that we don't need the old conninfo at all.
I included your test case in my patch, and it passes.
Also, Hayato Kuroda's report was an issue also because the error could
be thrown even if slotname was NULL. Patch attached for that, as well.
Thank you, also!
Regards,
Jeff Davis
From 3430b5c0e29351ebc19c6763411ca1b29b401fec Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Fri, 8 May 2026 16:23:00 -0700
Subject: [PATCH v4 1/2] 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.
Add parameter to GetSubscrpition() that allows the caller to bypass
generating the conninfo, which is unneeded in these cases because it's
about to be replaced anyway.
Test case from Chao Li.
Reported-by: Chao Li <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/pg_subscription.c | 64 ++++++++++++----------
src/backend/commands/subscriptioncmds.c | 22 ++++++--
src/backend/replication/logical/worker.c | 4 +-
src/include/catalog/pg_subscription.h | 3 +-
src/test/regress/expected/subscription.out | 16 ++++++
src/test/regress/regress.c | 3 +
src/test/regress/sql/subscription.sql | 19 +++++++
7 files changed, 92 insertions(+), 39 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 1f1fdc75af6..79739f0354b 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -74,7 +74,8 @@ GetPublicationsStr(List *publications, StringInfo dest, bool quote_literal)
* Fetch the subscription from the syscache.
*/
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;
@@ -100,7 +101,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;
@@ -120,37 +121,40 @@ GetSubscription(Oid subid, bool missing_ok, bool aclcheck)
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);
+
+ /* recheck ACL if requested */
+ if (conninfo_aclcheck)
+ {
+ 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 1e10d9d9a58..21b27991d3c 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1424,6 +1424,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
bool update_failover = false;
bool update_two_phase = false;
bool check_pub_rdt = false;
+ bool conninfo_needed = true;
bool retain_dead_tuples;
int max_retention;
bool retention_active;
@@ -1454,13 +1455,22 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
stmt->subname);
/*
- * 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.
+ * If we're replacing the connection information, we don't need the old
+ * conninfo at all. Trying to construct it could encounter errors, which
+ * would prevent the user from fixing the problem.
*/
- sub = GetSubscription(subid, false, false);
+ if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
+ stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
+ conninfo_needed = false;
+
+ /*
+ * Skip ACL checks when constructing the existing connection information;
+ * the ACL check will be performed on the new connection information, if
+ * any. If changing something unrelated to conninfo, there's no need to do
+ * an additional ACL check on the foreign server here; that will be done
+ * by the subscription worker.
+ */
+ sub = GetSubscription(subid, false, conninfo_needed, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
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..128f44eaacd 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -174,16 +174,32 @@ 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;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+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
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.
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
DROP FUNCTION test_fdw_connection(oid, oid, internal);
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..c90f8a03ed8 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,6 +124,12 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
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;
+CREATE SERVER test_server2 FOREIGN DATA WRAPPER test_fdw;
+GRANT USAGE ON FOREIGN SERVER test_server2 TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server2 OPTIONS(user 'bar', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -131,12 +137,25 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- fail, must connect but lacks USAGE on server, as well as user mapping
DROP SUBSCRIPTION regress_testsub6;
+-- ok, should not need to resolve conninfo for the old broken server
+ALTER SUBSCRIPTION regress_testsub6 SERVER test_server2;
+DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server2;
+RESET SESSION AUTHORIZATION;
+REVOKE USAGE ON FOREIGN SERVER test_server2 FROM regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
+-- ok, should not need to resolve conninfo for the current broken server
+ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist2 password=secret';
+RESET SESSION AUTHORIZATION;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
ALTER SUBSCRIPTION regress_testsub6 SET (slot_name = NONE);
DROP SUBSCRIPTION regress_testsub6;
SET SESSION AUTHORIZATION regress_subscription_user;
REVOKE CREATE ON DATABASE REGRESSION FROM regress_subscription_user3;
+DROP SERVER test_server2;
DROP SERVER test_server;
-- fail, FDW is dependent
--
2.43.0
From 39b2a13ee7904256fc100a0f92104c056386712e Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Fri, 8 May 2026 17:16:50 -0700
Subject: [PATCH v4 2/2] Avoid errors during DROP SUBSCRIPTION.
Previously, an error during ForeignServerConnectionString() could
prevent a subscription from being dropped, even if slot_name was
NULL. ACL check errors were handled, but not other errors, such as a
dropped user mapping.
Change to only construct conninfo if slotname is defined. Errors
(aside from ACL check failures) will not benefit from the more helpful
ReportSlotConnectionError(), but at least they will not interfere with
a user fixing the problem.
Reported-by: Hayato Kuroda (Fujitsu) <[email protected]>
Discussion: https://postgr.es/m/os9pr01mb12149b54dea148108c6fa5667f5...@os9pr01mb12149.jpnprd01.prod.outlook.com
---
src/backend/commands/subscriptioncmds.c | 71 +++++++++++++------------
1 file changed, 37 insertions(+), 34 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 21b27991d3c..498a102200e 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2193,7 +2193,7 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
Datum datum;
bool isnull;
char *subname;
- char *conninfo;
+ char *conninfo = NULL;
char *slotname;
List *subworkers;
ListCell *lc;
@@ -2256,39 +2256,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);
@@ -2297,6 +2264,42 @@ DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel)
else
slotname = NULL;
+ /* conninfo only needed if slotname is valid */
+ if (slotname)
+ {
+ 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 and ReportSlotConnectionError()
+ * for a more helpful error.
+ */
+ 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);
+ }
+ }
+
/*
* Since dropping a replication slot is not transactional, the replication
* slot stays dropped even if the transaction rolls back. So we cannot
--
2.43.0
