On Fri, 2026-07-10 at 12:59 -0700, Noah Misch wrote:
> An Opus 4.8 review of commit 8185bb5 found two pg_dump+restore
> failure
> scenarios, visible in the attached test patch. (The patch also tests
> a
> REASSIGN OWNED finding, for which I started a distinct thread
> postgr.es/m/flat/[email protected]).
>
> Opus also emitted the attached report about these findings and
> others. I
> didn't examine the others closely. Finding-19, about invalidation
> callbacks,
> stood out as perhaps most exciting if true.
Patch attached.
Generating and validating the connection requires the subscription
owner to be set correctly, the foreign server ACLs to be set, and the
user mapping to exist. The checks at DDL time are were a convenient way
to catch errors, but end up being too strict because those things can
change before the connection is actually needed. In particular, pg_dump
does the DDL in parts (first creating the subscription, then changing
the owner), and we need the first part to succeed.
It would be nice to expand the pg_dump tests to cover this, but that
would require a test dependency on postgres_fdw (or some kind of built-
in test FDW), and I don't think we want that. So I just included SQL
tests.
I think there's a remaining bug involving retaindeadtuples
(228c3708685) where it still tries to connect during binary upgrade.
That can be seen if you add a $publisher->stop to line 317 (right
before the pg_upgrade that's supposed to succeed) in
004_subscription.pl.
Regards,
Jeff Davis
From 648b90412b801708b15dcb23ce63757451733072 Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Sun, 19 Jul 2026 11:26:41 -0700
Subject: [PATCH v1] Fix dump/restore of server-based subscriptions.
Defer connection validation until the time the connection is actually
used. A server-based subscription generates the connection string
based on the subscription owner and available user mapping, which may
change between the time the subscription is created and when it's
actually used.
In particular, dump/restore first creates the subscription and then
changes the owner. The creation must succeed even if the user mapping
doesn't exist for the restoring user, or if there's some other problem
with the connection string which might be fine after the owner is set
properly.
This change also affects ALTER SUBSCRIPTION ... OWNER TO, which
previously checked for a user mapping, and now does not (until the
connection is used). While that could be made to work with
dump/restore, it would make subscriptions dependent on the ACLs for
foreign servers, and move subscription creation to a later phase.
Reported-by: Noah Misch <[email protected]>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/20260710192533.4f.noahmisch%40microsoft.com
Backpatch-through: 19
---
doc/src/sgml/ref/create_subscription.sgml | 7 ++--
src/backend/commands/subscriptioncmds.c | 49 ++++++++++------------
src/backend/foreign/foreign.c | 28 +++++++++----
src/include/foreign/foreign.h | 1 +
src/test/regress/expected/subscription.out | 11 +++--
src/test/regress/sql/subscription.sql | 14 ++++---
6 files changed, 63 insertions(+), 47 deletions(-)
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 81fbf3487a4..82f960fa027 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -83,10 +83,9 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
<para>
A foreign server to use for the connection. The server's foreign data
wrapper must have a <replaceable>connection_function</replaceable>
- registered, and a user mapping for the subscription owner on the server
- must exist. Additionally, the subscription owner must have
- <literal>USAGE</literal> privileges on
- <replaceable>servername</replaceable>.
+ registered. When the connection is used, the subscription owner must
+ have <literal>USAGE</literal> privileges on
+ <replaceable>servername</replaceable> and a user mapping must exist.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 630d2498fa0..195bd939708 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -805,11 +805,26 @@ 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);
-
serverid = server->serverid;
- conninfo = ForeignServerConnectionString(owner, server);
+
+ if (opts.connect)
+ {
+ /* make sure a user mapping exists */
+ GetUserMapping(owner, server->serverid);
+
+ conninfo = ForeignServerConnectionString(owner, server);
+ }
+ else
+ {
+ /*
+ * If connect = false, don't check the connection information
+ * (necessary for dump/restore, which creates the subscription as
+ * the restoring user first and then changes the owner). However,
+ * still check that the server's FDW at least supports a
+ * connection function.
+ */
+ GetForeignServerConnectionFunction(server);
+ }
}
else
{
@@ -819,8 +834,9 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
conninfo = stmt->conninfo;
}
- /* Check the connection info string. */
- walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
+ /* Check the connection info string, if we need one. */
+ if (conninfo)
+ walrcv_check_conninfo(conninfo, opts.passwordrequired && !superuser());
publications = stmt->publication;
@@ -2904,27 +2920,6 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(aclresult, OBJECT_DATABASE,
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.
- */
- 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);
- }
-
form->subowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index 821d45c1e11..8de081a7377 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -194,15 +194,12 @@ GetForeignServerByName(const char *srvname, bool missing_ok)
/*
- * Retrieve connection string from server's FDW.
- *
- * NB: leaks into CurrentMemoryContext.
+ * Get the connection function from a server's FDW.
*/
-char *
-ForeignServerConnectionString(Oid userid, ForeignServer *server)
+Oid
+GetForeignServerConnectionFunction(ForeignServer *server)
{
ForeignDataWrapper *fdw;
- Datum connection_datum;
fdw = GetForeignDataWrapper(server->fdwid);
@@ -213,7 +210,24 @@ ForeignServerConnectionString(Oid userid, ForeignServer *server)
fdw->fdwname),
errdetail("Foreign data wrapper must be defined with CONNECTION specified.")));
- connection_datum = OidFunctionCall3(fdw->fdwconnection,
+ return fdw->fdwconnection;
+}
+
+
+/*
+ * Retrieve connection string from server's FDW.
+ *
+ * NB: leaks into CurrentMemoryContext.
+ */
+char *
+ForeignServerConnectionString(Oid userid, ForeignServer *server)
+{
+ Datum connection_datum;
+ Oid connection_function;
+
+ connection_function = GetForeignServerConnectionFunction(server);
+
+ connection_datum = OidFunctionCall3(connection_function,
ObjectIdGetDatum(userid),
ObjectIdGetDatum(server->serverid),
PointerGetDatum(NULL));
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h
index 92a55214fee..f0d2d1f6ad5 100644
--- a/src/include/foreign/foreign.h
+++ b/src/include/foreign/foreign.h
@@ -70,6 +70,7 @@ extern ForeignServer *GetForeignServerExtended(Oid serverid,
uint16 flags);
extern ForeignServer *GetForeignServerByName(const char *srvname,
bool missing_ok);
+extern Oid GetForeignServerConnectionFunction(ForeignServer *server);
extern char *ForeignServerConnectionString(Oid userid,
ForeignServer *server);
extern UserMapping *GetUserMapping(Oid userid, Oid serverid);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index d201ad764f0..5daa48a1c35 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -173,10 +173,6 @@ 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
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server PUBLICATION testpub WITH (slot_name = NONE, connect = false);
ERROR: foreign data wrapper "test_fdw" does not support subscription connections
@@ -184,10 +180,12 @@ 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;
+-- ok, user mapping is not needed with 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.
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -209,6 +207,11 @@ 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, changing owner does not require server access or a user mapping
+RESET SESSION AUTHORIZATION;
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
-- 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 86c402c59aa..f53a59326c9 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -120,11 +120,6 @@ 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);
-
-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);
@@ -132,9 +127,12 @@ RESET SESSION AUTHORIZATION;
ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
SET SESSION AUTHORIZATION regress_subscription_user3;
+-- ok, user mapping is not needed with connect = false
CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
+CREATE USER MAPPING FOR regress_subscription_user3 SERVER test_server OPTIONS(user 'foo', password 'secret');
+
RESET SESSION AUTHORIZATION;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -159,6 +157,12 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
DROP USER MAPPING FOR regress_subscription_user3 SERVER test_server;
+-- ok, changing owner does not require server access or a user mapping
+RESET SESSION AUTHORIZATION;
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user3;
+SET SESSION AUTHORIZATION regress_subscription_user3;
+
-- ok, test_server lacks user mapping, but replacing connection anyway
BEGIN;
ALTER SUBSCRIPTION regress_testsub6 CONNECTION 'dbname=regress_doesnotexist password=secret';
--
2.43.0
