On Mon, 2026-08-03 at 16:06 +0530, Amit Kapila wrote:
> Right, that is possible. In such a scenario, the current behavior of
> the apply-worker appears okay to me. Anyway, the feature
> disable_on_error is for the user to evaluate/analyze the current
> ERROR
> and accordingly take the next action. In this case, she can enable
> the
> subscription again.
That makes sense to me.
> > Or, perhaps these are just edge cases, and part (b) is not very
> > important?
> >
>
> I think so. We don't need to do anything for part (b).
Agreed.
> BTW, shall we add a detailed comment as to why we separate the load
> of
> connection info from other subscription parameters for future readers
> on the following lines:
Done using your wording in v4-0001.
New v4 series attached.
Regards,
Jeff Davis
From 7631a896d83e08372509486387475360937f4c8d Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 12:34:03 -0700
Subject: [PATCH v4 1/8] Remove Subscription conninfo field; generate in
caller.
After server-based subscriptions, conninfo became more than just a
catalog field. It has its own error paths, and it's important that
callers that don't need conninfo don't encounter errors related to it.
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Reviewed-by: Amit Kapila <[email protected]>
Backpatch-through: 19
---
src/backend/catalog/pg_subscription.c | 105 ++++++++++--------
src/backend/commands/subscriptioncmds.c | 43 +++++--
.../replication/logical/sequencesync.c | 2 +-
src/backend/replication/logical/tablesync.c | 2 +-
src/backend/replication/logical/worker.c | 22 +++-
src/include/catalog/pg_subscription.h | 6 +-
src/include/replication/worker_internal.h | 1 +
7 files changed, 116 insertions(+), 65 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 5ff61edb989..9083c5762cc 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -79,14 +79,10 @@ 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.
+ * Callers that need conninfo must call SubscriptionConninfo().
*/
Subscription *
-GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
- bool conninfo_aclcheck)
+GetSubscription(Oid subid, bool missing_ok)
{
HeapTuple tup;
Subscription *sub;
@@ -96,8 +92,6 @@ GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
MemoryContext cxt;
MemoryContext oldcxt;
- Assert(conninfo_needed || !conninfo_aclcheck);
-
tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
if (!HeapTupleIsValid(tup))
@@ -140,42 +134,6 @@ GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
sub->retentionactive = subform->subretentionactive;
sub->conflictlogrelid = subform->subconflictlogrelid;
- if (conninfo_needed)
- {
- if (OidIsValid(subform->subserver))
- {
- 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);
- }
- }
-
/* Get slotname */
datum = SysCacheGetAttr(SUBSCRIPTIONOID,
tup,
@@ -226,6 +184,65 @@ GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
return sub;
}
+/*
+ * Generate the connection string for a subscription.
+ *
+ * This is deliberately separate from GetSubscription() because resolving
+ * conninfo for a server-based subscription has its own error paths (foreign
+ * server USAGE, user mapping, ForeignServerConnectionString()). Keeping it
+ * separate lets a caller load the subscription and decide whether a
+ * connection is actually needed, and check things such as whether the
+ * subscription is enabled, before risking those errors. Callers that never
+ * connect thus never hit them, which matters during restore.
+ */
+char *
+SubscriptionConninfo(Subscription *sub, bool aclcheck)
+{
+ HeapTuple tup;
+ Form_pg_subscription subform;
+ Datum datum;
+ char *conninfo;
+
+ tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(sub->oid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for subscription %u", sub->oid);
+
+ subform = (Form_pg_subscription) GETSTRUCT(tup);
+
+ if (OidIsValid(subform->subserver))
+ {
+ ForeignServer *server;
+ AclResult aclresult;
+
+ server = GetForeignServer(subform->subserver);
+
+ if (aclcheck)
+ {
+ aclresult = object_aclcheck(ForeignServerRelationId,
+ subform->subserver,
+ sub->owner, 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(sub->owner, false),
+ server->servername)));
+ }
+
+ conninfo = ForeignServerConnectionString(sub->owner, server);
+ }
+ else
+ {
+ datum = SysCacheGetAttrNotNull(SUBSCRIPTIONOID, tup,
+ Anum_pg_subscription_subconninfo);
+ conninfo = TextDatumGetCString(datum);
+ }
+
+ ReleaseSysCache(tup);
+
+ return conninfo;
+}
+
/*
* Return number of subscriptions defined in given database.
* Used by dropdb() to check if database can indeed be dropped.
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 67f5699b2c7..b52a45305a6 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1088,7 +1088,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
static void
AlterSubscription_refresh(Subscription *sub, bool copy_data,
- List *validate_publications)
+ List *validate_publications, char *conninfo)
{
char *err;
List *pubrels = NIL;
@@ -1112,12 +1112,19 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
WalReceiverConn *wrconn;
bool must_use_password;
+ /*
+ * Should not happen: CREATE/ALTER/DROP SUBSCRIPTION did not call
+ * SubscriptionConninfo() in a path where it's required.
+ */
+ if (!conninfo)
+ elog(ERROR, "no connection string provided for subscription");
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(conninfo, true, true, must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -1358,19 +1365,26 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data,
* Marks all sequences with INIT state.
*/
static void
-AlterSubscription_refresh_seq(Subscription *sub)
+AlterSubscription_refresh_seq(Subscription *sub, char *conninfo)
{
char *err = NULL;
WalReceiverConn *wrconn;
bool must_use_password;
List *subrel_states;
+ /*
+ * Should not happen: CREATE/ALTER/DROP SUBSCRIPTION did not call
+ * SubscriptionConninfo() in a path where it's required.
+ */
+ if (!conninfo)
+ elog(ERROR, "no connection string provided for subscription");
+
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
/* Try to connect to the publisher. */
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(sub->conninfo, true, true, must_use_password,
+ wrconn = walrcv_connect(conninfo, true, true, must_use_password,
sub->name, &err);
if (!wrconn)
ereport(ERROR,
@@ -1627,6 +1641,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
int max_retention;
bool retention_active;
char *new_conninfo = NULL;
+ char *orig_conninfo = NULL;
char *origin;
Subscription *sub;
Form_pg_subscription form;
@@ -1729,6 +1744,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
orig_conninfo_needed = false;
}
+ sub = GetSubscription(subid, 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
@@ -1736,7 +1753,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* there's no need to do an additional ACL check here; that will be done
* by the subscription worker.
*/
- sub = GetSubscription(subid, false, orig_conninfo_needed, false);
+ if (orig_conninfo_needed)
+ orig_conninfo = SubscriptionConninfo(sub, false);
retain_dead_tuples = sub->retaindeadtuples;
origin = sub->origin;
@@ -2227,7 +2245,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
sub->publications = stmt->publication;
AlterSubscription_refresh(sub, opts.copy_data,
- stmt->publication);
+ stmt->publication,
+ orig_conninfo);
}
break;
@@ -2282,7 +2301,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
sub->publications = publist;
AlterSubscription_refresh(sub, opts.copy_data,
- validate_publications);
+ validate_publications,
+ orig_conninfo);
}
break;
@@ -2321,7 +2341,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION ... REFRESH PUBLICATION");
- AlterSubscription_refresh(sub, opts.copy_data, NULL);
+ AlterSubscription_refresh(sub, opts.copy_data, NULL,
+ orig_conninfo);
break;
}
@@ -2334,7 +2355,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
errmsg("%s is not allowed for disabled subscriptions",
"ALTER SUBSCRIPTION ... REFRESH SEQUENCES"));
- AlterSubscription_refresh_seq(sub);
+ AlterSubscription_refresh_seq(sub, orig_conninfo);
break;
}
@@ -2406,7 +2427,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
char *err;
WalReceiverConn *wrconn;
- Assert(new_conninfo || orig_conninfo_needed);
+ Assert(new_conninfo || orig_conninfo);
/* Load the library providing us libpq calls. */
load_file("libpqwalreceiver", false);
@@ -2416,7 +2437,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
* available.
*/
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
- wrconn = walrcv_connect(new_conninfo ? new_conninfo : sub->conninfo,
+ wrconn = walrcv_connect(new_conninfo ? new_conninfo : orig_conninfo,
true, true, must_use_password, sub->name,
&err);
if (!wrconn)
diff --git a/src/backend/replication/logical/sequencesync.c b/src/backend/replication/logical/sequencesync.c
index ea24827aa9e..6d551d45791 100644
--- a/src/backend/replication/logical/sequencesync.c
+++ b/src/backend/replication/logical/sequencesync.c
@@ -815,7 +815,7 @@ LogicalRepSyncSequences(void)
* Establish the connection to the publisher for sequence synchronization.
*/
LogRepWorkerWalRcvConn =
- walrcv_connect(MySubscription->conninfo, true, true,
+ walrcv_connect(MySubscriptionConninfo, true, true,
must_use_password,
app_name.data, &err);
if (LogRepWorkerWalRcvConn == NULL)
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index a04b84ebc1d..e5101997cd3 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -1305,7 +1305,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
* so that synchronous replication can distinguish them.
*/
LogRepWorkerWalRcvConn =
- walrcv_connect(MySubscription->conninfo, true, true,
+ walrcv_connect(MySubscriptionConninfo, true, true,
must_use_password,
slotname, &err);
if (LogRepWorkerWalRcvConn == NULL)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 2548d3feb54..86fd5eff295 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -482,6 +482,7 @@ static MemoryContext LogicalStreamingContext = NULL;
WalReceiverConn *LogRepWorkerWalRcvConn = NULL;
Subscription *MySubscription = NULL;
+char *MySubscriptionConninfo = NULL;
static bool MySubscriptionValid = false;
static List *on_commit_wakeup_workers_subids = NIL;
@@ -5061,6 +5062,7 @@ void
maybe_reread_subscription(void)
{
Subscription *newsub;
+ char *new_conninfo;
bool started_tx = false;
/* When cache state is valid there is nothing to do here. */
@@ -5074,7 +5076,7 @@ maybe_reread_subscription(void)
started_tx = true;
}
- newsub = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
+ newsub = GetSubscription(MyLogicalRepWorker->subid, true);
if (newsub)
{
@@ -5097,6 +5099,9 @@ maybe_reread_subscription(void)
proc_exit(0);
}
+ /* allocated in transaction context */
+ new_conninfo = SubscriptionConninfo(newsub, true);
+
/* Exit if the subscription was disabled. */
if (!newsub->enabled)
{
@@ -5120,7 +5125,7 @@ maybe_reread_subscription(void)
* 'parallel' to any other value or the server decides not to stream the
* in-progress transaction.
*/
- if (strcmp(newsub->conninfo, MySubscription->conninfo) != 0 ||
+ if (strcmp(new_conninfo, MySubscriptionConninfo) != 0 ||
strcmp(newsub->name, MySubscription->name) != 0 ||
strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
newsub->binary != MySubscription->binary ||
@@ -5171,6 +5176,10 @@ maybe_reread_subscription(void)
MemoryContextDelete(MySubscription->cxt);
MySubscription = newsub;
+ /* Owned by ApplyContext */
+ pfree(MySubscriptionConninfo);
+ MySubscriptionConninfo = MemoryContextStrdup(ApplyContext, new_conninfo);
+
/* Change synchronous commit according to the user's wishes */
SetConfigOption("synchronous_commit", MySubscription->synccommit,
PGC_BACKEND, PGC_S_OVERRIDE);
@@ -5718,7 +5727,7 @@ run_apply_worker(void)
must_use_password = MySubscription->passwordrequired &&
!MySubscription->ownersuperuser;
- LogRepWorkerWalRcvConn = walrcv_connect(MySubscription->conninfo, true,
+ LogRepWorkerWalRcvConn = walrcv_connect(MySubscriptionConninfo, true,
true, must_use_password,
MySubscription->name, &err);
@@ -5831,7 +5840,7 @@ InitializeLogRepWorker(void)
LockSharedObject(SubscriptionRelationId, MyLogicalRepWorker->subid, 0,
AccessShareLock);
- MySubscription = GetSubscription(MyLogicalRepWorker->subid, true, true, true);
+ MySubscription = GetSubscription(MyLogicalRepWorker->subid, true);
if (MySubscription)
{
@@ -5850,6 +5859,11 @@ InitializeLogRepWorker(void)
proc_exit(0);
}
+ /* build conninfo in transaction context and copy to ApplyContext */
+ MySubscriptionConninfo =
+ MemoryContextStrdup(ApplyContext,
+ SubscriptionConninfo(MySubscription, true));
+
MySubscriptionValid = true;
if (!MySubscription->enabled)
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 65ce8e145fb..5a9c07fe8d6 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -173,7 +173,6 @@ typedef struct Subscription
* exceeded max_retention_duration, when
* defined */
Oid conflictlogrelid; /* conflict log table Oid */
- char *conninfo; /* Connection string to the publisher */
char *slotname; /* Name of the replication slot */
char *synccommit; /* Synchronous commit setting for worker */
char *walrcvtimeout; /* wal_receiver_timeout setting for worker */
@@ -222,9 +221,8 @@ typedef struct Subscription
#endif /* EXPOSE_TO_CLIENT_CODE */
-extern Subscription *GetSubscription(Oid subid, bool missing_ok,
- bool conninfo_needed,
- bool conninfo_aclcheck);
+extern Subscription *GetSubscription(Oid subid, bool missing_ok);
+extern char *SubscriptionConninfo(Subscription *sub, bool aclcheck);
extern void DisableSubscription(Oid subid);
extern int CountDBSubscriptions(Oid dbid);
diff --git a/src/include/replication/worker_internal.h b/src/include/replication/worker_internal.h
index 745b7d9e969..88cb7c1e252 100644
--- a/src/include/replication/worker_internal.h
+++ b/src/include/replication/worker_internal.h
@@ -247,6 +247,7 @@ extern PGDLLIMPORT struct WalReceiverConn *LogRepWorkerWalRcvConn;
/* Worker and subscription objects. */
extern PGDLLIMPORT Subscription *MySubscription;
+extern PGDLLIMPORT char *MySubscriptionConninfo;
extern PGDLLIMPORT LogicalRepWorker *MyLogicalRepWorker;
extern PGDLLIMPORT bool in_remote_transaction;
--
2.43.0
From 1b8ea3d6a17741a1c007f8478810480ce56114f4 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 13:38:17 -0700
Subject: [PATCH v4 2/8] Build subscription conninfo after checking that it's
enabled.
If a subscription is disabled, don't try to build conninfo because
that may generate a confusing error and try to disable an
already-disabled subscription.
Partially addresses finding 5 in report from linked discussion.
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Backpatch-through: 19
---
src/backend/replication/logical/worker.c | 28 +++++++++++++++---------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 86fd5eff295..e4baf29a206 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5099,9 +5099,6 @@ maybe_reread_subscription(void)
proc_exit(0);
}
- /* allocated in transaction context */
- new_conninfo = SubscriptionConninfo(newsub, true);
-
/* Exit if the subscription was disabled. */
if (!newsub->enabled)
{
@@ -5112,6 +5109,13 @@ maybe_reread_subscription(void)
apply_worker_exit();
}
+ /*
+ * May raise error, so build conninfo after checking that the subscription
+ * is enabled. Allocated in transaction context; must be copied to
+ * ApplyContext when we set MySubscriptionConninfo.
+ */
+ new_conninfo = SubscriptionConninfo(newsub, true);
+
/* !slotname should never happen when enabled is true. */
Assert(newsub->slotname);
@@ -5859,13 +5863,6 @@ InitializeLogRepWorker(void)
proc_exit(0);
}
- /* build conninfo in transaction context and copy to ApplyContext */
- MySubscriptionConninfo =
- MemoryContextStrdup(ApplyContext,
- SubscriptionConninfo(MySubscription, true));
-
- MySubscriptionValid = true;
-
if (!MySubscription->enabled)
{
ereport(LOG,
@@ -5875,6 +5872,17 @@ InitializeLogRepWorker(void)
apply_worker_exit();
}
+ /*
+ * May raise error for server-based subscriptions, so build conninfo after
+ * checking that the subscription is enabled. Build in transaction context
+ * and copy to ApplyContext.
+ */
+ MySubscriptionConninfo =
+ MemoryContextStrdup(ApplyContext,
+ SubscriptionConninfo(MySubscription, true));
+
+ MySubscriptionValid = true;
+
/*
* Restart the worker if retain_dead_tuples was enabled during startup.
*
--
2.43.0
From ec7b48e48830c9ca35af1d1e3cc8bc9a92687c4f Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 13:40:02 -0700
Subject: [PATCH v4 3/8] Be precise about when ALTER SUBSCRIPTION needs
conninfo.
Decide early whether the original conninfo is needed so that errors
happen consistently.
Addresses finding 12 in report from linked discussion.
Co-authored-by: Shlok Kyal <[email protected]>
Reported-by: Noah Misch <[email protected]>
Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Backpatch-through: 19
---
src/backend/commands/subscriptioncmds.c | 83 ++++++++++++++--------
src/test/regress/expected/subscription.out | 6 ++
src/test/regress/sql/subscription.sql | 7 ++
3 files changed, 68 insertions(+), 28 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index b52a45305a6..4ff5a15fc53 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1632,7 +1632,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
Datum values[Natts_pg_subscription];
HeapTuple tup;
Oid subid;
- bool orig_conninfo_needed = true;
+ bool orig_conninfo_needed = false;
bool update_tuple = false;
bool update_failover = false;
bool update_two_phase = false;
@@ -1714,37 +1714,64 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
if (supported_opts > 0)
parse_subscription_options(pstate, stmt->options, supported_opts, &opts);
+ sub = GetSubscription(subid, false);
+
/*
- * 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 DISABLE, 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.
+ * Determine in advance whether we need the original conninfo or not, so
+ * that errors are generated consistently in cases where we do need it;
+ * and not generated at all if we don't.
*/
- if (stmt->kind == ALTER_SUBSCRIPTION_ENABLED)
- {
- if (opts.specified_opts == SUBOPT_ENABLED && !opts.enabled)
- orig_conninfo_needed = false;
- }
- else if (stmt->kind == ALTER_SUBSCRIPTION_SERVER ||
- stmt->kind == ALTER_SUBSCRIPTION_CONNECTION)
- {
- orig_conninfo_needed = false;
- }
- else if (stmt->kind == ALTER_SUBSCRIPTION_OPTIONS)
+
+ /* conninfo needed when refreshing */
+ switch (stmt->kind)
{
- /* ... SET (slot_name = NONE) with no other options */
- if (opts.specified_opts == SUBOPT_SLOT_NAME && !opts.slot_name)
- orig_conninfo_needed = false;
- }
+ case ALTER_SUBSCRIPTION_REFRESH_PUBLICATION:
+ case ALTER_SUBSCRIPTION_REFRESH_SEQUENCES:
+ orig_conninfo_needed = true;
+ break;
- sub = GetSubscription(subid, false);
+ case ALTER_SUBSCRIPTION_SET_PUBLICATION:
+ case ALTER_SUBSCRIPTION_ADD_PUBLICATION:
+ case ALTER_SUBSCRIPTION_DROP_PUBLICATION:
+ /* opts.refresh defaults to true when the option is supported */
+ orig_conninfo_needed = opts.refresh;
+ break;
+
+ case ALTER_SUBSCRIPTION_ENABLED:
+ orig_conninfo_needed = opts.enabled && sub->retaindeadtuples;
+ break;
+
+ case ALTER_SUBSCRIPTION_OPTIONS:
+ {
+ if (sub->slotname)
+ {
+ if (IsSet(opts.specified_opts, SUBOPT_FAILOVER))
+ orig_conninfo_needed = true;
+ if (IsSet(opts.specified_opts, SUBOPT_TWOPHASE_COMMIT) &&
+ !opts.twophase)
+ orig_conninfo_needed = true;
+ }
+
+ if (IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES) &&
+ opts.retaindeadtuples)
+ orig_conninfo_needed = true;
+
+ if (IsSet(opts.specified_opts, SUBOPT_ORIGIN))
+ {
+ bool rdt;
+
+ rdt = IsSet(opts.specified_opts, SUBOPT_RETAIN_DEAD_TUPLES) ?
+ opts.retaindeadtuples : sub->retaindeadtuples;
+
+ if (rdt && pg_strcasecmp(opts.origin, LOGICALREP_ORIGIN_ANY) == 0)
+ orig_conninfo_needed = true;
+ }
+ }
+ break;
+
+ default:
+ break;
+ }
/*
* Skip ACL checks on the subscription's foreign server, if any. If
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 259db747334..d0955ca1159 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -229,6 +229,12 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
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, catalog-only forms don't construct conninfo
+ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = local);
+ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = off);
+ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = true);
+ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = false);
+ALTER SUBSCRIPTION regress_testsub6 SET PUBLICATION testpub WITH (refresh = false);
-- ok, test_server lacks user mapping, but replacing connection anyway
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 7718c742974..98304737adc 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -176,6 +176,13 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+-- ok, catalog-only forms don't construct conninfo
+ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = local);
+ALTER SUBSCRIPTION regress_testsub6 SET (synchronous_commit = off);
+ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = true);
+ALTER SUBSCRIPTION regress_testsub6 SET (disable_on_error = false);
+ALTER SUBSCRIPTION regress_testsub6 SET PUBLICATION testpub WITH (refresh = false);
+
-- ok, test_server lacks user mapping, but replacing connection anyway
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
--
2.43.0
From 4eef163f2724ab4043fa39321aeeed5975da33c9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 13:47:29 -0700
Subject: [PATCH v4 4/8] Always check foreign-server USAGE when resolving
subscription conninfo.
Previously, this was skipped in some cases to avoid raising errors
when conninfo wasn't even needed. That was wrong in cases where
conninfo was needed.
Now that we only build conninfo when needed, always perform the USAGE
check.
Addresses finding 7 in report from linked discussion.
Co-authored-by: Shlok Kyal <[email protected]>
Reported-by: Noah Misch <[email protected]>
Reviewed-by: Hayato Kuroda (Fujitsu) <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Backpatch-through: 19
---
src/backend/catalog/pg_subscription.c | 23 ++++++++++------------
src/backend/commands/subscriptioncmds.c | 9 +--------
src/backend/replication/logical/worker.c | 4 ++--
src/include/catalog/pg_subscription.h | 2 +-
src/test/regress/expected/subscription.out | 3 +++
src/test/regress/sql/subscription.sql | 3 +++
6 files changed, 20 insertions(+), 24 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 9083c5762cc..f1e8b624d8e 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -196,7 +196,7 @@ GetSubscription(Oid subid, bool missing_ok)
* connect thus never hit them, which matters during restore.
*/
char *
-SubscriptionConninfo(Subscription *sub, bool aclcheck)
+SubscriptionConninfo(Subscription *sub)
{
HeapTuple tup;
Form_pg_subscription subform;
@@ -216,18 +216,15 @@ SubscriptionConninfo(Subscription *sub, bool aclcheck)
server = GetForeignServer(subform->subserver);
- if (aclcheck)
- {
- aclresult = object_aclcheck(ForeignServerRelationId,
- subform->subserver,
- sub->owner, 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(sub->owner, false),
- server->servername)));
- }
+ aclresult = object_aclcheck(ForeignServerRelationId,
+ subform->subserver,
+ sub->owner, 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(sub->owner, false),
+ server->servername)));
conninfo = ForeignServerConnectionString(sub->owner, server);
}
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 4ff5a15fc53..fff4a0cb01f 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1773,15 +1773,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
break;
}
- /*
- * 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.
- */
if (orig_conninfo_needed)
- orig_conninfo = SubscriptionConninfo(sub, false);
+ orig_conninfo = SubscriptionConninfo(sub);
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 e4baf29a206..d60825f9683 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5114,7 +5114,7 @@ maybe_reread_subscription(void)
* is enabled. Allocated in transaction context; must be copied to
* ApplyContext when we set MySubscriptionConninfo.
*/
- new_conninfo = SubscriptionConninfo(newsub, true);
+ new_conninfo = SubscriptionConninfo(newsub);
/* !slotname should never happen when enabled is true. */
Assert(newsub->slotname);
@@ -5879,7 +5879,7 @@ InitializeLogRepWorker(void)
*/
MySubscriptionConninfo =
MemoryContextStrdup(ApplyContext,
- SubscriptionConninfo(MySubscription, true));
+ SubscriptionConninfo(MySubscription));
MySubscriptionValid = true;
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 5a9c07fe8d6..d2781a0b837 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -222,7 +222,7 @@ typedef struct Subscription
#endif /* EXPOSE_TO_CLIENT_CODE */
extern Subscription *GetSubscription(Oid subid, bool missing_ok);
-extern char *SubscriptionConninfo(Subscription *sub, bool aclcheck);
+extern char *SubscriptionConninfo(Subscription *sub);
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 d0955ca1159..f67ffab1f54 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -215,6 +215,9 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
+-- fail, connecting forms recheck USAGE on the foreign server
+ALTER SUBSCRIPTION regress_testsub6 REFRESH PUBLICATION;
+ERROR: subscription owner "regress_subscription_user3" does not have permission on foreign server "test_server"
-- 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"
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 98304737adc..47e2b6ef09c 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -161,6 +161,9 @@ BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
ABORT;
+-- fail, connecting forms recheck USAGE on the foreign server
+ALTER SUBSCRIPTION regress_testsub6 REFRESH PUBLICATION;
+
-- fails, cannot drop slot
DROP SUBSCRIPTION regress_testsub6;
--
2.43.0
From 65b089678c3724557f97ff0837cd202a35026e7d Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 18:18:25 -0700
Subject: [PATCH v4 5/8] For subscription DDL, demote user mapping checks to
WARNING.
The checks are useful to report to the user, but there's no reason to
raise an error. If needed while constructing conninfo, fdwconnection
will raise an error then.
Partially addresses finding 1, and addresses finding 13 in report from
the linked discussion.
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/commands/subscriptioncmds.c | 8 ++++----
src/backend/foreign/foreign.c | 14 +++++++++++++-
src/include/foreign/foreign.h | 1 +
src/test/regress/expected/subscription.out | 8 +++-----
src/test/regress/sql/subscription.sql | 5 +----
5 files changed, 22 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index fff4a0cb01f..343c4cbcccd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -806,8 +806,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_FOREIGN_SERVER, server->servername);
- /* make sure a user mapping exists */
- GetUserMapping(owner, server->serverid);
+ /* check user mapping */
+ GetUserMappingExtended(owner, server->serverid, WARNING);
serverid = server->serverid;
conninfo = ForeignServerConnectionString(owner, server);
@@ -2170,8 +2170,8 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
GetUserNameFromId(form->subowner, false),
new_server->servername));
- /* make sure a user mapping exists */
- GetUserMapping(form->subowner, new_server->serverid);
+ /* check user mapping */
+ GetUserMappingExtended(form->subowner, new_server->serverid, WARNING);
new_conninfo = ForeignServerConnectionString(form->subowner,
new_server);
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 821d45c1e11..73343f017b3 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -230,6 +230,16 @@ ForeignServerConnectionString(Oid userid, ForeignServer *server)
*/
UserMapping *
GetUserMapping(Oid userid, Oid serverid)
+{
+ return GetUserMappingExtended(userid, serverid, ERROR);
+}
+
+/*
+ * Like GetUserMapping(), but allows caller to specify an elevel. If elevel is
+ * less than ERROR, returns NULL if the user mapping doesn't exist.
+ */
+UserMapping *
+GetUserMappingExtended(Oid userid, Oid serverid, int elevel)
{
Datum datum;
HeapTuple tp;
@@ -252,10 +262,12 @@ GetUserMapping(Oid userid, Oid serverid)
{
ForeignServer *server = GetForeignServer(serverid);
- ereport(ERROR,
+ ereport(elevel,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("user mapping not found for user \"%s\", server \"%s\"",
MappingUserName(userid), server->servername)));
+
+ return NULL;
}
um = palloc_object(UserMapping);
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 92a55214fee..9b4532895a4 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -73,6 +73,7 @@ extern ForeignServer *GetForeignServerByName(const char *srvname,
extern char *ForeignServerConnectionString(Oid userid,
ForeignServer *server);
extern UserMapping *GetUserMapping(Oid userid, Oid serverid);
+extern UserMapping *GetUserMappingExtended(Oid userid, Oid serverid, int elevel);
extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid);
extern ForeignDataWrapper *GetForeignDataWrapperExtended(Oid fdwid,
uint16 flags);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index f67ffab1f54..e36f227129b 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -177,14 +177,12 @@ ERROR: permission denied for foreign server test_server
RESET SESSION AUTHORIZATION;
GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, need user mapping
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
-ERROR: user mapping not found for user "regress_subscription_user3", server "test_server"
-CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
--- fail, need CONNECTION clause
+-- warn, need user mapping, then fail, FDW doesn't support connections
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
+WARNING: user mapping not found for user "regress_subscription_user3", server "test_server"
ERROR: foreign data wrapper "test_fdw" does not support subscription connections
DETAIL: Foreign data wrapper must be defined with CONNECTION specified.
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 47e2b6ef09c..5ee13df6653 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -124,14 +124,11 @@ RESET SESSION AUTHORIZATION;
GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--- fail, need user mapping
+-- warn, need user mapping, then fail, FDW doesn't support connections
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
--- fail, need CONNECTION clause
-CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
-
RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
--
2.43.0
From 1dc48bdab5c9375ed01eef7804e9b1e41c1baaee Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 18:26:23 -0700
Subject: [PATCH v4 6/8] CREATE SUBSCRIPTION: do not construct conninfo
unnecessarily.
Still check that the creating user has USAGE privileges on the server,
and that the FDW supports subscription connections.
Addresses finding 1 in the report from the linked discussion.
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/commands/subscriptioncmds.c | 37 ++++++++++++++++------
src/backend/foreign/foreign.c | 4 +--
src/test/regress/expected/subscription.out | 4 +--
3 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 343c4cbcccd..da9963e22ad 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -677,8 +677,8 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
Datum values[Natts_pg_subscription];
Oid owner = GetUserId();
HeapTuple tup;
- Oid serverid;
- char *conninfo;
+ Oid serverid = InvalidOid;
+ char *conninfo = NULL;
char originname[NAMEDATALEN];
List *publications;
uint32 supported_opts;
@@ -799,30 +799,47 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
ForeignServer *server;
Assert(!stmt->conninfo);
- conninfo = NULL;
server = GetForeignServerByName(stmt->servername, false);
- aclresult = object_aclcheck(ForeignServerRelationId, server->serverid, owner, ACL_USAGE);
+ serverid = server->serverid;
+
+ /* check USAGE privileges on server */
+ aclresult = object_aclcheck(ForeignServerRelationId, serverid, owner, ACL_USAGE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_FOREIGN_SERVER, server->servername);
/* check user mapping */
GetUserMappingExtended(owner, server->serverid, WARNING);
- serverid = server->serverid;
- conninfo = ForeignServerConnectionString(owner, server);
+ /*
+ * Check conninfo if connecting; otherwise only check that the
+ * server's FDW supports connections.
+ */
+ if (opts.connect)
+ {
+ conninfo = ForeignServerConnectionString(owner, server);
+ walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
+ }
+ else
+ {
+ ForeignDataWrapper *fdw = GetForeignDataWrapper(server->fdwid);
+
+ if (!OidIsValid(fdw->fdwconnection))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("foreign-data wrapper \"%s\" does not support subscription connections",
+ fdw->fdwname),
+ errdetail("Foreign-data wrapper must be defined with CONNECTION specified.")));
+ }
}
else
{
Assert(stmt->conninfo);
- serverid = InvalidOid;
conninfo = stmt->conninfo;
+ walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
}
- /* Check the connection info string. */
- walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
-
publications = stmt->publication;
/* Everything ok, form a new tuple. */
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 73343f017b3..7ad8e8ee56b 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -209,9 +209,9 @@ ForeignServerConnectionString(Oid userid, ForeignServer *server)
if (!OidIsValid(fdw->fdwconnection))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("foreign data wrapper \"%s\" does not support subscription connections",
+ errmsg("foreign-data wrapper \"%s\" does not support subscription connections",
fdw->fdwname),
- errdetail("Foreign data wrapper must be defined with CONNECTION specified.")));
+ errdetail("Foreign-data wrapper must be defined with CONNECTION specified.")));
connection_datum = OidFunctionCall3(fdw->fdwconnection,
ObjectIdGetDatum(userid),
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index e36f227129b..715c84afaa9 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -180,8 +180,8 @@ SET SESSION AUTHORIZATION regress_subscription_user3;
-- warn, need user mapping, then fail, FDW doesn't support connections
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
WARNING: user mapping not found for user "regress_subscription_user3", server "test_server"
-ERROR: foreign data wrapper "test_fdw" does not support subscription connections
-DETAIL: Foreign data wrapper must be defined with CONNECTION specified.
+ERROR: foreign-data wrapper "test_fdw" does not support subscription connections
+DETAIL: Foreign-data wrapper must be defined with CONNECTION specified.
CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
--
2.43.0
From e879c3ad114c3f261c3869e517a1ee868a2f5cb8 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 20:28:47 -0700
Subject: [PATCH v4 7/8] Revert "Validate subscription conninfo on owner
change"
This reverts commit 1c9c35890421e96a91129b51f2c6446a6d95af95.
Raising errors during OWNER TO can cause problems during restore. An
upcoming commit will avoid other errors that can happen in this path.
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
doc/src/sgml/ref/alter_subscription.sgml | 7 -------
src/backend/commands/subscriptioncmds.c | 14 ++------------
src/test/regress/expected/subscription.out | 17 -----------------
src/test/regress/regress.c | 9 ---------
src/test/regress/sql/subscription.sql | 15 ---------------
5 files changed, 2 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 0f81af5608b..6fc3e07a2d5 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -53,13 +53,6 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
to alter the owner, you must be able to <literal>SET ROLE</literal> to the
new owning role. If the subscription has
<literal>password_required=false</literal>, only superusers can modify it.
- If the subscription uses a foreign server, the new owner must have
- <literal>USAGE</literal> privilege on the foreign server, a user mapping
- for the new owner or for <literal>PUBLIC</literal> must exist, and the
- connection string generated for the new owner must be valid. If the new
- owner is not a superuser and the subscription has
- <literal>password_required=true</literal>, the generated connection string
- must include a password.
</para>
<para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index da9963e22ad..9c9c0de04eb 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -3007,12 +3007,11 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
/*
* If the subscription uses a server, check that the new owner has USAGE
- * privileges on the server, that a user mapping exists, and that the
- * resulting connection string is valid for the new owner.
+ * privileges on the server and that a user mapping exists. Note: does not
+ * re-check the resulting connection string.
*/
if (OidIsValid(form->subserver))
{
- char *conninfo;
ForeignServer *server = GetForeignServer(form->subserver);
aclresult = object_aclcheck(ForeignServerRelationId, server->serverid, newOwnerId, ACL_USAGE);
@@ -3025,15 +3024,6 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
/* make sure a user mapping exists */
GetUserMapping(newOwnerId, server->serverid);
-
- conninfo = ForeignServerConnectionString(newOwnerId, server);
-
- /* Load the library providing us libpq calls. */
- load_file("libpqwalreceiver", false);
- /* Check the connection info string. */
- walrcv_check_conninfo(conninfo,
- form->subpasswordrequired &&
- !superuser_arg(newOwnerId));
}
form->subowner = newOwnerId;
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 715c84afaa9..5fcd6891e4c 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -9,10 +9,6 @@ CREATE FUNCTION test_fdw_connection(oid, oid, internal)
RETURNS text
AS :'regresslib', 'test_fdw_connection'
LANGUAGE C;
-CREATE FUNCTION test_fdw_connection_no_password(oid, oid, internal)
- RETURNS text
- AS :'regresslib', 'test_fdw_connection_no_password'
- LANGUAGE C;
CREATE ROLE regress_subscription_user LOGIN SUPERUSER;
CREATE ROLE regress_subscription_user2;
CREATE ROLE regress_subscription_user3 IN ROLE pg_create_subscription;
@@ -191,18 +187,6 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
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.
RESET SESSION AUTHORIZATION;
-GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user2;
-CREATE USER MAPPING FOR regress_subscription_user2 SERVER test_server OPTIONS(user 'foo');
-ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection_no_password;
-WARNING: changing the foreign-data wrapper connection function can cause the options for dependent objects to become invalid
--- fail, new owner's generated conninfo must satisfy password_required
-ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
-ERROR: password is required
-DETAIL: Non-superusers must provide a password in the connection string.
-ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
-WARNING: changing the foreign-data wrapper connection function can cause the options for dependent objects to become invalid
-DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
-REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
-- fail, subscription depends on the server and cannot be dropped by CASCADE
DROP SERVER test_server CASCADE;
ERROR: cannot drop server test_server because subscription regress_testsub6 depends on it
@@ -258,7 +242,6 @@ HINT: Use DROP ... CASCADE to drop the dependent objects too.
ALTER FOREIGN DATA WRAPPER test_fdw NO CONNECTION;
WARNING: removing the foreign-data wrapper connection function will cause dependent subscriptions to fail
DROP FUNCTION test_fdw_connection(oid, oid, internal);
-DROP FUNCTION test_fdw_connection_no_password(oid, oid, internal);
DROP FOREIGN DATA WRAPPER test_fdw;
-- fail - invalid connection string during ALTER
ALTER SUBSCRIPTION regress_testsub CONNECTION 'foobar';
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 14d301b3499..9801cdd1d8c 100644
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -742,15 +742,6 @@ test_fdw_connection(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
}
-PG_FUNCTION_INFO_V1(test_fdw_connection_no_password);
-Datum
-test_fdw_connection_no_password(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"));
-}
-
PG_FUNCTION_INFO_V1(is_catalog_text_unique_index_oid);
Datum
is_catalog_text_unique_index_oid(PG_FUNCTION_ARGS)
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 5ee13df6653..58082f1c268 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -12,10 +12,6 @@ CREATE FUNCTION test_fdw_connection(oid, oid, internal)
RETURNS text
AS :'regresslib', 'test_fdw_connection'
LANGUAGE C;
-CREATE FUNCTION test_fdw_connection_no_password(oid, oid, internal)
- RETURNS text
- AS :'regresslib', 'test_fdw_connection_no_password'
- LANGUAGE C;
CREATE ROLE regress_subscription_user LOGIN SUPERUSER;
CREATE ROLE regress_subscription_user2;
@@ -137,16 +133,6 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
RESET SESSION AUTHORIZATION;
-GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user2;
-CREATE USER MAPPING FOR regress_subscription_user2 SERVER test_server OPTIONS(user 'foo');
-ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection_no_password;
-
--- fail, new owner's generated conninfo must satisfy password_required
-ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
-
-ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
-DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
-REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
-- fail, subscription depends on the server and cannot be dropped by CASCADE
DROP SERVER test_server CASCADE;
@@ -206,7 +192,6 @@ DROP FUNCTION test_fdw_connection(oid, oid, internal);
ALTER FOREIGN DATA WRAPPER test_fdw NO CONNECTION;
DROP FUNCTION test_fdw_connection(oid, oid, internal);
-DROP FUNCTION test_fdw_connection_no_password(oid, oid, internal);
DROP FOREIGN DATA WRAPPER test_fdw;
--
2.43.0
From 6acc78cdf70263adc4e6cfe0fca1ebc7ac31dc2b Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 30 Jul 2026 18:30:48 -0700
Subject: [PATCH v4 8/8] When changing owner of a subscription, do not throw an
error.
Errors will be caught when the connection is actually used.
Restore uses multiple DDL commands to restore a subscription, so
checks of the intermediate state risk restore errors. In the future we
could address this with a more careful restoration order, but the
DDL-time errors are merely for convenience.
Addresses finding 2 in the report from the linked discussion.
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/[email protected]
Backpatch-through: 19
---
src/backend/commands/subscriptioncmds.c | 24 +++++++---------------
src/test/regress/expected/subscription.out | 5 +++++
src/test/regress/sql/subscription.sql | 4 ++++
3 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 9c9c0de04eb..8cb7d607412 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -3006,25 +3006,15 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
get_database_name(MyDatabaseId));
/*
- * If the subscription uses a server, check that the new owner has USAGE
- * privileges on the server and that a user mapping exists. Note: does not
- * re-check the resulting connection string.
+ * The privileges will be checked before the connection is actually used,
+ * so it does not need to be done here. Avoid unnecessary risk of errors
+ * here, which could interfere with restore.
+ *
+ * However, it is convenient to check if a user mapping exists, and raise
+ * a WARNING if not.
*/
if (OidIsValid(form->subserver))
- {
- ForeignServer *server = GetForeignServer(form->subserver);
-
- aclresult = object_aclcheck(ForeignServerRelationId, server->serverid, newOwnerId, ACL_USAGE);
- if (aclresult != ACLCHECK_OK)
- ereport(ERROR,
- errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("new subscription owner \"%s\" does not have permission on foreign server \"%s\"",
- GetUserNameFromId(newOwnerId, false),
- server->servername));
-
- /* make sure a user mapping exists */
- GetUserMapping(newOwnerId, server->serverid);
- }
+ GetUserMappingExtended(newOwnerId, form->subserver, WARNING);
form->subowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 5fcd6891e4c..7b672aac72e 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -191,6 +191,11 @@ RESET SESSION AUTHORIZATION;
DROP SERVER test_server CASCADE;
ERROR: cannot drop server test_server because subscription regress_testsub6 depends on it
HINT: Drop subscription regress_testsub6 first.
+-- ok, USAGE privilege on server not checked for OWNER TO, but warn
+-- about user mapping
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+WARNING: user mapping not found for user "regress_subscription_user2", server "test_server"
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
-- ok, lacks USAGE on test_server, but replacing connection anyway
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 58082f1c268..c8d9f80a499 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -136,6 +136,10 @@ RESET SESSION AUTHORIZATION;
-- fail, subscription depends on the server and cannot be dropped by CASCADE
DROP SERVER test_server CASCADE;
+-- ok, USAGE privilege on server not checked for OWNER TO, but warn
+-- about user mapping
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--
2.43.0
