pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedCREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
27+ messages / 6 participants
[nested] [flat]
* CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-10 19:59 Noah Misch <noah@leadboat.com>
0 siblings, 3 replies; 27+ messages in thread
From: Noah Misch @ 2026-07-10 19:59 UTC (permalink / raw)
To: pgsql@j-davis.com; +Cc: pgsql-hackers
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/20260710192533.4f.noahmisch@microsoft.com).
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.
commit ada4f72 (subscription-server-defect-tests)
Author: Noah Misch <nm@cqla.c.codeql-202606a.internal>
AuthorDate: Wed Jul 8 03:26:04 2026 +0000
Commit: Noah Misch <nm@cqla.c.codeql-202606a.internal>
CommitDate: Wed Jul 8 03:26:04 2026 +0000
Add TAP tests demonstrating CREATE SUBSCRIPTION ... SERVER defects
These tests reproduce three user-visible defects introduced by commit
8185bb5 (CREATE SUBSCRIPTION ... SERVER) and still present in the tree.
Each check asserts the current, buggy behavior and documents the
expected correct behavior, so a future fix flips the relevant assertion.
- pg_dump/restore of a server-based subscription fails because
CreateSubscription() resolves the user mapping for the restoring role
rather than the subscription owner, even with connect = false.
- Restoring such a subscription leaves it owned by the restoring
superuser, because ALTER SUBSCRIPTION ... OWNER TO is restored in the
main pass while the owner's GRANT USAGE on the foreign server is
deferred to the ACL pass.
- REASSIGN OWNED BY, run from a database other than the one holding the
subscription's foreign server, fails with "cache lookup failed for
foreign server", breaking multi-database role removal.
The scenarios use connect = false, so no publisher is required.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TePr48d48d89GukfGsNw9b
---
contrib/postgres_fdw/meson.build | 1 +
contrib/postgres_fdw/t/011_subscription_defects.pl | 233 +++++++++++++++++++++
2 files changed, 234 insertions(+)
diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build
index 3e2ed06..6cb204b 100644
--- a/contrib/postgres_fdw/meson.build
+++ b/contrib/postgres_fdw/meson.build
@@ -52,6 +52,7 @@ tests += {
'tests': [
't/001_auth_scram.pl',
't/010_subscription.pl',
+ 't/011_subscription_defects.pl',
],
},
}
diff --git a/contrib/postgres_fdw/t/011_subscription_defects.pl b/contrib/postgres_fdw/t/011_subscription_defects.pl
new file mode 100644
index 0000000..34ea757
--- /dev/null
+++ b/contrib/postgres_fdw/t/011_subscription_defects.pl
@@ -0,0 +1,233 @@
+# Copyright (c) 2021-2026, PostgreSQL Global Development Group
+
+# Demonstrations of user-visible defects in CREATE SUBSCRIPTION ... SERVER
+# (commit 8185bb5) that are still present in the tree.
+#
+# IMPORTANT: each block below asserts the *current, buggy* behavior so the
+# test passes and serves as an executable reproduction. Every block also
+# states, in comments, what the correct behavior should be. When a defect
+# is fixed, the corresponding assertion will start to fail and must be
+# flipped to the "correct behavior" noted alongside it.
+#
+# All scenarios use connect => false, so no publisher is required: the
+# defects are in the local CREATE/ALTER/REASSIGN and dump/restore paths,
+# not in replication itself.
+#
+# pg_subscription is a shared catalog, so a distinct subscription-owner role
+# is used per finding (and every cross-database catalog query is filtered by
+# subdbid) to keep the scenarios isolated from one another.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+$node->start;
+
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# The cluster superuser is the OS user under TAP, not necessarily "postgres";
+# capture it so the assertions below are independent of that name.
+my $super = $node->safe_psql('postgres', 'SELECT current_user');
+
+# One subscription-owner role per finding, plus a REASSIGN OWNED target.
+$node->safe_psql(
+ 'postgres', q{
+ CREATE ROLE owner1 LOGIN;
+ CREATE ROLE owner2 LOGIN;
+ CREATE ROLE owner4 LOGIN;
+ CREATE ROLE bob LOGIN;
+ GRANT pg_create_subscription TO owner1, owner2, owner4;
+});
+
+# Common foreign-server options. postgres_fdw's connection function builds a
+# libpq conninfo from these; with connect => false it is validated but never
+# used to connect, so the host/port need not be reachable.
+my $srv_opts = "OPTIONS (host 'localhost', port '1', dbname 'nx')";
+
+#############################################################################
+# Finding 1: pg_dump/pg_restore (and hence pg_upgrade) of a SERVER-based
+# subscription fails, because CreateSubscription() resolves the user mapping
+# for the role *executing the restore* (GetUserId()), not the subscription's
+# eventual owner -- even with connect => false.
+#
+# The dump is emitted "CREATE SUBSCRIPTION ... SERVER ... WITH (connect =
+# false ...)" followed later by "ALTER SUBSCRIPTION ... OWNER TO owner1", so
+# the CREATE runs as the restoring superuser. If that superuser has no user
+# mapping (and there is no PUBLIC mapping) on the server, the restore of a
+# perfectly valid dump fails.
+#
+# CORRECT BEHAVIOR: the dump should restore cleanly (a connect=false restore
+# should not require the restoring role to have a user mapping on the
+# server).
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect1_src');
+ $node->safe_psql(
+ 'defect1_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ -- Only owner1 has a mapping; the restoring superuser will not.
+ CREATE USER MAPPING FOR owner1 SERVER s OPTIONS (user 'repl', password 'secret');
+ GRANT USAGE ON FOREIGN SERVER s TO owner1;
+ GRANT CREATE ON DATABASE defect1_src TO owner1;
+ });
+ $node->safe_psql(
+ 'defect1_src', q{
+ SET SESSION AUTHORIZATION owner1;
+ CREATE SUBSCRIPTION defect1_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ my $dump = "$tempdir/defect1.sql";
+ command_ok(
+ [ 'pg_dump', '-f', $dump, $node->connstr('defect1_src') ],
+ 'finding 1: pg_dump of a SERVER-based subscription succeeds');
+
+ $node->safe_psql('postgres', 'CREATE DATABASE defect1_dst');
+
+ # BUG: restoring as the superuser (which has no mapping) fails at the
+ # CREATE SUBSCRIPTION step. When fixed, this should become command_ok().
+ command_fails_like(
+ [
+ 'psql', '--no-psqlrc', '-v', 'ON_ERROR_STOP=1',
+ '-f', $dump, '-d', $node->connstr('defect1_dst')
+ ],
+ qr/user mapping not found for user "\Q$super\E", server "s"/,
+ 'finding 1: restore fails - CREATE resolves the restorer\'s mapping, not the owner\'s'
+ );
+
+ # The subscription was therefore not restored into defect1_dst at all.
+ my $got = $node->safe_psql(
+ 'defect1_dst', q{
+ SELECT count(*) FROM pg_subscription
+ WHERE subname = 'defect1_sub'
+ AND subdbid = (SELECT oid FROM pg_database WHERE datname = 'defect1_dst')
+ });
+ is($got, '0', 'finding 1: subscription is missing after the failed restore');
+}
+
+#############################################################################
+# Finding 2: restoring a SERVER-based subscription whose owner derives its
+# foreign-server USAGE from a GRANT (rather than ownership) leaves the
+# subscription owned by the wrong role. AlterSubscriptionOwner_internal()
+# requires the new owner to hold USAGE on the server, but pg_dump emits
+# "ALTER SUBSCRIPTION ... OWNER TO owner2" in the main restore pass while
+# "GRANT USAGE ON FOREIGN SERVER" is deferred to the later ACL pass -- so at
+# the moment of the OWNER TO, owner2 does not yet have USAGE.
+#
+# CORRECT BEHAVIOR: the OWNER TO should succeed during restore and the
+# subscription should end up owned by owner2.
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect2_src');
+ $node->safe_psql(
+ 'defect2_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ -- PUBLIC mapping, so finding 1 does not mask this one: the
+ -- restoring superuser can resolve a connection.
+ CREATE USER MAPPING FOR PUBLIC SERVER s OPTIONS (user 'repl', password 'secret');
+ -- owner2's USAGE comes from a GRANT, not from owning the server.
+ GRANT USAGE ON FOREIGN SERVER s TO owner2;
+ GRANT CREATE ON DATABASE defect2_src TO owner2;
+ });
+ $node->safe_psql(
+ 'defect2_src', q{
+ SET SESSION AUTHORIZATION owner2;
+ CREATE SUBSCRIPTION defect2_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ my $dump = "$tempdir/defect2.sql";
+ command_ok(
+ [ 'pg_dump', '-f', $dump, $node->connstr('defect2_src') ],
+ 'finding 2: pg_dump succeeds');
+
+ $node->safe_psql('postgres', 'CREATE DATABASE defect2_dst');
+
+ # BUG: the CREATE SUBSCRIPTION succeeds (PUBLIC mapping), but the
+ # subsequent ALTER SUBSCRIPTION ... OWNER TO owner2 fails because the
+ # GRANT USAGE has not been restored yet. When fixed: command_ok().
+ command_fails_like(
+ [
+ 'psql', '--no-psqlrc', '-v', 'ON_ERROR_STOP=1',
+ '-f', $dump, '-d', $node->connstr('defect2_dst')
+ ],
+ qr/new subscription owner "owner2" does not have permission on foreign server "s"/,
+ 'finding 2: restore fails at OWNER TO because GRANT USAGE is in a later pass'
+ );
+
+ # BUG: the subscription is left owned by the restoring superuser instead
+ # of owner2. When fixed, the expected value is 'owner2'.
+ my $owner = $node->safe_psql(
+ 'defect2_dst', q{
+ SELECT subowner::regrole FROM pg_subscription
+ WHERE subname = 'defect2_sub'
+ AND subdbid = (SELECT oid FROM pg_database WHERE datname = 'defect2_dst')
+ });
+ is($owner, $super,
+ 'finding 2: subscription is left owned by the restoring superuser, not owner2'
+ );
+}
+
+#############################################################################
+# Finding 4: REASSIGN OWNED BY, run from any database other than the one
+# holding the subscription's foreign server, fails with an internal error.
+# pg_subscription is a shared catalog, so shdepReassignOwned() processes the
+# subscription from every database; AlterSubscriptionOwner_internal() then
+# looks the server up in the *current* database's (per-database)
+# pg_foreign_server and raises "cache lookup failed for foreign server".
+# This breaks the documented multi-database role-removal workflow (REASSIGN
+# OWNED / DROP OWNED in each database, then DROP ROLE).
+#
+# CORRECT BEHAVIOR: REASSIGN OWNED from another database should reassign the
+# subscription without an internal error (either succeeding, or failing with
+# a clean, user-facing permission error like the in-database case below).
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect4_src');
+ $node->safe_psql(
+ 'defect4_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ CREATE USER MAPPING FOR owner4 SERVER s OPTIONS (user 'repl', password 'secret');
+ GRANT USAGE ON FOREIGN SERVER s TO owner4;
+ GRANT CREATE ON DATABASE defect4_src TO owner4;
+ });
+ $node->safe_psql(
+ 'defect4_src', q{
+ SET SESSION AUTHORIZATION owner4;
+ CREATE SUBSCRIPTION defect4_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ # BUG: from the "postgres" database (which has no such foreign server),
+ # REASSIGN OWNED aborts with an internal (XX000) cache-lookup error.
+ my ($ret, $out, $err) =
+ $node->psql('postgres', 'REASSIGN OWNED BY owner4 TO bob');
+ isnt($ret, 0, 'finding 4: cross-database REASSIGN OWNED fails');
+ like(
+ $err,
+ qr/cache lookup failed for foreign server \d+/,
+ 'finding 4: cross-database REASSIGN OWNED raises an internal cache-lookup error'
+ );
+
+ # Contrast: run from the subscription's own database, the same command
+ # fails with a clean, user-facing permission error (bob lacks USAGE on
+ # the server) -- showing the cross-database internal error is the defect,
+ # not the reassignment being disallowed.
+ ($ret, $out, $err) =
+ $node->psql('defect4_src', 'REASSIGN OWNED BY owner4 TO bob');
+ isnt($ret, 0, 'finding 4 (contrast): in-database REASSIGN OWNED also fails here');
+ like(
+ $err,
+ qr/new subscription owner "bob" does not have permission on foreign server "s"/,
+ 'finding 4 (contrast): in-database REASSIGN OWNED gives a clean permission error'
+ );
+}
+
+done_testing();
# User-visible defects in commit 8185bb5 still present in master
**Commit:** 8185bb5 "CREATE SUBSCRIPTION ... SERVER." (Jeff Davis, 2026-03-06)
**Master at time of audit:** 3c9a38b
**Method:** 30-agent workflow — 16 area-focused finders (subscription DDL, foreign.c resolution, FDW DDL, dependencies, workers, postgres_fdw, extension packaging, pg_dump, psql, parser/nodes, security, NULL-sweep, docs, concurrency, end-to-end semantics) produced 40 raw findings; deduplicated to 19; the top 13 each adversarially verified by an independent skeptic instructed to refute, checking three gates: still in master, attributable to 8185bb5 (not pre-existing), and user-visible. All 13 were **confirmed** with code citations; the remaining 6 lower-priority findings are listed unverified.
Line numbers refer to current master.
---
## Live-cluster verification (findings 1, 2, 4, 6)
Reproduced on a running PostgreSQL 20devel cluster built from this tree (meson, `build/tmp_install`), using the regression suite's `test_fdw_connection` (returns a static conninfo, so no publisher is needed) and `connect=false` subscriptions.
| # | Static verdict | Live result | Observed |
|---|---|---|---|
| 1 | confirmed | **CONFIRMED** | Restore as postgres fails at `CREATE SUBSCRIPTION`: `ERROR 42704: user mapping not found for user "postgres", server "test_server"` at `GetUserMapping, foreign.c:255`. Control: identical CREATE run as the mapped owner (alice) succeeds. |
| 2 | confirmed | **CONFIRMED** | Restore's `CREATE` succeeds (PUBLIC mapping), then `ALTER SUBSCRIPTION ... OWNER TO alice` fails: `ERROR 42501: new subscription owner "alice" does not have permission on foreign server "test_server"` at `AlterSubscriptionOwner_internal, subscriptioncmds.c:2712`; subscription left owned by postgres. Control: `GRANT USAGE` first → OWNER TO succeeds. |
| 4 | confirmed | **CONFIRMED** | `REASSIGN OWNED BY alice TO bob` from the `postgres` database: `ERROR XX000: cache lookup failed for foreign server 16393`. Contrast: same command from the subscription's own database gives a clean permission error. |
| 6 | confirmed | **REFUTED** | Concurrent `DROP SERVER` does **not** race in — it blocks on `AccessExclusiveLock on object ... of class 1417` (server) and times out; no dangling reference results. See the rewritten entry below. |
**Bottom line:** three of the four requested findings reproduce exactly as described; **Finding 6 does not reproduce and is withdrawn.**
---
## Confirmed — major
### 1. pg_dump/pg_restore/pg_upgrade of SERVER subscriptions fail: CREATE resolves the connection for the creating user, not the eventual owner, even with connect=false — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:784`
**Symptom:** Restoring a dump of a database containing a server-based subscription fails with `ERROR: user mapping not found for user "postgres", server "..."` whenever the restoring role has no user mapping (and no PUBLIC mapping) on that server — even though the dump emits `connect = false`. pg_restore errors out; pg_upgrade aborts mid-restore on a perfectly valid source cluster.
**Repro:** As superuser: `CREATE SERVER s ...; CREATE USER MAPPING FOR alice SERVER s ...; GRANT USAGE ON FOREIGN SERVER s TO alice;` As alice: `CREATE SUBSCRIPTION sub SERVER s PUBLICATION pub WITH (connect=false, slot_name=NONE);` Then `pg_dump db | psql newdb` as postgres, or pg_upgrade the cluster: the emitted CREATE SUBSCRIPTION fails.
**Mechanism:** `CreateSubscription()` sets `owner = GetUserId()` (line 654) and in the servername branch (771–798) unconditionally — regardless of `connect=false` — calls `GetUserMapping(owner, serverid)` (line 784), which errors with no superuser bypass (foreign.c:251–258), then `ForeignServerConnectionString()` and `walrcv_check_conninfo()`. `dumpSubscription()` emits `CREATE SUBSCRIPTION ... SERVER ... WITH (connect = false, ...)` executed as the restore role; ownership is applied only afterwards via `ALTER SUBSCRIPTION ... OWNER TO`. pg_upgrade runs `pg_restore --exit-on-error` and aborts; binary-upgrade mode additionally emits `ALTER SUBSCRIPTION ... ENABLE`, hitting the same path via `GetSubscription(conninfo_needed=true)`. The docs' recommended setup (mapping only FOR the subscribing user) triggers this; pg_dump.sgml:1735–1748 promises subscription dumps restore without remote access. Follow-ups e5c4058/702e9df fixed only ALTER/DROP paths.
**Verifier notes:** Confirmed on all gates. Restore succeeds only if the restoring role happens to have a role-specific or PUBLIC mapping, or with `--use-set-session-authorization`; pg_upgrade uses neither escape. pg_dump TAP tests cover only CONNECTION-based subscriptions.
### 2. Restore ordering: ALTER SUBSCRIPTION ... OWNER TO fails because GRANT USAGE ON FOREIGN SERVER restores later, in the ACL pass — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:2710`
**Symptom:** Restore errors at the emitted `ALTER SUBSCRIPTION ... OWNER TO`: `new subscription owner "alice" does not have permission on foreign server ...`, because the GRANT has not been restored yet. Under psql the subscription silently stays owned by the bootstrap role (wrong catalog state); `pg_restore --exit-on-error` and pg_upgrade abort.
**Repro:** Grant-based (non-owner) subscription owner; dump; restore into a fresh database as superuser: CREATE SUBSCRIPTION succeeds, the following `ALTER SUBSCRIPTION sub OWNER TO alice` errors.
**Mechanism:** `AlterSubscriptionOwner_internal()` (2706–2720, added by 8185bb5) requires `object_aclcheck(ForeignServerRelationId, ..., ACL_USAGE)` plus `GetUserMapping(newOwnerId, ...)` before changing owner. pg_restore emits `ALTER ... OWNER TO` inside the subscription's own TOC entry in RESTORE_PASS_MAIN (`_printTocEntry`; SUBSCRIPTION is in `_getObjectDescription`, pg_backup_archiver.c:3862), while `GRANT USAGE ON FOREIGN SERVER` is an ACL TOC entry deferred by `_tocEntryRestorePass` (3366–3372) to RESTORE_PASS_ACL, strictly after all main-pass items even in serial restore. Plain-script dumps have the same order. Only new owners who own the server or are superuser restore cleanly. CONNECTION-based subscriptions have no such check — a new regression.
**Verifier notes:** Confirmed. Current master reads `GetForeignServer(form->subserver)` after the 11f8018 refactor; behavior identical.
### 3. postgres_fdw connection function embeds session-only credentials (SCRAM passthrough / GSS delegation), so CREATE succeeds but apply workers can never authenticate
**Where:** `contrib/postgres_fdw/connection.c:580`
**Symptom:** `CREATE SUBSCRIPTION ... SERVER` over a `use_scram_passthrough` server succeeds (connects, creates the remote slot) when the creator logged in via SCRAM — but apply/tablesync workers can never connect to a SCRAM-authenticated publisher. The subscription never replicates; the log fills with connection errors as the launcher restarts the worker. Non-superuser owners instead get a misleading `password is required` at CREATE. Undocumented.
**Mechanism:** `construct_connection_params()` (580–614) appends `scram_client_key`/`scram_server_key`/`require_auth` only when `MyProcPort != NULL && MyProcPort->has_scram_keys`; `MyProcPort` is assigned solely in backend_startup.c:177, so it is NULL in logical-replication background workers. The DDL session has keys, so CreateSubscription's `walrcv_check_conninfo`/`walrcv_connect` succeed; workers rebuild the conninfo via `GetSubscription → ForeignServerConnectionString → postgres_fdw_connection` (worker.c:5826) with no keys and no password. Superuser-owned subscriptions loop on publisher auth failure; non-superuser owners abort in `check_conn_params()`/`libpqrcv_check_conninfo`. GSSAPI delegated credentials have the same session-vs-worker asymmetry (`be_gssapi_get_delegation(MyProcPort)`, connection.c:769). postgres-fdw.sgml's Subscription Management section claims option parity with no caveat. No DDL-time rejection or warning exists.
**Verifier notes:** Confirmed. The MyProcPort-conditional SCRAM code pre-existed but was only reachable from client backends; 8185bb5 exposed it to bgworkers via `postgres_fdw_connection`, so the omission is attributable.
### 4. REASSIGN OWNED BY fails with XX000 "cache lookup failed for foreign server" when the role owns a server-based subscription in another database — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:2708`
**Symptom:** `REASSIGN OWNED BY old TO new`, run in any database other than the subscription's, aborts with the internal error `cache lookup failed for foreign server NNN` (XX000). The documented multi-database role-removal workflow (REASSIGN OWNED in each database, then DROP ROLE) breaks.
**Mechanism:** pg_subscription is a shared catalog, so its pg_shdepend row has `dbid = InvalidOid`, and `shdepReassignOwned` processes it from every database, calling `AlterSubscriptionOwner_internal`. 8185bb5 added a lookup of the subscription's foreign server there — but pg_foreign_server is per-database, so from any other database `GetForeignServerExtended` raises `elog(ERROR, "cache lookup failed for foreign server %u")`. Pre-commit code had no server lookup and succeeded. In the freak case of an OID collision, the ACL/user-mapping checks would be evaluated against an unrelated server of the current database.
**Verifier notes:** Confirmed. The literal `GetForeignServer()` call shape is from refactor 11f8018, but 8185bb5's original `object_aclcheck` + `GetUserMapping` fails identically cross-database (same XX000 for non-superusers via `object_aclmask_ext`; superusers fail in `GetUserMapping`). Broken since 8185bb5 either way.
### 5. Worker-side conninfo resolution errors preempt clean disabled-subscription handling and can permanently disable a live SERVER subscription via disable_on_error
**Where:** `src/backend/replication/logical/worker.c:5077`
**Symptom:** Disabling a SERVER subscription and dropping its user mapping in one transaction makes the running worker exit with `ERROR: user mapping not found ...` plus a bumped `apply_error_count` and (with `disable_on_error`) a misleading "disabled because of an error" LOG, instead of the clean "will stop because the subscription was disabled". Rotating a live mapping via DROP+CREATE (separate commits) can permanently disable the subscription if the worker rereads in the gap.
**Mechanism:** `maybe_reread_subscription()` (worker.c:5077) calls `GetSubscription(subid, true, true, true)`, which runs `object_aclcheck` and `ForeignServerConnectionString → postgres_fdw_connection → GetUserMapping` — erroring before the `newsub->enabled` clean-exit check at worker.c:5101. The error unwinds to `start_apply`'s PG_CATCH (5644–5673): with `disableonerr` → `DisableSubscriptionAndExit` (apply_error_count++, misleading LOG); else pgstat error + rethrow. Pre-8185bb5, `GetSubscription` could not fail for an existing subscription. Follow-ups e5c4058/702e9df fixed this class only in ALTER/DROP DDL paths.
**Verifier notes:** Confirmed; all cited lines match master. (One label fix: the third `maybe_reread_subscription` caller is worker.c:740, stream handling.)
### 6. ~~CREATE/ALTER SUBSCRIPTION ... SERVER takes no lock on the foreign server, so a concurrent DROP SERVER leaves a dangling subserver OID~~ — REFUTED on a live cluster
**Original claim:** `CreateSubscription`/`AlterSubscription` look up the server via `GetForeignServerByName()` (a syscache read) and record the dependency without locking the server, so a concurrent `DROP SERVER` — whose `findDependentObjects` MVCC scan can't see the uncommitted pg_depend row — could succeed and leave `pg_subscription.subserver` dangling (worker restart loop, DROP SUBSCRIPTION failure, pg_dump NULL-deref crash).
**Why it's wrong:** `recordDependencyOn` → `recordMultipleDependencies` calls `dependencyLockAndCheckObject()` (src/backend/catalog/pg_depend.c:121) **before** inserting the pg_depend row. That function takes an `AccessShareLock` on the referenced server via `LockDatabaseObject()` and then rechecks the object still exists, with the express purpose (per its header comment) to "make sure that we don't record a bogus reference permanently in the catalogs." The lock is held to end of transaction and conflicts with the `AccessExclusiveLock` that `DROP SERVER` acquires. This is a generic backstop in the dependency layer that both the finder and its adversarial verifier overlooked — they reasoned only about MVCC row visibility.
**Live test (PostgreSQL 20devel from this tree):**
- With `CREATE SUBSCRIPTION s6 SERVER test_server ... (connect=false)` held in an open transaction, a concurrent `DROP SERVER test_server` **blocked** and hit `lock_timeout`: `ERROR: canceling statement due to lock timeout / CONTEXT: waiting for AccessExclusiveLock on object 16427 of class 1417`. After commit, `s6.subserver` still resolved to the live server — no dangling reference.
- `pg_locks` during the open transaction shows the CREATE holding `AccessShareLock` on `pg_foreign_server` objid 16427; the `ALTER SUBSCRIPTION ... SERVER` path holds the identical lock.
- Once the subscription is committed, `DROP SERVER` (and `DROP SERVER CASCADE`) fail cleanly with `cannot drop server test_server because subscription s6 depends on it`; `pg_dump` of the database succeeds.
- The pre-dependency window (server lookup → `recordDependencyOn`) is also safe: if a `DROP SERVER` commits there, `dependencyLockAndCheckObject`'s existence recheck aborts the CREATE with an error rather than persisting a dangling OID.
No supported sequence produces a dangling `subserver`, so the downstream symptoms (worker restart loop, DROP failure, pg_dump crash) are unreachable. Finding withdrawn.
### 7. ALTER SUBSCRIPTION commands that connect to the publisher skip the foreign-server USAGE privilege check
**Where:** `src/backend/commands/subscriptioncmds.c:1571`
**Symptom:** A subscription owner whose USAGE on the foreign server was revoked can still make the server connect to the remote host with the user mapping's credentials via ALTER SUBSCRIPTION (REFRESH PUBLICATION/SEQUENCES, SET/ADD/DROP PUBLICATION with refresh, SET (failover/two_phase), retain_dead_tuples checks) — including dropping remote tablesync slots and altering remote slots — while the apply worker, CREATE, DROP, and OWNER TO all enforce USAGE. REVOKE appears ineffective to the DBA.
**Mechanism:** `AlterSubscription` calls `GetSubscription(subid, false, orig_conninfo_needed, false)` — `conninfo_aclcheck=false` — so the conninfo is built via `ForeignServerConnectionString` without `object_aclcheck`. That conninfo feeds foreground `walrcv_connect` in `AlterSubscription_refresh` (line 1060, incl. `ReplicationSlotDropAtPubNode`), `AlterSubscription_refresh_seq` (1312), and the failover/two_phase/check_pub_rdt block (2223). The justifying comment says the ACL check "will be done by the subscription worker", but these connections happen in the ALTER command itself. CREATE (779), ALTER..SERVER (1929), OWNER TO (2710), `construct_subserver_conninfo` (2281), and worker.c (5077/5826) all enforce USAGE — the ALTER paths are the inconsistent gap.
**Verifier notes:** Confirmed. Not an escalation (the owner uses their own mapping on their own subscription); the impact is that REVOKE USAGE fails to stop owner-initiated publisher connections.
---
## Confirmed — minor
### 8. \dew / \dew+ do not display the FDW connection function
**Where:** `src/bin/psql/describe.c:6188`
`listForeignDataWrappers()` shows Handler and Validator but never fdwconnection, in either mode; `git grep fdwconnection src/bin/psql` is empty. A DBA cannot see whether an FDW supports `CREATE SUBSCRIPTION ... SERVER` without querying the catalog directly. The commit updated \dRs+ (Server column) but missed \dew; sibling omissions from the same commit (pg_dump b71bf3b, tab completion 5fa7837, catalogs.sgml 90630ec "missed in commit 8185bb5347") were all treated as bugs and fixed.
### 9. DROP SUBSCRIPTION of a server-based subscription fails outright when the conninfo cannot be constructed and any relation is not READY, even with slot_name = NONE
**Where:** `src/backend/commands/subscriptioncmds.c:2522`
With non-READY pg_subscription_rel rows, DropSubscription calls `construct_subserver_conninfo()`, which traps only ACL failures into `*err`; `ForeignServerConnectionString()` failures (e.g. dropped user mapping) still ereport — a bare `user mapping not found` with no drop context and no HINT, aborting before both tolerant paths (clean return for `wrconn == NULL && !slotname`, and the hinted `ReportSlotConnectionError`). Conninfo-based subscriptions in the same state drop cleanly. Follow-up 702e9df fixed only the `rstates == NIL` case and its commit message acknowledges this residual case. The verifier notes this is a knowingly accepted limitation per the code comment — but it remains user-visible, hint-less, and untested (the regression suite covers only the slot-set variant, subscription.out:218).
### 10. DROP SERVER ... CASCADE cannot drop a dependent subscription, contradicting drop_server.sgml, with no hint and no doc of the restriction
**Where:** `doc/src/sgml/ref/drop_server.sgml:65` / `src/backend/catalog/dependency.c:907`
`findDependentObjects` unconditionally errors for any dependent in a shared catalog before the deptype/CASCADE handling is consulted, so `DROP SERVER s CASCADE` fails with `cannot drop server s because subscription sub depends on it` — while drop_server.sgml's CASCADE text promises automatic dropping. Blocking the drop is intentional (doDeletion cannot drop subscriptions or orphan the remote slot), but the restriction is documented nowhere, the error has no hint, and no test covers it. DROP FDW CASCADE and DROP OWNED hit it transitively.
### 11. create_foreign_data_wrapper.sgml omits the connection function's required text return type
**Where:** `doc/src/sgml/ref/create_foreign_data_wrapper.sgml:113`
The CONNECTION entry documents the three argument types only; `lookup_fdw_connection_func()` (foreigncmds.c:542–549) rejects any return type other than text. An author following the docs (`RETURNS varchar`) gets `function my_conn must return type text` on valid-per-docs input. Adjacent HANDLER/VALIDATOR entries do state their return-type contracts; nothing in the docs states the result is used as a libpq conninfo.
### 12. ALTER SUBSCRIPTION forms that never use a connection fail on server-based subscriptions when the conninfo cannot be constructed
**Where:** `src/backend/commands/subscriptioncmds.c:1557`
Purely local operations — `SKIP (lsn ...)`, `SET (disable_on_error/binary/origin/synchronous_commit ...)`, SET/ADD/DROP PUBLICATION `WITH (refresh = false)` — raise `user mapping not found` on a server-based subscription (even disabled), because `orig_conninfo_needed` stays true for every kind except the four e5c4058 exemptions (DISABLE, SERVER, CONNECTION, lone SET (slot_name=NONE)). The same commands never validate conninfo for CONNECTION-based subscriptions. Verifier note: a blanket exemption is impossible for OPTIONS/PUBLICATION kinds (refresh=true, failover, two_phase, retain_dead_tuples genuinely need conninfo) — the gap is the lack of per-option conditioning. Repro requires the mapping to exist at CREATE and be dropped after, with no PUBLIC fallback.
### 13. CREATE SUBSCRIPTION ... SERVER checks user-mapping existence before FDW subscription capability
**Where:** `src/backend/commands/subscriptioncmds.c:784` (comment at 783)
Pointing CREATE SUBSCRIPTION at a server of an FDW without a connection function (e.g. file_fdw) first reports `user mapping not found`; after the user creates a useless mapping, the second attempt reveals the real, unfixable `foreign data wrapper "file_fdw" does not support subscription connections`. The capability check lives inside `ForeignServerConnectionString` (foreign.c:209–214), unreachable until a mapping exists, though it needs no mapping and could run first. Same ordering in ALTER ... SERVER (1940/1942).
---
## Unverified (found, ranked below the 13-verifier cap; not adversarially checked)
### 14. User-mapping passwords logged verbatim at DEBUG1 on every worker start (minor/medium)
`SetupApplyOrSyncWorker()` (worker.c:5988–5990) logs `connecting to publisher using connection string "%s"` with `MySubscription->conninfo`. The elog pre-dates the commit, but 8185bb5 newly routes pg_user_mapping secrets (password) into that string via `postgres_fdw_connection`. Previously the logged string only duplicated pg_subscription.subconninfo, and user-mapping options were never emitted anywhere; there is no redaction (cf. `libpqrcv_get_conninfo`'s password stripping). Anyone with log access sees the password.
### 15. findDependentObjects rejects shared-catalog dependents before lock-and-recheck: DROP SERVER spuriously fails while a concurrent DROP SUBSCRIPTION is committing (minor/medium)
The dependency.c:907 shared-catalog error runs before `AcquireDeletionLock()`/`systable_recheck_tuple()`, so DROP SERVER errors instead of waiting out a concurrent DROP SUBSCRIPTION that has already deleted its pg_depend row but is still dropping the remote slot before commit. Any other dependent class would block and then succeed.
### 16. fdwhandler.sgml never mentions connection functions (minor/medium)
The FDW-author chapter still says an author implements "a handler function, and optionally a validator function"; the new third SQL-registered support function — its contract (user oid, server oid, internal → text libpq conninfo) — is absent from the chapter. The partial signature in create_foreign_data_wrapper.sgml is the only author-facing documentation.
### 17. alter_subscription.sgml documents no requirements for ALTER SUBSCRIPTION ... SERVER and none of the new OWNER TO failure conditions (minor/medium)
ALTER ... SERVER requires the subscription owner (not the invoker) to have USAGE and a user mapping on the new server, and its FDW to have a connection function; OWNER TO requires the same of the new owner. None of these appear on the ALTER SUBSCRIPTION page, while the equivalent CREATE-time requirements are documented.
### 18. Server's application_name option overrides the subscription name on the publisher (minor/low)
`construct_connection_params()` serializes a server-level `application_name` into the conninfo; `libpqrcv_connect` passes the subscription name only as `fallback_application_name`, which loses. pg_stat_replication shows the shared FDW name and `synchronous_standby_names` matching by subscription name silently fails. CONNECTION-based subscriptions don't auto-inject this.
### 19. Lost-invalidation window at worker startup can leave a worker on stale server-derived conninfo indefinitely (minor/low)
`InitializeLogRepWorker()` resolves conninfo via `GetSubscription` (worker.c:5826) before registering the FOREIGNSERVEROID/USERMAPPINGOID/FOREIGNDATAWRAPPEROID invalidation callbacks (5896–5916); an ALTER SERVER/USER MAPPING committing in that gap is lost, and no lock serializes server/mapping DDL against worker startup (unlike subscription DDL). Sub-millisecond window; low confidence.
---
## Coverage notes
- Areas swept that produced no surviving findings: grammar/keyword regressions, node read/write/copy/equal support, extension upgrade-path state divergence (1.2→1.3 vs fresh install), tab completion (fixed post-commit by 5fa7837), catalogs.sgml (fixed post-commit by 90630ec), pg_subscription view masking, connection-string quoting/injection in `appendEscapedValue`, direct SQL calls of the connection function (nominal three-arg signature includes `internal`, blocking SQL calls).
- Already-fixed follow-ups were excluded by instruction and by per-file `git log 8185bb5..master` checks in every finder and verifier.
- Verification was static analysis + git archaeology only; no repros were executed against a running cluster.
Attachments:
[text/plain] test-subscription-server-v0.patch (12.1K, ../../20260710195902.4f.noahmisch@microsoft.com/2-test-subscription-server-v0.patch)
download | inline diff:
commit ada4f72 (subscription-server-defect-tests)
Author: Noah Misch <nm@cqla.c.codeql-202606a.internal>
AuthorDate: Wed Jul 8 03:26:04 2026 +0000
Commit: Noah Misch <nm@cqla.c.codeql-202606a.internal>
CommitDate: Wed Jul 8 03:26:04 2026 +0000
Add TAP tests demonstrating CREATE SUBSCRIPTION ... SERVER defects
These tests reproduce three user-visible defects introduced by commit
8185bb5 (CREATE SUBSCRIPTION ... SERVER) and still present in the tree.
Each check asserts the current, buggy behavior and documents the
expected correct behavior, so a future fix flips the relevant assertion.
- pg_dump/restore of a server-based subscription fails because
CreateSubscription() resolves the user mapping for the restoring role
rather than the subscription owner, even with connect = false.
- Restoring such a subscription leaves it owned by the restoring
superuser, because ALTER SUBSCRIPTION ... OWNER TO is restored in the
main pass while the owner's GRANT USAGE on the foreign server is
deferred to the ACL pass.
- REASSIGN OWNED BY, run from a database other than the one holding the
subscription's foreign server, fails with "cache lookup failed for
foreign server", breaking multi-database role removal.
The scenarios use connect = false, so no publisher is required.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TePr48d48d89GukfGsNw9b
---
contrib/postgres_fdw/meson.build | 1 +
contrib/postgres_fdw/t/011_subscription_defects.pl | 233 +++++++++++++++++++++
2 files changed, 234 insertions(+)
diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build
index 3e2ed06..6cb204b 100644
--- a/contrib/postgres_fdw/meson.build
+++ b/contrib/postgres_fdw/meson.build
@@ -52,6 +52,7 @@ tests += {
'tests': [
't/001_auth_scram.pl',
't/010_subscription.pl',
+ 't/011_subscription_defects.pl',
],
},
}
diff --git a/contrib/postgres_fdw/t/011_subscription_defects.pl b/contrib/postgres_fdw/t/011_subscription_defects.pl
new file mode 100644
index 0000000..34ea757
--- /dev/null
+++ b/contrib/postgres_fdw/t/011_subscription_defects.pl
@@ -0,0 +1,233 @@
+# Copyright (c) 2021-2026, PostgreSQL Global Development Group
+
+# Demonstrations of user-visible defects in CREATE SUBSCRIPTION ... SERVER
+# (commit 8185bb5) that are still present in the tree.
+#
+# IMPORTANT: each block below asserts the *current, buggy* behavior so the
+# test passes and serves as an executable reproduction. Every block also
+# states, in comments, what the correct behavior should be. When a defect
+# is fixed, the corresponding assertion will start to fail and must be
+# flipped to the "correct behavior" noted alongside it.
+#
+# All scenarios use connect => false, so no publisher is required: the
+# defects are in the local CREATE/ALTER/REASSIGN and dump/restore paths,
+# not in replication itself.
+#
+# pg_subscription is a shared catalog, so a distinct subscription-owner role
+# is used per finding (and every cross-database catalog query is filtered by
+# subdbid) to keep the scenarios isolated from one another.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+$node->start;
+
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
+# The cluster superuser is the OS user under TAP, not necessarily "postgres";
+# capture it so the assertions below are independent of that name.
+my $super = $node->safe_psql('postgres', 'SELECT current_user');
+
+# One subscription-owner role per finding, plus a REASSIGN OWNED target.
+$node->safe_psql(
+ 'postgres', q{
+ CREATE ROLE owner1 LOGIN;
+ CREATE ROLE owner2 LOGIN;
+ CREATE ROLE owner4 LOGIN;
+ CREATE ROLE bob LOGIN;
+ GRANT pg_create_subscription TO owner1, owner2, owner4;
+});
+
+# Common foreign-server options. postgres_fdw's connection function builds a
+# libpq conninfo from these; with connect => false it is validated but never
+# used to connect, so the host/port need not be reachable.
+my $srv_opts = "OPTIONS (host 'localhost', port '1', dbname 'nx')";
+
+#############################################################################
+# Finding 1: pg_dump/pg_restore (and hence pg_upgrade) of a SERVER-based
+# subscription fails, because CreateSubscription() resolves the user mapping
+# for the role *executing the restore* (GetUserId()), not the subscription's
+# eventual owner -- even with connect => false.
+#
+# The dump is emitted "CREATE SUBSCRIPTION ... SERVER ... WITH (connect =
+# false ...)" followed later by "ALTER SUBSCRIPTION ... OWNER TO owner1", so
+# the CREATE runs as the restoring superuser. If that superuser has no user
+# mapping (and there is no PUBLIC mapping) on the server, the restore of a
+# perfectly valid dump fails.
+#
+# CORRECT BEHAVIOR: the dump should restore cleanly (a connect=false restore
+# should not require the restoring role to have a user mapping on the
+# server).
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect1_src');
+ $node->safe_psql(
+ 'defect1_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ -- Only owner1 has a mapping; the restoring superuser will not.
+ CREATE USER MAPPING FOR owner1 SERVER s OPTIONS (user 'repl', password 'secret');
+ GRANT USAGE ON FOREIGN SERVER s TO owner1;
+ GRANT CREATE ON DATABASE defect1_src TO owner1;
+ });
+ $node->safe_psql(
+ 'defect1_src', q{
+ SET SESSION AUTHORIZATION owner1;
+ CREATE SUBSCRIPTION defect1_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ my $dump = "$tempdir/defect1.sql";
+ command_ok(
+ [ 'pg_dump', '-f', $dump, $node->connstr('defect1_src') ],
+ 'finding 1: pg_dump of a SERVER-based subscription succeeds');
+
+ $node->safe_psql('postgres', 'CREATE DATABASE defect1_dst');
+
+ # BUG: restoring as the superuser (which has no mapping) fails at the
+ # CREATE SUBSCRIPTION step. When fixed, this should become command_ok().
+ command_fails_like(
+ [
+ 'psql', '--no-psqlrc', '-v', 'ON_ERROR_STOP=1',
+ '-f', $dump, '-d', $node->connstr('defect1_dst')
+ ],
+ qr/user mapping not found for user "\Q$super\E", server "s"/,
+ 'finding 1: restore fails - CREATE resolves the restorer\'s mapping, not the owner\'s'
+ );
+
+ # The subscription was therefore not restored into defect1_dst at all.
+ my $got = $node->safe_psql(
+ 'defect1_dst', q{
+ SELECT count(*) FROM pg_subscription
+ WHERE subname = 'defect1_sub'
+ AND subdbid = (SELECT oid FROM pg_database WHERE datname = 'defect1_dst')
+ });
+ is($got, '0', 'finding 1: subscription is missing after the failed restore');
+}
+
+#############################################################################
+# Finding 2: restoring a SERVER-based subscription whose owner derives its
+# foreign-server USAGE from a GRANT (rather than ownership) leaves the
+# subscription owned by the wrong role. AlterSubscriptionOwner_internal()
+# requires the new owner to hold USAGE on the server, but pg_dump emits
+# "ALTER SUBSCRIPTION ... OWNER TO owner2" in the main restore pass while
+# "GRANT USAGE ON FOREIGN SERVER" is deferred to the later ACL pass -- so at
+# the moment of the OWNER TO, owner2 does not yet have USAGE.
+#
+# CORRECT BEHAVIOR: the OWNER TO should succeed during restore and the
+# subscription should end up owned by owner2.
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect2_src');
+ $node->safe_psql(
+ 'defect2_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ -- PUBLIC mapping, so finding 1 does not mask this one: the
+ -- restoring superuser can resolve a connection.
+ CREATE USER MAPPING FOR PUBLIC SERVER s OPTIONS (user 'repl', password 'secret');
+ -- owner2's USAGE comes from a GRANT, not from owning the server.
+ GRANT USAGE ON FOREIGN SERVER s TO owner2;
+ GRANT CREATE ON DATABASE defect2_src TO owner2;
+ });
+ $node->safe_psql(
+ 'defect2_src', q{
+ SET SESSION AUTHORIZATION owner2;
+ CREATE SUBSCRIPTION defect2_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ my $dump = "$tempdir/defect2.sql";
+ command_ok(
+ [ 'pg_dump', '-f', $dump, $node->connstr('defect2_src') ],
+ 'finding 2: pg_dump succeeds');
+
+ $node->safe_psql('postgres', 'CREATE DATABASE defect2_dst');
+
+ # BUG: the CREATE SUBSCRIPTION succeeds (PUBLIC mapping), but the
+ # subsequent ALTER SUBSCRIPTION ... OWNER TO owner2 fails because the
+ # GRANT USAGE has not been restored yet. When fixed: command_ok().
+ command_fails_like(
+ [
+ 'psql', '--no-psqlrc', '-v', 'ON_ERROR_STOP=1',
+ '-f', $dump, '-d', $node->connstr('defect2_dst')
+ ],
+ qr/new subscription owner "owner2" does not have permission on foreign server "s"/,
+ 'finding 2: restore fails at OWNER TO because GRANT USAGE is in a later pass'
+ );
+
+ # BUG: the subscription is left owned by the restoring superuser instead
+ # of owner2. When fixed, the expected value is 'owner2'.
+ my $owner = $node->safe_psql(
+ 'defect2_dst', q{
+ SELECT subowner::regrole FROM pg_subscription
+ WHERE subname = 'defect2_sub'
+ AND subdbid = (SELECT oid FROM pg_database WHERE datname = 'defect2_dst')
+ });
+ is($owner, $super,
+ 'finding 2: subscription is left owned by the restoring superuser, not owner2'
+ );
+}
+
+#############################################################################
+# Finding 4: REASSIGN OWNED BY, run from any database other than the one
+# holding the subscription's foreign server, fails with an internal error.
+# pg_subscription is a shared catalog, so shdepReassignOwned() processes the
+# subscription from every database; AlterSubscriptionOwner_internal() then
+# looks the server up in the *current* database's (per-database)
+# pg_foreign_server and raises "cache lookup failed for foreign server".
+# This breaks the documented multi-database role-removal workflow (REASSIGN
+# OWNED / DROP OWNED in each database, then DROP ROLE).
+#
+# CORRECT BEHAVIOR: REASSIGN OWNED from another database should reassign the
+# subscription without an internal error (either succeeding, or failing with
+# a clean, user-facing permission error like the in-database case below).
+#############################################################################
+{
+ $node->safe_psql('postgres', 'CREATE DATABASE defect4_src');
+ $node->safe_psql(
+ 'defect4_src', qq{
+ CREATE EXTENSION postgres_fdw;
+ CREATE SERVER s FOREIGN DATA WRAPPER postgres_fdw $srv_opts;
+ CREATE USER MAPPING FOR owner4 SERVER s OPTIONS (user 'repl', password 'secret');
+ GRANT USAGE ON FOREIGN SERVER s TO owner4;
+ GRANT CREATE ON DATABASE defect4_src TO owner4;
+ });
+ $node->safe_psql(
+ 'defect4_src', q{
+ SET SESSION AUTHORIZATION owner4;
+ CREATE SUBSCRIPTION defect4_sub SERVER s PUBLICATION p
+ WITH (connect = false, slot_name = NONE);
+ });
+
+ # BUG: from the "postgres" database (which has no such foreign server),
+ # REASSIGN OWNED aborts with an internal (XX000) cache-lookup error.
+ my ($ret, $out, $err) =
+ $node->psql('postgres', 'REASSIGN OWNED BY owner4 TO bob');
+ isnt($ret, 0, 'finding 4: cross-database REASSIGN OWNED fails');
+ like(
+ $err,
+ qr/cache lookup failed for foreign server \d+/,
+ 'finding 4: cross-database REASSIGN OWNED raises an internal cache-lookup error'
+ );
+
+ # Contrast: run from the subscription's own database, the same command
+ # fails with a clean, user-facing permission error (bob lacks USAGE on
+ # the server) -- showing the cross-database internal error is the defect,
+ # not the reassignment being disallowed.
+ ($ret, $out, $err) =
+ $node->psql('defect4_src', 'REASSIGN OWNED BY owner4 TO bob');
+ isnt($ret, 0, 'finding 4 (contrast): in-database REASSIGN OWNED also fails here');
+ like(
+ $err,
+ qr/new subscription owner "bob" does not have permission on foreign server "s"/,
+ 'finding 4 (contrast): in-database REASSIGN OWNED gives a clean permission error'
+ );
+}
+
+done_testing();
[text/plain] 8185bb5-subscription-server.md (23.5K, ../../20260710195902.4f.noahmisch@microsoft.com/3-8185bb5-subscription-server.md)
download | inline:
# User-visible defects in commit 8185bb5 still present in master
**Commit:** 8185bb5 "CREATE SUBSCRIPTION ... SERVER." (Jeff Davis, 2026-03-06)
**Master at time of audit:** 3c9a38b
**Method:** 30-agent workflow — 16 area-focused finders (subscription DDL, foreign.c resolution, FDW DDL, dependencies, workers, postgres_fdw, extension packaging, pg_dump, psql, parser/nodes, security, NULL-sweep, docs, concurrency, end-to-end semantics) produced 40 raw findings; deduplicated to 19; the top 13 each adversarially verified by an independent skeptic instructed to refute, checking three gates: still in master, attributable to 8185bb5 (not pre-existing), and user-visible. All 13 were **confirmed** with code citations; the remaining 6 lower-priority findings are listed unverified.
Line numbers refer to current master.
---
## Live-cluster verification (findings 1, 2, 4, 6)
Reproduced on a running PostgreSQL 20devel cluster built from this tree (meson, `build/tmp_install`), using the regression suite's `test_fdw_connection` (returns a static conninfo, so no publisher is needed) and `connect=false` subscriptions.
| # | Static verdict | Live result | Observed |
|---|---|---|---|
| 1 | confirmed | **CONFIRMED** | Restore as postgres fails at `CREATE SUBSCRIPTION`: `ERROR 42704: user mapping not found for user "postgres", server "test_server"` at `GetUserMapping, foreign.c:255`. Control: identical CREATE run as the mapped owner (alice) succeeds. |
| 2 | confirmed | **CONFIRMED** | Restore's `CREATE` succeeds (PUBLIC mapping), then `ALTER SUBSCRIPTION ... OWNER TO alice` fails: `ERROR 42501: new subscription owner "alice" does not have permission on foreign server "test_server"` at `AlterSubscriptionOwner_internal, subscriptioncmds.c:2712`; subscription left owned by postgres. Control: `GRANT USAGE` first → OWNER TO succeeds. |
| 4 | confirmed | **CONFIRMED** | `REASSIGN OWNED BY alice TO bob` from the `postgres` database: `ERROR XX000: cache lookup failed for foreign server 16393`. Contrast: same command from the subscription's own database gives a clean permission error. |
| 6 | confirmed | **REFUTED** | Concurrent `DROP SERVER` does **not** race in — it blocks on `AccessExclusiveLock on object ... of class 1417` (server) and times out; no dangling reference results. See the rewritten entry below. |
**Bottom line:** three of the four requested findings reproduce exactly as described; **Finding 6 does not reproduce and is withdrawn.**
---
## Confirmed — major
### 1. pg_dump/pg_restore/pg_upgrade of SERVER subscriptions fail: CREATE resolves the connection for the creating user, not the eventual owner, even with connect=false — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:784`
**Symptom:** Restoring a dump of a database containing a server-based subscription fails with `ERROR: user mapping not found for user "postgres", server "..."` whenever the restoring role has no user mapping (and no PUBLIC mapping) on that server — even though the dump emits `connect = false`. pg_restore errors out; pg_upgrade aborts mid-restore on a perfectly valid source cluster.
**Repro:** As superuser: `CREATE SERVER s ...; CREATE USER MAPPING FOR alice SERVER s ...; GRANT USAGE ON FOREIGN SERVER s TO alice;` As alice: `CREATE SUBSCRIPTION sub SERVER s PUBLICATION pub WITH (connect=false, slot_name=NONE);` Then `pg_dump db | psql newdb` as postgres, or pg_upgrade the cluster: the emitted CREATE SUBSCRIPTION fails.
**Mechanism:** `CreateSubscription()` sets `owner = GetUserId()` (line 654) and in the servername branch (771–798) unconditionally — regardless of `connect=false` — calls `GetUserMapping(owner, serverid)` (line 784), which errors with no superuser bypass (foreign.c:251–258), then `ForeignServerConnectionString()` and `walrcv_check_conninfo()`. `dumpSubscription()` emits `CREATE SUBSCRIPTION ... SERVER ... WITH (connect = false, ...)` executed as the restore role; ownership is applied only afterwards via `ALTER SUBSCRIPTION ... OWNER TO`. pg_upgrade runs `pg_restore --exit-on-error` and aborts; binary-upgrade mode additionally emits `ALTER SUBSCRIPTION ... ENABLE`, hitting the same path via `GetSubscription(conninfo_needed=true)`. The docs' recommended setup (mapping only FOR the subscribing user) triggers this; pg_dump.sgml:1735–1748 promises subscription dumps restore without remote access. Follow-ups e5c4058/702e9df fixed only ALTER/DROP paths.
**Verifier notes:** Confirmed on all gates. Restore succeeds only if the restoring role happens to have a role-specific or PUBLIC mapping, or with `--use-set-session-authorization`; pg_upgrade uses neither escape. pg_dump TAP tests cover only CONNECTION-based subscriptions.
### 2. Restore ordering: ALTER SUBSCRIPTION ... OWNER TO fails because GRANT USAGE ON FOREIGN SERVER restores later, in the ACL pass — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:2710`
**Symptom:** Restore errors at the emitted `ALTER SUBSCRIPTION ... OWNER TO`: `new subscription owner "alice" does not have permission on foreign server ...`, because the GRANT has not been restored yet. Under psql the subscription silently stays owned by the bootstrap role (wrong catalog state); `pg_restore --exit-on-error` and pg_upgrade abort.
**Repro:** Grant-based (non-owner) subscription owner; dump; restore into a fresh database as superuser: CREATE SUBSCRIPTION succeeds, the following `ALTER SUBSCRIPTION sub OWNER TO alice` errors.
**Mechanism:** `AlterSubscriptionOwner_internal()` (2706–2720, added by 8185bb5) requires `object_aclcheck(ForeignServerRelationId, ..., ACL_USAGE)` plus `GetUserMapping(newOwnerId, ...)` before changing owner. pg_restore emits `ALTER ... OWNER TO` inside the subscription's own TOC entry in RESTORE_PASS_MAIN (`_printTocEntry`; SUBSCRIPTION is in `_getObjectDescription`, pg_backup_archiver.c:3862), while `GRANT USAGE ON FOREIGN SERVER` is an ACL TOC entry deferred by `_tocEntryRestorePass` (3366–3372) to RESTORE_PASS_ACL, strictly after all main-pass items even in serial restore. Plain-script dumps have the same order. Only new owners who own the server or are superuser restore cleanly. CONNECTION-based subscriptions have no such check — a new regression.
**Verifier notes:** Confirmed. Current master reads `GetForeignServer(form->subserver)` after the 11f8018 refactor; behavior identical.
### 3. postgres_fdw connection function embeds session-only credentials (SCRAM passthrough / GSS delegation), so CREATE succeeds but apply workers can never authenticate
**Where:** `contrib/postgres_fdw/connection.c:580`
**Symptom:** `CREATE SUBSCRIPTION ... SERVER` over a `use_scram_passthrough` server succeeds (connects, creates the remote slot) when the creator logged in via SCRAM — but apply/tablesync workers can never connect to a SCRAM-authenticated publisher. The subscription never replicates; the log fills with connection errors as the launcher restarts the worker. Non-superuser owners instead get a misleading `password is required` at CREATE. Undocumented.
**Mechanism:** `construct_connection_params()` (580–614) appends `scram_client_key`/`scram_server_key`/`require_auth` only when `MyProcPort != NULL && MyProcPort->has_scram_keys`; `MyProcPort` is assigned solely in backend_startup.c:177, so it is NULL in logical-replication background workers. The DDL session has keys, so CreateSubscription's `walrcv_check_conninfo`/`walrcv_connect` succeed; workers rebuild the conninfo via `GetSubscription → ForeignServerConnectionString → postgres_fdw_connection` (worker.c:5826) with no keys and no password. Superuser-owned subscriptions loop on publisher auth failure; non-superuser owners abort in `check_conn_params()`/`libpqrcv_check_conninfo`. GSSAPI delegated credentials have the same session-vs-worker asymmetry (`be_gssapi_get_delegation(MyProcPort)`, connection.c:769). postgres-fdw.sgml's Subscription Management section claims option parity with no caveat. No DDL-time rejection or warning exists.
**Verifier notes:** Confirmed. The MyProcPort-conditional SCRAM code pre-existed but was only reachable from client backends; 8185bb5 exposed it to bgworkers via `postgres_fdw_connection`, so the omission is attributable.
### 4. REASSIGN OWNED BY fails with XX000 "cache lookup failed for foreign server" when the role owns a server-based subscription in another database — LIVE-VERIFIED ✓
**Where:** `src/backend/commands/subscriptioncmds.c:2708`
**Symptom:** `REASSIGN OWNED BY old TO new`, run in any database other than the subscription's, aborts with the internal error `cache lookup failed for foreign server NNN` (XX000). The documented multi-database role-removal workflow (REASSIGN OWNED in each database, then DROP ROLE) breaks.
**Mechanism:** pg_subscription is a shared catalog, so its pg_shdepend row has `dbid = InvalidOid`, and `shdepReassignOwned` processes it from every database, calling `AlterSubscriptionOwner_internal`. 8185bb5 added a lookup of the subscription's foreign server there — but pg_foreign_server is per-database, so from any other database `GetForeignServerExtended` raises `elog(ERROR, "cache lookup failed for foreign server %u")`. Pre-commit code had no server lookup and succeeded. In the freak case of an OID collision, the ACL/user-mapping checks would be evaluated against an unrelated server of the current database.
**Verifier notes:** Confirmed. The literal `GetForeignServer()` call shape is from refactor 11f8018, but 8185bb5's original `object_aclcheck` + `GetUserMapping` fails identically cross-database (same XX000 for non-superusers via `object_aclmask_ext`; superusers fail in `GetUserMapping`). Broken since 8185bb5 either way.
### 5. Worker-side conninfo resolution errors preempt clean disabled-subscription handling and can permanently disable a live SERVER subscription via disable_on_error
**Where:** `src/backend/replication/logical/worker.c:5077`
**Symptom:** Disabling a SERVER subscription and dropping its user mapping in one transaction makes the running worker exit with `ERROR: user mapping not found ...` plus a bumped `apply_error_count` and (with `disable_on_error`) a misleading "disabled because of an error" LOG, instead of the clean "will stop because the subscription was disabled". Rotating a live mapping via DROP+CREATE (separate commits) can permanently disable the subscription if the worker rereads in the gap.
**Mechanism:** `maybe_reread_subscription()` (worker.c:5077) calls `GetSubscription(subid, true, true, true)`, which runs `object_aclcheck` and `ForeignServerConnectionString → postgres_fdw_connection → GetUserMapping` — erroring before the `newsub->enabled` clean-exit check at worker.c:5101. The error unwinds to `start_apply`'s PG_CATCH (5644–5673): with `disableonerr` → `DisableSubscriptionAndExit` (apply_error_count++, misleading LOG); else pgstat error + rethrow. Pre-8185bb5, `GetSubscription` could not fail for an existing subscription. Follow-ups e5c4058/702e9df fixed this class only in ALTER/DROP DDL paths.
**Verifier notes:** Confirmed; all cited lines match master. (One label fix: the third `maybe_reread_subscription` caller is worker.c:740, stream handling.)
### 6. ~~CREATE/ALTER SUBSCRIPTION ... SERVER takes no lock on the foreign server, so a concurrent DROP SERVER leaves a dangling subserver OID~~ — REFUTED on a live cluster
**Original claim:** `CreateSubscription`/`AlterSubscription` look up the server via `GetForeignServerByName()` (a syscache read) and record the dependency without locking the server, so a concurrent `DROP SERVER` — whose `findDependentObjects` MVCC scan can't see the uncommitted pg_depend row — could succeed and leave `pg_subscription.subserver` dangling (worker restart loop, DROP SUBSCRIPTION failure, pg_dump NULL-deref crash).
**Why it's wrong:** `recordDependencyOn` → `recordMultipleDependencies` calls `dependencyLockAndCheckObject()` (src/backend/catalog/pg_depend.c:121) **before** inserting the pg_depend row. That function takes an `AccessShareLock` on the referenced server via `LockDatabaseObject()` and then rechecks the object still exists, with the express purpose (per its header comment) to "make sure that we don't record a bogus reference permanently in the catalogs." The lock is held to end of transaction and conflicts with the `AccessExclusiveLock` that `DROP SERVER` acquires. This is a generic backstop in the dependency layer that both the finder and its adversarial verifier overlooked — they reasoned only about MVCC row visibility.
**Live test (PostgreSQL 20devel from this tree):**
- With `CREATE SUBSCRIPTION s6 SERVER test_server ... (connect=false)` held in an open transaction, a concurrent `DROP SERVER test_server` **blocked** and hit `lock_timeout`: `ERROR: canceling statement due to lock timeout / CONTEXT: waiting for AccessExclusiveLock on object 16427 of class 1417`. After commit, `s6.subserver` still resolved to the live server — no dangling reference.
- `pg_locks` during the open transaction shows the CREATE holding `AccessShareLock` on `pg_foreign_server` objid 16427; the `ALTER SUBSCRIPTION ... SERVER` path holds the identical lock.
- Once the subscription is committed, `DROP SERVER` (and `DROP SERVER CASCADE`) fail cleanly with `cannot drop server test_server because subscription s6 depends on it`; `pg_dump` of the database succeeds.
- The pre-dependency window (server lookup → `recordDependencyOn`) is also safe: if a `DROP SERVER` commits there, `dependencyLockAndCheckObject`'s existence recheck aborts the CREATE with an error rather than persisting a dangling OID.
No supported sequence produces a dangling `subserver`, so the downstream symptoms (worker restart loop, DROP failure, pg_dump crash) are unreachable. Finding withdrawn.
### 7. ALTER SUBSCRIPTION commands that connect to the publisher skip the foreign-server USAGE privilege check
**Where:** `src/backend/commands/subscriptioncmds.c:1571`
**Symptom:** A subscription owner whose USAGE on the foreign server was revoked can still make the server connect to the remote host with the user mapping's credentials via ALTER SUBSCRIPTION (REFRESH PUBLICATION/SEQUENCES, SET/ADD/DROP PUBLICATION with refresh, SET (failover/two_phase), retain_dead_tuples checks) — including dropping remote tablesync slots and altering remote slots — while the apply worker, CREATE, DROP, and OWNER TO all enforce USAGE. REVOKE appears ineffective to the DBA.
**Mechanism:** `AlterSubscription` calls `GetSubscription(subid, false, orig_conninfo_needed, false)` — `conninfo_aclcheck=false` — so the conninfo is built via `ForeignServerConnectionString` without `object_aclcheck`. That conninfo feeds foreground `walrcv_connect` in `AlterSubscription_refresh` (line 1060, incl. `ReplicationSlotDropAtPubNode`), `AlterSubscription_refresh_seq` (1312), and the failover/two_phase/check_pub_rdt block (2223). The justifying comment says the ACL check "will be done by the subscription worker", but these connections happen in the ALTER command itself. CREATE (779), ALTER..SERVER (1929), OWNER TO (2710), `construct_subserver_conninfo` (2281), and worker.c (5077/5826) all enforce USAGE — the ALTER paths are the inconsistent gap.
**Verifier notes:** Confirmed. Not an escalation (the owner uses their own mapping on their own subscription); the impact is that REVOKE USAGE fails to stop owner-initiated publisher connections.
---
## Confirmed — minor
### 8. \dew / \dew+ do not display the FDW connection function
**Where:** `src/bin/psql/describe.c:6188`
`listForeignDataWrappers()` shows Handler and Validator but never fdwconnection, in either mode; `git grep fdwconnection src/bin/psql` is empty. A DBA cannot see whether an FDW supports `CREATE SUBSCRIPTION ... SERVER` without querying the catalog directly. The commit updated \dRs+ (Server column) but missed \dew; sibling omissions from the same commit (pg_dump b71bf3b, tab completion 5fa7837, catalogs.sgml 90630ec "missed in commit 8185bb5347") were all treated as bugs and fixed.
### 9. DROP SUBSCRIPTION of a server-based subscription fails outright when the conninfo cannot be constructed and any relation is not READY, even with slot_name = NONE
**Where:** `src/backend/commands/subscriptioncmds.c:2522`
With non-READY pg_subscription_rel rows, DropSubscription calls `construct_subserver_conninfo()`, which traps only ACL failures into `*err`; `ForeignServerConnectionString()` failures (e.g. dropped user mapping) still ereport — a bare `user mapping not found` with no drop context and no HINT, aborting before both tolerant paths (clean return for `wrconn == NULL && !slotname`, and the hinted `ReportSlotConnectionError`). Conninfo-based subscriptions in the same state drop cleanly. Follow-up 702e9df fixed only the `rstates == NIL` case and its commit message acknowledges this residual case. The verifier notes this is a knowingly accepted limitation per the code comment — but it remains user-visible, hint-less, and untested (the regression suite covers only the slot-set variant, subscription.out:218).
### 10. DROP SERVER ... CASCADE cannot drop a dependent subscription, contradicting drop_server.sgml, with no hint and no doc of the restriction
**Where:** `doc/src/sgml/ref/drop_server.sgml:65` / `src/backend/catalog/dependency.c:907`
`findDependentObjects` unconditionally errors for any dependent in a shared catalog before the deptype/CASCADE handling is consulted, so `DROP SERVER s CASCADE` fails with `cannot drop server s because subscription sub depends on it` — while drop_server.sgml's CASCADE text promises automatic dropping. Blocking the drop is intentional (doDeletion cannot drop subscriptions or orphan the remote slot), but the restriction is documented nowhere, the error has no hint, and no test covers it. DROP FDW CASCADE and DROP OWNED hit it transitively.
### 11. create_foreign_data_wrapper.sgml omits the connection function's required text return type
**Where:** `doc/src/sgml/ref/create_foreign_data_wrapper.sgml:113`
The CONNECTION entry documents the three argument types only; `lookup_fdw_connection_func()` (foreigncmds.c:542–549) rejects any return type other than text. An author following the docs (`RETURNS varchar`) gets `function my_conn must return type text` on valid-per-docs input. Adjacent HANDLER/VALIDATOR entries do state their return-type contracts; nothing in the docs states the result is used as a libpq conninfo.
### 12. ALTER SUBSCRIPTION forms that never use a connection fail on server-based subscriptions when the conninfo cannot be constructed
**Where:** `src/backend/commands/subscriptioncmds.c:1557`
Purely local operations — `SKIP (lsn ...)`, `SET (disable_on_error/binary/origin/synchronous_commit ...)`, SET/ADD/DROP PUBLICATION `WITH (refresh = false)` — raise `user mapping not found` on a server-based subscription (even disabled), because `orig_conninfo_needed` stays true for every kind except the four e5c4058 exemptions (DISABLE, SERVER, CONNECTION, lone SET (slot_name=NONE)). The same commands never validate conninfo for CONNECTION-based subscriptions. Verifier note: a blanket exemption is impossible for OPTIONS/PUBLICATION kinds (refresh=true, failover, two_phase, retain_dead_tuples genuinely need conninfo) — the gap is the lack of per-option conditioning. Repro requires the mapping to exist at CREATE and be dropped after, with no PUBLIC fallback.
### 13. CREATE SUBSCRIPTION ... SERVER checks user-mapping existence before FDW subscription capability
**Where:** `src/backend/commands/subscriptioncmds.c:784` (comment at 783)
Pointing CREATE SUBSCRIPTION at a server of an FDW without a connection function (e.g. file_fdw) first reports `user mapping not found`; after the user creates a useless mapping, the second attempt reveals the real, unfixable `foreign data wrapper "file_fdw" does not support subscription connections`. The capability check lives inside `ForeignServerConnectionString` (foreign.c:209–214), unreachable until a mapping exists, though it needs no mapping and could run first. Same ordering in ALTER ... SERVER (1940/1942).
---
## Unverified (found, ranked below the 13-verifier cap; not adversarially checked)
### 14. User-mapping passwords logged verbatim at DEBUG1 on every worker start (minor/medium)
`SetupApplyOrSyncWorker()` (worker.c:5988–5990) logs `connecting to publisher using connection string "%s"` with `MySubscription->conninfo`. The elog pre-dates the commit, but 8185bb5 newly routes pg_user_mapping secrets (password) into that string via `postgres_fdw_connection`. Previously the logged string only duplicated pg_subscription.subconninfo, and user-mapping options were never emitted anywhere; there is no redaction (cf. `libpqrcv_get_conninfo`'s password stripping). Anyone with log access sees the password.
### 15. findDependentObjects rejects shared-catalog dependents before lock-and-recheck: DROP SERVER spuriously fails while a concurrent DROP SUBSCRIPTION is committing (minor/medium)
The dependency.c:907 shared-catalog error runs before `AcquireDeletionLock()`/`systable_recheck_tuple()`, so DROP SERVER errors instead of waiting out a concurrent DROP SUBSCRIPTION that has already deleted its pg_depend row but is still dropping the remote slot before commit. Any other dependent class would block and then succeed.
### 16. fdwhandler.sgml never mentions connection functions (minor/medium)
The FDW-author chapter still says an author implements "a handler function, and optionally a validator function"; the new third SQL-registered support function — its contract (user oid, server oid, internal → text libpq conninfo) — is absent from the chapter. The partial signature in create_foreign_data_wrapper.sgml is the only author-facing documentation.
### 17. alter_subscription.sgml documents no requirements for ALTER SUBSCRIPTION ... SERVER and none of the new OWNER TO failure conditions (minor/medium)
ALTER ... SERVER requires the subscription owner (not the invoker) to have USAGE and a user mapping on the new server, and its FDW to have a connection function; OWNER TO requires the same of the new owner. None of these appear on the ALTER SUBSCRIPTION page, while the equivalent CREATE-time requirements are documented.
### 18. Server's application_name option overrides the subscription name on the publisher (minor/low)
`construct_connection_params()` serializes a server-level `application_name` into the conninfo; `libpqrcv_connect` passes the subscription name only as `fallback_application_name`, which loses. pg_stat_replication shows the shared FDW name and `synchronous_standby_names` matching by subscription name silently fails. CONNECTION-based subscriptions don't auto-inject this.
### 19. Lost-invalidation window at worker startup can leave a worker on stale server-derived conninfo indefinitely (minor/low)
`InitializeLogRepWorker()` resolves conninfo via `GetSubscription` (worker.c:5826) before registering the FOREIGNSERVEROID/USERMAPPINGOID/FOREIGNDATAWRAPPEROID invalidation callbacks (5896–5916); an ALTER SERVER/USER MAPPING committing in that gap is lost, and no lock serializes server/mapping DDL against worker startup (unlike subscription DDL). Sub-millisecond window; low confidence.
---
## Coverage notes
- Areas swept that produced no surviving findings: grammar/keyword regressions, node read/write/copy/equal support, extension upgrade-path state divergence (1.2→1.3 vs fresh install), tab completion (fixed post-commit by 5fa7837), catalogs.sgml (fixed post-commit by 90630ec), pg_subscription view masking, connection-string quoting/injection in `appendEscapedValue`, direct SQL calls of the connection function (nominal three-arg signature includes `internal`, blocking SQL calls).
- Already-fixed follow-ups were excluded by instruction and by per-file `git log 8185bb5..master` checks in every finder and verifier.
- Verification was static analysis + git archaeology only; no repros were executed against a running cluster.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-19 22:32 Jeff Davis <pgsql@j-davis.com>
parent: Noah Misch <noah@leadboat.com>
2 siblings, 2 replies; 27+ messages in thread
From: Jeff Davis @ 2026-07-19 22:32 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Amit Kapila <amit.kapila16@gmail.com>
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/20260710192533.4f.noahmisch@microsoft.com).
>
> 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
Attachments:
[text/x-patch] v1-0001-Fix-dump-restore-of-server-based-subscriptions.patch (12.1K, ../../35e50b0f80c850e5b9eb1dedcc0844852a2d2de0.camel@j-davis.com/2-v1-0001-Fix-dump-restore-of-server-based-subscriptions.patch)
download | inline diff:
From 648b90412b801708b15dcb23ce63757451733072 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <noah@leadboat.com>
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-28 16:10 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 2 replies; 27+ messages in thread
From: Jeff Davis @ 2026-07-28 16:10 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Amit Kapila <amit.kapila16@gmail.com>
On Sun, 2026-07-19 at 15:32 -0700, Jeff Davis wrote:
> 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.
There are two other threads discussing closely-related problems:
https://www.postgresql.org/message-id/CAHGQGwFGa6+wWVgUmZPFwN=fBY59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail...
https://www.postgresql.org/message-id/OS9PR01MB12149C3ED34272966B25DB173F5C12@OS9PR01MB12149.jpnprd0...
I'd like to step back and discuss where the complexity comes from:
* During restore, we simply want it to recreate the right catalog
state, and it uses multiple commands to do so (CREATE SUBSCRIPTION,
ALTER SUBSCRIPTION OWNER TO, etc.). It should never connect to the
publisher, and validation is mostly counterproductive for the
intermediate states.
* When we no longer need a slot (due to ALTER/DROP), we would like to
drop it from the publisher, but for various reasons a connection to the
publisher may be impossible. In that case, the user may still want the
ALTER/DROP to succeed.
* Validation at DDL-time is useful for interactive purposes, but
limited. Whatever is validated may change before connection time (e.g.
privileges on the server may be revoked), so connection-time validation
is the authoritative one.
To reconcile these goals, we need to weaken DDL-time validation a bit,
be more precise about when we try to generate a conninfo, and then be
sure that restore doesn't do anything that would cause a conninfo to be
generated or a connection to happen.
We can preserve the most useful kinds of validation by putting it
behind an if (!superuser()) guard, so it doesn't interfere with
restores.
Thoughts?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-28 22:24 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 27+ messages in thread
From: Jeff Davis @ 2026-07-28 22:24 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Amit Kapila <amit.kapila16@gmail.com>
On Sun, 2026-07-19 at 15:32 -0700, Jeff Davis wrote:
> 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.
Amit, can you please look into the check_pub_rdt issue? I think the
right fix is to just check it when the worker connects, not at DDL
time.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-28 22:36 Jeff Davis <pgsql@j-davis.com>
parent: Noah Misch <noah@leadboat.com>
2 siblings, 0 replies; 27+ messages in thread
From: Jeff Davis @ 2026-07-28 22:36 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Robert Haas <robertmhaas@gmail.com>
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/20260710192533.4f.noahmisch@microsoft.com).
>
> 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.
Partial patch series:
0001: Finding 10 preexisting issue: Add missing lock release for
DROP OWNED BY (backport to 16)
0002: Finding 10 & 15: Improve & document DROP SERVER CASCADE
0003: Finding 3: Reject use_scram_passthrough for
subscription connections.
Robert, can you take a look at 0001, which fixes an issue introduced in
6566133c5f? I don't think it's major but it can retain the lock for
longer.
Regards,
Jeff Davis
Attachments:
[text/x-patch] v2-0001-Fix-lock-release-for-role-membership-grants-in-DR.patch (1.1K, ../../2487ddcd737d4fc8e408e87aa9ad4365eed3bbb3.camel@j-davis.com/2-v2-0001-Fix-lock-release-for-role-membership-grants-in-DR.patch)
download | inline diff:
From 428d8c7c41806deeb1357a6e95e8f9a2687dbd56 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 14:50:45 -0700
Subject: [PATCH v2 1/3] Fix lock release for role membership grants in DROP
OWNED BY.
Add ReleaseDeletionLock() to match AcquireDeletionLock(). Introduced
by commit 6566133c5f5.
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 16
---
src/backend/catalog/dependency.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c54774b3275..52cd2caf9d4 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1600,6 +1600,9 @@ ReleaseDeletionLock(const ObjectAddress *object)
{
if (object->classId == RelationRelationId)
UnlockRelationOid(object->objectId, AccessExclusiveLock);
+ else if (object->classId == AuthMemRelationId)
+ UnlockSharedObject(object->classId, object->objectId, 0,
+ AccessExclusiveLock);
else
/* assume we should lock the whole object not a sub-object */
UnlockDatabaseObject(object->classId, object->objectId, 0,
--
2.43.0
[text/x-patch] v2-0002-Improve-DROP-SERVER-handling-of-dependent-subscri.patch (5.5K, ../../2487ddcd737d4fc8e408e87aa9ad4365eed3bbb3.camel@j-davis.com/3-v2-0002-Improve-DROP-SERVER-handling-of-dependent-subscri.patch)
download | inline diff:
From f40a6c256c709014b1fbe7670f68061800d504ce Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 14:48:12 -0700
Subject: [PATCH v2 2/3] Improve DROP SERVER handling of dependent
subscriptions.
Acquire a lock on the subscription to avoid unnecessary errors. Also
issue a HINT and document the restriction that CASCADE won't cascade
to the subscription object.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 19
---
doc/src/sgml/ref/drop_server.sgml | 4 ++++
src/backend/catalog/dependency.c | 28 ++++++++++++----------
src/test/regress/expected/subscription.out | 4 ++++
src/test/regress/sql/subscription.sql | 2 ++
4 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/drop_server.sgml b/doc/src/sgml/ref/drop_server.sgml
index f83a661b3eb..5fa0b763f36 100644
--- a/doc/src/sgml/ref/drop_server.sgml
+++ b/doc/src/sgml/ref/drop_server.sgml
@@ -66,6 +66,10 @@ DROP SERVER [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [, .
user mappings),
and in turn all objects that depend on those objects
(see <xref linkend="ddl-depend"/>).
+ However, a subscription that uses the server is never dropped
+ automatically; it must be dropped with
+ <link linkend="sql-dropsubscription"><command>DROP SUBSCRIPTION</command></link>
+ before the server can be dropped.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 52cd2caf9d4..b80949a5eeb 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -900,17 +900,6 @@ findDependentObjects(const ObjectAddress *object,
object->objectSubId == 0)
continue;
- /*
- * Check that the dependent object is not in a shared catalog, which
- * is not supported by doDeletion().
- */
- if (IsSharedRelation(otherObject.classId))
- ereport(ERROR,
- (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
- errmsg("cannot drop %s because %s depends on it",
- getObjectDescription(object, false),
- getObjectDescription(&otherObject, false))));
-
/*
* Must lock the dependent object before recursing to it.
*/
@@ -931,6 +920,19 @@ findDependentObjects(const ObjectAddress *object,
continue;
}
+ /*
+ * Check that the dependent object is not in a shared catalog, which
+ * is not supported by doDeletion().
+ */
+ if (IsSharedRelation(otherObject.classId))
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s depends on it",
+ getObjectDescription(object, false),
+ getObjectDescription(&otherObject, false)),
+ errhint("Drop %s first.",
+ getObjectDescription(&otherObject, false))));
+
/*
* We do need to delete it, so identify objflags to be passed down,
* which depend on the dependency type.
@@ -1579,7 +1581,7 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
else
LockRelationOid(object->objectId, AccessExclusiveLock);
}
- else if (object->classId == AuthMemRelationId)
+ else if (IsSharedRelation(object->classId))
LockSharedObject(object->classId, object->objectId, 0,
AccessExclusiveLock);
else
@@ -1600,7 +1602,7 @@ ReleaseDeletionLock(const ObjectAddress *object)
{
if (object->classId == RelationRelationId)
UnlockRelationOid(object->objectId, AccessExclusiveLock);
- else if (object->classId == AuthMemRelationId)
+ else if (IsSharedRelation(object->classId))
UnlockSharedObject(object->classId, object->objectId, 0,
AccessExclusiveLock);
else
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 1bb785f4f9f..259db747334 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -205,6 +205,10 @@ 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
+HINT: Drop subscription regress_testsub6 first.
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 f19740fdfb8..7718c742974 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -150,6 +150,8 @@ 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;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--
2.43.0
[text/x-patch] v2-0003-postgres_fdw-reject-use_scram_passthrough-for-sub.patch (3.5K, ../../2487ddcd737d4fc8e408e87aa9ad4365eed3bbb3.camel@j-davis.com/4-v2-0003-postgres_fdw-reject-use_scram_passthrough-for-sub.patch)
download | inline diff:
From 96bc4e83b40feaf747871081a48350991bd5b5f3 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 13:50:29 -0700
Subject: [PATCH v2 3/3] postgres_fdw: reject use_scram_passthrough for
subscriptions.
The subscription is initiated from a loical replication worker, so
SCRAM pass-through won't work.
Resolves finding 3 in report.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 19
---
contrib/postgres_fdw/connection.c | 12 ++++++++++++
contrib/postgres_fdw/t/010_subscription.pl | 14 +++++++++++++-
doc/src/sgml/postgres-fdw.sgml | 7 +++++++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index aab21695979..094eac2f343 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -2479,6 +2479,18 @@ postgres_fdw_connection(PG_FUNCTION_ARGS)
char *appname;
char *sep = "";
+ /*
+ * SCRAM pass-through cannot work for subscriptions because the connection
+ * happens in a worker process.
+ */
+ if (UseScramPassthrough(server, user))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("SCRAM pass-through authentication is not supported for subscription connections"),
+ errdetail("The foreign server or user mapping for user \"%s\" has \"use_scram_passthrough\" enabled.",
+ GetUserNameFromId(userid, false)),
+ errhint("Store a password in the user mapping instead.")));
+
construct_connection_params(server, user, &keywords, &values, &appname);
initStringInfo(&str);
diff --git a/contrib/postgres_fdw/t/010_subscription.pl b/contrib/postgres_fdw/t/010_subscription.pl
index c34b3d15b8d..53f1ea73ece 100644
--- a/contrib/postgres_fdw/t/010_subscription.pl
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -41,7 +41,19 @@ $node_subscriber->safe_psql('postgres',
);
$node_subscriber->safe_psql('postgres',
- "CREATE USER MAPPING FOR PUBLIC SERVER tap_server");
+ "CREATE USER MAPPING FOR PUBLIC SERVER tap_server OPTIONS (use_scram_passthrough 'true')");
+
+my ($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
+ "CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH (password_required=false)");
+isnt($ret, 0,
+ 'CREATE SUBSCRIPTION succeeds with use_scram_passthrough');
+like(
+ $stderr,
+ qr/ERROR.*SCRAM pass-through authentication is not supported for subscription connections/,
+ 'CREATE SUBSCRIPTION gives correct connection error');
+
+$node_subscriber->safe_psql('postgres',
+ "ALTER USER MAPPING FOR PUBLIC SERVER tap_server OPTIONS (DROP use_scram_passthrough)");
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH (password_required=false)"
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index b9e1b04463e..8b0669f672d 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -861,6 +861,13 @@ OPTIONS (ADD password_required 'false');
This is a technical requirement of the SCRAM protocol.
</para>
</listitem>
+
+ <listitem>
+ <para>
+ The foreign server must not be used for subscription connections
+ (see <xref linkend="postgres-fdw-server-subscription"/>).
+ </para>
+ </listitem>
</itemizedlist>
</para>
</listitem>
--
2.43.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-29 02:35 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 0 replies; 27+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-07-29 02:35 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>; Noah Misch <noah@leadboat.com>
Dear Jeff,
> There are two other threads discussing closely-related problems:
>
> https://www.postgresql.org/message-id/CAHGQGwFGa6+wWVgUmZPFwN=fB
> Y59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail.com
> https://www.postgresql.org/message-id/OS9PR01MB12149C3ED34272966B25
> DB173F5C12@OS9PR01MB12149.jpnprd01.prod.outlook.com
Not so closely, but here may be another one: ALTER SUBSCRIPTION REFRESH
PUBLICATION may allow reading foreign servers if even when the user does not
have enough privilege.
> To reconcile these goals, we need to weaken DDL-time validation a bit,
> be more precise about when we try to generate a conninfo, and then be
> sure that restore doesn't do anything that would cause a conninfo to be
> generated or a connection to happen.
Sounds reasonable. To confirm, the deferrable approach could solve the issue
reported by me because ALTER SUBSCRIPTION SET OWNER command won't
check the foreign server's privilege. Is it correct?
[1]: https://www.postgresql.org/message-id/CANhcyEU9VsaLwo908ws_1MxNB79f%2Bcr-JVfig%3DZoaf4%2BKQe%2BGQ%40...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-29 21:30 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-07-29 21:30 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Tue, 2026-07-28 at 09:10 -0700, Jeff Davis wrote:
> * During restore, we simply want it to recreate the right catalog
> state, and it uses multiple commands to do so (CREATE SUBSCRIPTION,
> ALTER SUBSCRIPTION OWNER TO, etc.). It should never connect to the
> publisher, and validation is mostly counterproductive for the
> intermediate states.
>
> * When we no longer need a slot (due to ALTER/DROP), we would like to
> drop it from the publisher, but for various reasons a connection to
> the
> publisher may be impossible. In that case, the user may still want
> the
> ALTER/DROP to succeed.
>
> * Validation at DDL-time is useful for interactive purposes, but
> limited. Whatever is validated may change before connection time
> (e.g.
> privileges on the server may be revoked), so connection-time
> validation
> is the authoritative one.
>
> To reconcile these goals, we need to weaken DDL-time validation a
> bit,
> be more precise about when we try to generate a conninfo, and then be
> sure that restore doesn't do anything that would cause a conninfo to
> be
> generated or a connection to happen.
Summary of which commands require a connection (and therefore cannot be
used during restore):
CREATE SUBSCRIPTION iff connect=true
ALTER SUBSCRIPTION SET (failover|twophase) iff slot_name
ALTER SUBSCRIPTION SET|ADD|DROP PUBLICATION iff refresh
ALTER SUBSCRIPTION REFRESH
DROP SUBSCRIPTION iff slot_name or tablesync slots
Proposal:
* Form a conninfo if and only if a connection is immediately
required. That is, it's one of the DDL commands above, or a
logical worker.
* Check USAGE on the server when a connection is formed or when
ALTER sets the server or when the subscription owner changes
(unless superuser changes the owner, in which case it may be part
of a multi-command DDL sequence during restore).
* Check that the server's FDW supports a connection function when
DDL sets the subscription's server.
* Check that a user mapping exists during DDL when the server or
owner changes, but demote the message to a WARNING, because it
may be part of a multi-command DDL sequence during restore.
- CREATE SUBSCRIPTION already issues WARNINGS during restore.
* Ensure that none of the commands during restore need a connection.
- check_pub_rdt should happen at connection time, and only
opportunistically at DDL time if already forming a connection
* Check walrcv_check_conninfo() before connecting, or during
CREATE/ALTER SUBSCRIPTION ... CONNECTION.
- If a connection is not needed for DDL, and it's a server-based
subscription, walrcv_check_conninfo() will be called only by
the logical worker when a connection is needed.
- That loses some convenience for interactive DDL, but avoids
false positive failures during restore.
If we reordered the commands during restore, as in Hayato Kuroda's
second patch[1], we could tighten the checks. But I'm not sure we want
to do that for v19.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/OS9PR01MB121493DA4C1A7748B11A646D8F5C02%40OS9PR01MB12149.jpnpr...
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-31 04:20 Jeff Davis <pgsql@j-davis.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-07-31 04:20 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers@postgresql.org, Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Wed, 2026-07-29 at 14:30 -0700, Jeff Davis wrote:
> Proposal:
>
> * Form a conninfo if and only if a connection is immediately
> required. That is, it's one of the DDL commands above, or a
> logical worker.
>
> * Check USAGE on the server when a connection is formed or when
> ALTER sets the server or when the subscription owner changes
> (unless superuser changes the owner, in which case it may be part
> of a multi-command DDL sequence during restore).
>
> * Check that the server's FDW supports a connection function when
> DDL sets the subscription's server.
>
> * Check that a user mapping exists during DDL when the server or
> owner changes, but demote the message to a WARNING, because it
> may be part of a multi-command DDL sequence during restore.
> - CREATE SUBSCRIPTION already issues WARNINGS during restore.
>
> * Ensure that none of the commands during restore need a connection.
> - check_pub_rdt should happen at connection time, and only
> opportunistically at DDL time if already forming a connection
>
> * Check walrcv_check_conninfo() before connecting, or during
> CREATE/ALTER SUBSCRIPTION ... CONNECTION.
> - If a connection is not needed for DDL, and it's a server-based
> subscription, walrcv_check_conninfo() will be called only by
> the logical worker when a connection is needed.
> - That loses some convenience for interactive DDL, but avoids
> false positive failures during restore.
Here's a consolidated series of commits.
Fujii, note that this includes a revert of your commit 1c9c358904.
Doing too much validation can cause problems for restore.
Amit, this series does not include the check_pub_rdt change to move it
to the worker.
Shlok Kyal, the change to detect when a refresh is happening is in my
patch 0006, the ACL check fix is in my patch 0007.
Hayato Kuroda, Amit already pushed the --no-subscriptions fix
(02decf9a9a). I didn't change the dump/restore order in this series,
because reducing the errors seems right for v19. We can reconsider it
for v20.
Regards,
Jeff Davis
Attachments:
[text/x-patch] v3-0001-Fix-lock-release-for-role-membership-grants-in-DR.patch (3.5K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/2-v3-0001-Fix-lock-release-for-role-membership-grants-in-DR.patch)
download | inline diff:
From a0150ddf088a622d38eac247ebd7ff862e7fb7a8 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 14:50:45 -0700
Subject: [PATCH v3 01/11] Fix lock release for role membership grants in DROP
OWNED BY.
Commit 6566133c5f5 added a case for AuthMemRelationId in
AcquireDeletionLock(), but not ReleaseDeletionLock(). The fall-through
case would go to UnlockDatabaseObject(), which would raise a WARNING;
and the lock would be retained until the end of the transaction.
Add the missing branch.
Discussion: https://postgr.es/m/2487ddcd737d4fc8e408e87aa9ad4365eed3bbb3.camel@j-davis.com
Backpatch-through: 16
---
src/backend/catalog/dependency.c | 3 ++
.../isolation/expected/drop-owned-grant.out | 8 +++++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/drop-owned-grant.spec | 30 +++++++++++++++++++
4 files changed, 42 insertions(+)
create mode 100644 src/test/isolation/expected/drop-owned-grant.out
create mode 100644 src/test/isolation/specs/drop-owned-grant.spec
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index c54774b3275..52cd2caf9d4 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1600,6 +1600,9 @@ ReleaseDeletionLock(const ObjectAddress *object)
{
if (object->classId == RelationRelationId)
UnlockRelationOid(object->objectId, AccessExclusiveLock);
+ else if (object->classId == AuthMemRelationId)
+ UnlockSharedObject(object->classId, object->objectId, 0,
+ AccessExclusiveLock);
else
/* assume we should lock the whole object not a sub-object */
UnlockDatabaseObject(object->classId, object->objectId, 0,
diff --git a/src/test/isolation/expected/drop-owned-grant.out b/src/test/isolation/expected/drop-owned-grant.out
new file mode 100644
index 00000000000..ea6cca277b6
--- /dev/null
+++ b/src/test/isolation/expected/drop-owned-grant.out
@@ -0,0 +1,8 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1b s1d s2d s1c
+step s1b: BEGIN;
+step s1d: DROP OWNED BY regress_dropowned_grantor;
+step s2d: DROP OWNED BY regress_dropowned_grantor; <waiting ...>
+step s1c: COMMIT;
+step s2d: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 26abed9f9f0..df8ce44ede6 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -129,3 +129,4 @@ test: lock-nowait
test: for-portion-of
test: ddl-dependency-locking
test: pub-concurrent-drop
+test: drop-owned-grant
diff --git a/src/test/isolation/specs/drop-owned-grant.spec b/src/test/isolation/specs/drop-owned-grant.spec
new file mode 100644
index 00000000000..636cc213a6b
--- /dev/null
+++ b/src/test/isolation/specs/drop-owned-grant.spec
@@ -0,0 +1,30 @@
+# Test locking of role membership grants during concurrent DROP OWNED BY.
+
+setup
+{
+ CREATE ROLE regress_dropowned_role;
+ CREATE ROLE regress_dropowned_member;
+ CREATE ROLE regress_dropowned_grantor;
+ GRANT regress_dropowned_role TO regress_dropowned_grantor
+ WITH ADMIN OPTION;
+ SET ROLE regress_dropowned_grantor;
+ GRANT regress_dropowned_role TO regress_dropowned_member;
+ RESET ROLE;
+}
+
+teardown
+{
+ DROP ROLE regress_dropowned_member;
+ DROP ROLE regress_dropowned_grantor;
+ DROP ROLE regress_dropowned_role;
+}
+
+session s1
+step s1b { BEGIN; }
+step s1d { DROP OWNED BY regress_dropowned_grantor; }
+step s1c { COMMIT; }
+
+session s2
+step s2d { DROP OWNED BY regress_dropowned_grantor; }
+
+permutation s1b s1d s2d s1c
--
2.43.0
[text/x-patch] v3-0002-Improve-DROP-SERVER-handling-of-dependent-subscri.patch (5.5K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/3-v3-0002-Improve-DROP-SERVER-handling-of-dependent-subscri.patch)
download | inline diff:
From deea81eb5b65343a6635acdaeb897ea91cd16e04 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 14:48:12 -0700
Subject: [PATCH v3 02/11] Improve DROP SERVER handling of dependent
subscriptions.
Acquire a lock on the subscription to avoid unnecessary errors. Also
issue a HINT and document the restriction that CASCADE won't cascade
to the subscription object.
Addresses finding 10 & 15 in report from linked discussion.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 19
---
doc/src/sgml/ref/drop_server.sgml | 4 +++
src/backend/catalog/dependency.c | 31 +++++++++++++---------
src/test/regress/expected/subscription.out | 4 +++
src/test/regress/sql/subscription.sql | 2 ++
4 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/drop_server.sgml b/doc/src/sgml/ref/drop_server.sgml
index f83a661b3eb..5fa0b763f36 100644
--- a/doc/src/sgml/ref/drop_server.sgml
+++ b/doc/src/sgml/ref/drop_server.sgml
@@ -66,6 +66,10 @@ DROP SERVER [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [, .
user mappings),
and in turn all objects that depend on those objects
(see <xref linkend="ddl-depend"/>).
+ However, a subscription that uses the server is never dropped
+ automatically; it must be dropped with
+ <link linkend="sql-dropsubscription"><command>DROP SUBSCRIPTION</command></link>
+ before the server can be dropped.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 52cd2caf9d4..c8dd78341eb 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -900,17 +900,6 @@ findDependentObjects(const ObjectAddress *object,
object->objectSubId == 0)
continue;
- /*
- * Check that the dependent object is not in a shared catalog, which
- * is not supported by doDeletion().
- */
- if (IsSharedRelation(otherObject.classId))
- ereport(ERROR,
- (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
- errmsg("cannot drop %s because %s depends on it",
- getObjectDescription(object, false),
- getObjectDescription(&otherObject, false))));
-
/*
* Must lock the dependent object before recursing to it.
*/
@@ -931,6 +920,22 @@ findDependentObjects(const ObjectAddress *object,
continue;
}
+ /*
+ * Check that the dependent object is not in a shared catalog, which
+ * is not supported by doDeletion().
+ */
+ if (IsSharedRelation(otherObject.classId))
+ {
+ char *otherObjDesc = getObjectDescription(&otherObject,
+ false);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST),
+ errmsg("cannot drop %s because %s depends on it",
+ getObjectDescription(object, false), otherObjDesc),
+ errhint("Drop %s first.", otherObjDesc)));
+ }
+
/*
* We do need to delete it, so identify objflags to be passed down,
* which depend on the dependency type.
@@ -1579,7 +1584,7 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
else
LockRelationOid(object->objectId, AccessExclusiveLock);
}
- else if (object->classId == AuthMemRelationId)
+ else if (IsSharedRelation(object->classId))
LockSharedObject(object->classId, object->objectId, 0,
AccessExclusiveLock);
else
@@ -1600,7 +1605,7 @@ ReleaseDeletionLock(const ObjectAddress *object)
{
if (object->classId == RelationRelationId)
UnlockRelationOid(object->objectId, AccessExclusiveLock);
- else if (object->classId == AuthMemRelationId)
+ else if (IsSharedRelation(object->classId))
UnlockSharedObject(object->classId, object->objectId, 0,
AccessExclusiveLock);
else
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 1bb785f4f9f..259db747334 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -205,6 +205,10 @@ 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
+HINT: Drop subscription regress_testsub6 first.
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 f19740fdfb8..7718c742974 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -150,6 +150,8 @@ 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;
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
--
2.43.0
[text/x-patch] v3-0003-postgres_fdw-reject-use_scram_passthrough-for-sub.patch (3.5K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/4-v3-0003-postgres_fdw-reject-use_scram_passthrough-for-sub.patch)
download | inline diff:
From da32e6ad7fc6d2a006340a2640bc2880caf27b97 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Tue, 28 Jul 2026 13:50:29 -0700
Subject: [PATCH v3 03/11] postgres_fdw: reject use_scram_passthrough for
subscriptions.
The subscription is initiated from a logical replication worker, so
SCRAM pass-through won't work.
Partially addresses finding 3 in report from linked discussion.
Reported-by: Noah Misch <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch@microsoft.com
Backpatch-through: 19
---
contrib/postgres_fdw/connection.c | 12 ++++++++++++
contrib/postgres_fdw/t/010_subscription.pl | 16 +++++++++++++++-
doc/src/sgml/postgres-fdw.sgml | 7 +++++++
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index aab21695979..094eac2f343 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -2479,6 +2479,18 @@ postgres_fdw_connection(PG_FUNCTION_ARGS)
char *appname;
char *sep = "";
+ /*
+ * SCRAM pass-through cannot work for subscriptions because the connection
+ * happens in a worker process.
+ */
+ if (UseScramPassthrough(server, user))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("SCRAM pass-through authentication is not supported for subscription connections"),
+ errdetail("The foreign server or user mapping for user \"%s\" has \"use_scram_passthrough\" enabled.",
+ GetUserNameFromId(userid, false)),
+ errhint("Store a password in the user mapping instead.")));
+
construct_connection_params(server, user, &keywords, &values, &appname);
initStringInfo(&str);
diff --git a/contrib/postgres_fdw/t/010_subscription.pl b/contrib/postgres_fdw/t/010_subscription.pl
index c34b3d15b8d..449fa35ce31 100644
--- a/contrib/postgres_fdw/t/010_subscription.pl
+++ b/contrib/postgres_fdw/t/010_subscription.pl
@@ -41,7 +41,21 @@ $node_subscriber->safe_psql('postgres',
);
$node_subscriber->safe_psql('postgres',
- "CREATE USER MAPPING FOR PUBLIC SERVER tap_server");
+ "CREATE USER MAPPING FOR PUBLIC SERVER tap_server OPTIONS (use_scram_passthrough 'true')"
+);
+
+my ($ret, $stdout, $stderr) = $node_subscriber->psql('postgres',
+ "CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH (password_required=false)"
+);
+isnt($ret, 0, 'CREATE SUBSCRIPTION fails with use_scram_passthrough');
+like(
+ $stderr,
+ qr/ERROR.*SCRAM pass-through authentication is not supported for subscription connections/,
+ 'CREATE SUBSCRIPTION gives correct connection error');
+
+$node_subscriber->safe_psql('postgres',
+ "ALTER USER MAPPING FOR PUBLIC SERVER tap_server OPTIONS (DROP use_scram_passthrough)"
+);
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub SERVER tap_server PUBLICATION tap_pub WITH (password_required=false)"
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index b9e1b04463e..8b0669f672d 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -861,6 +861,13 @@ OPTIONS (ADD password_required 'false');
This is a technical requirement of the SCRAM protocol.
</para>
</listitem>
+
+ <listitem>
+ <para>
+ The foreign server must not be used for subscription connections
+ (see <xref linkend="postgres-fdw-server-subscription"/>).
+ </para>
+ </listitem>
</itemizedlist>
</para>
</listitem>
--
2.43.0
[text/x-patch] v3-0004-Remove-Subscription-conninfo-field-generate-in-ca.patch (16.4K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/5-v3-0004-Remove-Subscription-conninfo-field-generate-in-ca.patch)
download | inline diff:
From c7eb3abedf60408d776f0906398c896a815c44a5 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 12:34:03 -0700
Subject: [PATCH v3 04/11] 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
Backpatch-through: 19
---
src/backend/catalog/pg_subscription.c | 100 ++++++++++--------
src/backend/commands/subscriptioncmds.c | 43 ++++++--
.../replication/logical/sequencesync.c | 2 +-
src/backend/replication/logical/tablesync.c | 2 +-
src/backend/replication/logical/worker.c | 24 ++++-
src/include/catalog/pg_subscription.h | 6 +-
src/include/replication/worker_internal.h | 1 +
7 files changed, 112 insertions(+), 66 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 5ff61edb989..8fa2a460a25 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,60 @@ GetSubscription(Oid subid, bool missing_ok, bool conninfo_needed,
return sub;
}
+/*
+ * Generate conninfo string for subscription.
+ *
+ * For server-based subscriptions this may raise an error (e.g. due to a
+ * missing user mapping).
+ */
+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 013ac46db07..6ca215f8bf3 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 0423745a428..8b187d15822 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 1ca19c1a7a8..74409fc9202 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)
@@ -6005,7 +6019,7 @@ SetupApplyOrSyncWorker(int worker_slot)
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
- MySubscription->conninfo);
+ MySubscriptionConninfo);
/*
* Setup callback for syscache so that we know when something changes in
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
[text/x-patch] v3-0005-Build-subscription-conninfo-after-checking-that-i.patch (2.5K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/6-v3-0005-Build-subscription-conninfo-after-checking-that-i.patch)
download | inline diff:
From 37e6fcdf9e10aecb6aab08952c449aca98b35e10 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 13:38:17 -0700
Subject: [PATCH v3 05/11] 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 <noah@leadboat.com>
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 74409fc9202..1cdd28f5049 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
[text/x-patch] v3-0006-Be-precise-about-when-ALTER-SUBSCRIPTION-needs-co.patch (6.4K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/7-v3-0006-Be-precise-about-when-ALTER-SUBSCRIPTION-needs-co.patch)
download | inline diff:
From 1797d4b1a7eb1450c3611e3ba039e2bf977fb6e4 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 13:40:02 -0700
Subject: [PATCH v3 06/11] 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 <shlok.kyal.oss@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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 6ca215f8bf3..d05ef3fb4b2 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
[text/x-patch] v3-0007-Always-check-foreign-server-USAGE-when-resolving-.patch (6.3K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/8-v3-0007-Always-check-foreign-server-USAGE-when-resolving-.patch)
download | inline diff:
From 331840bce6463b99416fe7614464e10498972232 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 13:47:29 -0700
Subject: [PATCH v3 07/11] 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 <shlok.kyal.oss@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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 8fa2a460a25..76f09836bee 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -191,7 +191,7 @@ GetSubscription(Oid subid, bool missing_ok)
* missing user mapping).
*/
char *
-SubscriptionConninfo(Subscription *sub, bool aclcheck)
+SubscriptionConninfo(Subscription *sub)
{
HeapTuple tup;
Form_pg_subscription subform;
@@ -211,18 +211,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 d05ef3fb4b2..77e10d78e64 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 1cdd28f5049..9c4c31a5cbc 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
[text/x-patch] v3-0008-For-subscription-DDL-demote-user-mapping-checks-t.patch (6.4K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/9-v3-0008-For-subscription-DDL-demote-user-mapping-checks-t.patch)
download | inline diff:
From 243475c3e7887c19db73720887dd741f0ceeb532 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 18:18:25 -0700
Subject: [PATCH v3 08/11] 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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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 77e10d78e64..b91c07d208e 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
[text/x-patch] v3-0009-CREATE-SUBSCRIPTION-do-not-construct-conninfo-unn.patch (5.2K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/10-v3-0009-CREATE-SUBSCRIPTION-do-not-construct-conninfo-unn.patch)
download | inline diff:
From b55b408bf6a8351d468ff06bd92617aa3de6e277 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 18:26:23 -0700
Subject: [PATCH v3 09/11] 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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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 b91c07d208e..d52050282a5 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
[text/x-patch] v3-0010-Revert-Validate-subscription-conninfo-on-owner-ch.patch (8.8K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/11-v3-0010-Revert-Validate-subscription-conninfo-on-owner-ch.patch)
download | inline diff:
From 04d22977acd8ada50a767b5717ee5d29cba09c3f Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 20:28:47 -0700
Subject: [PATCH v3 10/11] Revert "Validate subscription conninfo on owner
change"
This reverts commit 1c9c35890421e96a91129b51f2c6446a6d95af95.
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
Backpatch-through: 19
---
doc/src/sgml/ref/alter_subscription.sgml | 7 -------
src/backend/commands/subscriptioncmds.c | 14 ++------------
src/test/regress/expected/subscription.out | 21 ---------------------
src/test/regress/regress.c | 9 ---------
src/test/regress/sql/subscription.sql | 18 ------------------
5 files changed, 2 insertions(+), 67 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 d52050282a5..54f35d41c27 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..d447cba1397 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,22 +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
-HINT: Drop subscription regress_testsub6 first.
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
@@ -258,7 +238,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..dd33b18da55 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,19 +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;
-
REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
SET SESSION AUTHORIZATION regress_subscription_user3;
@@ -206,7 +189,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
[text/x-patch] v3-0011-When-changing-owner-of-a-subscription-do-not-thro.patch (4.4K, ../../c7fe6424386631771dd419d17d0c3d952c2e7619.camel@j-davis.com/12-v3-0011-When-changing-owner-of-a-subscription-do-not-thro.patch)
download | inline diff:
From 9e9acf56b793334e80d32a38c183253bdbb237c5 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 30 Jul 2026 18:30:48 -0700
Subject: [PATCH v3 11/11] 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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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 54f35d41c27..fd7c4780acc 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 d447cba1397..f67ece3ccfd 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -187,6 +187,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.
RESET SESSION AUTHORIZATION;
+-- 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 dd33b18da55..266bc6d9deb 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -133,6 +133,10 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
RESET SESSION AUTHORIZATION;
+-- 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-31 08:42 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-07-31 08:42 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers@postgresql.org, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Fri, Jul 31, 2026 at 9:50 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> >
> >
> > * Ensure that none of the commands during restore need a connection.
> > - check_pub_rdt should happen at connection time, and only
> > opportunistically at DDL time if already forming a connection
> >
>
> Amit, this series does not include the check_pub_rdt change to move it
> to the worker.
>
I looked into this problem and agreed that authoritative checking
required for 'rdt' should be done in the worker as even after DDL the
upstream can change. However, I feel it is better to detect the same
at DDL time whenever possible as well as it gives immediate,
synchronous feedback for interactive CREATE/ALTER, whereas a
worker-only failure just lands in the server log and the worker keeps
restarting. Removing it would also mean enabling retain_dead_tuples no
longer validates the publisher at all in the common interactive case.
The only where the DDL-time check is actively harmful is binary
upgrade, where we are just recreating catalog state and must not
connect. So, I would avoid doing that by using IsBinaryUpgrade similar
to how we do in launcher and also add worker-level check as done in
attached.
--
With Regards,
Amit Kapila.
Attachments:
[application/octet-stream] v1-0001-Validate-publisher-for-retain_dead_tuples-at-appl.patch (5.8K, ../../CAA4eK1Kz_JWAEmzTMSmGGPnxM0=EAZG_WcNB5dz2We6ezVW1DQ@mail.gmail.com/2-v1-0001-Validate-publisher-for-retain_dead_tuples-at-appl.patch)
download | inline diff:
From 6e24fbbbe8c37a3fa1bcc4d259fdef42812a8216 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 31 Jul 2026 11:54:50 +0530
Subject: [PATCH v1] Validate publisher for retain_dead_tuples at apply worker
connect time.
Enabling retain_dead_tuples checked the publisher (version >= 19 and not
in recovery) only at DDL time. That forced a connection to the publisher
while re-enabling subscriptions during pg_upgrade, so the upgrade failed
if the publisher was unreachable. It was also insufficient: the
publisher's version and recovery status can change after the DDL command
(for example, after a failover), leaving the check stale.
Move the authoritative check to the apply worker, which runs it when it
connects to the publisher. The DDL-time check is retained as a
convenience but is now skipped during binary upgrade, since that path only
recreates catalog state and must not connect to the publisher.
---
src/backend/commands/subscriptioncmds.c | 23 ++++++++++++++++++-----
src/backend/replication/logical/worker.c | 16 ++++++++++++++++
src/include/commands/subscriptioncmds.h | 4 ++++
3 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 67f5699b2c7..c2a62198d45 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -139,7 +139,6 @@ static void check_publications_origin_sequences(WalReceiverConn *wrconn,
Oid *subrel_local_oids,
int subrel_count,
char *subname);
-static void check_pub_dead_tuple_retention(WalReceiverConn *wrconn);
static void check_duplicates_in_publist(List *publist, Datum *datums);
static List *merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -977,7 +976,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
NULL, 0, stmt->subname);
if (opts.retaindeadtuples)
- check_pub_dead_tuple_retention(wrconn);
+ CheckPubDeadTupleRetention(wrconn);
/*
* Set sync state based on if we were asked to do data copy or
@@ -2391,6 +2390,15 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
heap_freetuple(tup);
}
+ /*
+ * During binary upgrade, we only recreate the catalog state and must not
+ * connect to the publisher. The publisher's suitability for
+ * retain_dead_tuples is validated authoritatively by the apply worker
+ * when it connects, so skip the opportunistic DDL-time check here.
+ */
+ if (IsBinaryUpgrade)
+ check_pub_rdt = false;
+
/*
* Try to acquire the connection necessary either for modifying the slot
* or for checking if the remote server permits enabling
@@ -2428,7 +2436,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
PG_TRY();
{
if (retain_dead_tuples)
- check_pub_dead_tuple_retention(wrconn);
+ CheckPubDeadTupleRetention(wrconn);
check_publications_origin_tables(wrconn, sub->publications, false,
retain_dead_tuples, origin, NULL, 0,
@@ -3355,10 +3363,15 @@ check_publications_origin_sequences(WalReceiverConn *wrconn, List *publications,
* than the PG19, or if the publisher is in recovery (i.e., it is a standby
* server).
*
+ * This is used both at DDL time (as a convenience, when a connection to the
+ * publisher is already being made) and by the apply worker when it connects,
+ * which is the authoritative check because the publisher's version and
+ * recovery status can change after the DDL command.
+ *
* See comments atop worker.c for a detailed explanation.
*/
-static void
-check_pub_dead_tuple_retention(WalReceiverConn *wrconn)
+void
+CheckPubDeadTupleRetention(WalReceiverConn *wrconn)
{
WalRcvExecResult *res;
Oid RecoveryRow[1] = {BOOLOID};
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 1ca19c1a7a8..80ce11d7c0c 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5734,6 +5734,22 @@ run_apply_worker(void)
*/
(void) walrcv_identify_system(LogRepWorkerWalRcvConn, &startpointTLI, NULL);
+ /*
+ * If retain_dead_tuples is enabled, verify that the publisher is suitable,
+ * that is, it runs a version that supports the feature and is not in
+ * recovery. This is the authoritative check. Although the same
+ * validation is performed opportunistically at DDL time, the publisher's
+ * version or recovery status may have changed since then (for example,
+ * after a failover), and DDL-time validation is skipped entirely during
+ * binary upgrade.
+ */
+ if (MySubscription->retaindeadtuples)
+ {
+ StartTransactionCommand();
+ CheckPubDeadTupleRetention(LogRepWorkerWalRcvConn);
+ CommitTransactionCommand();
+ }
+
set_apply_error_context_origin(originname);
set_stream_options(&options, slotname, &origin_startpos);
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 63504232a14..c735db60020 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,6 +18,8 @@
#include "catalog/objectaddress.h"
#include "parser/parse_node.h"
+struct WalReceiverConn; /* avoid pulling in walreceiver.h here */
+
extern ObjectAddress CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
bool isTopLevel);
extern ObjectAddress AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, bool isTopLevel);
@@ -36,4 +38,6 @@ extern void CheckSubDeadTupleRetention(bool check_guc, bool sub_disabled,
bool retention_active,
bool max_retention_set);
+extern void CheckPubDeadTupleRetention(struct WalReceiverConn *wrconn);
+
#endif /* SUBSCRIPTIONCMDS_H */
--
2.54.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-31 16:09 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-07-31 16:09 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers@postgresql.org, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Fri, 2026-07-31 at 14:12 +0530, Amit Kapila wrote:
> However, I feel it is better to detect the same
> at DDL time whenever possible as well as it gives immediate,
> synchronous feedback for interactive CREATE/ALTER, whereas a
> worker-only failure just lands in the server log and the worker keeps
> restarting. Removing it would also mean enabling retain_dead_tuples
> no
> longer validates the publisher at all in the common interactive case.
I believe the only problem case is ALTER SUBSCRIPTION ... ENABLE,
right?
CREATE doesn't do the check when connect=false, so that's the same
behavior.
None of ALTER ... SERVER, ALTER ... CONNECTION, or ALTER ... SET
(retain_dead_tuples) are called by restore because it sets those things
with the CREATE statement.
If you'd still like ALTER SUBSCRIPTION ... ENABLE to do the convenience
check, then I think you could clarify the problem case in the comments:
+ /*
+ * During binary upgrade, we only recreate the catalog state and
must not
+ * connect to the publisher. The publisher's suitability for
+ * retain_dead_tuples is validated authoritatively by the apply
worker
+ * when it connects, so skip the opportunistic DDL-time check here.
+ */
+ if (IsBinaryUpgrade)
+ check_pub_rdt = false;
During any restore we must not connect to the publisher. It's only a
problem for binary upgrade because that's what issues the ENABLE.
But the overall logic is more like "restore must not create any
connections, therefore it must not issue any commands that set
check_pub_rdt". We can't detect an ordinary restore (because it's
treated the same as interactive SQL), so we just have to be sure not to
introduce check_pub_rdt cases in the ordinary restore path later.
Also, we need to integrate it with the series I posted because we must
not generate the conninfo if IsBinaryUpgrade. I expect yours will go in
first and I can rebase on that, so you don't need to make a change
here.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-07-31 23:15 Jeff Davis <pgsql@j-davis.com>
parent: Noah Misch <noah@leadboat.com>
2 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-07-31 23:15 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: pgsql-hackers
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/20260710192533.4f.noahmisch@microsoft.com).
>
> 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.
A question about Finding 5, which has two parts:
(a) Disabling a SERVER subscription and dropping its user mapping in
one transaction makes the running worker exit with 'ERROR: user mapping
not found'
(b) Rotating a live mapping via DROP+CREATE (separate commits) can
permanently disable the subscription if the worker rereads in the gap.
I already published a patch for (a).
Part (b) is about the definition of disable_on_error, which is
documented:
"Specifies whether the subscription should be automatically disabled if
any errors are detected by subscription workers during data replication
from the publisher. The default is false."
Finding 5 seems to interpret "during data replication" to mean
"conflict on the remote side", but not other kinds of errors. Is that
the right interpretation? Or should most kinds of errors result in the
subscription being disabled?
Finding 5 frames DROP USER MAPPING + CREATE USER MAPPING (in different
commits) as something that should not cause the subscription to be
disabled. But if the DROP has happened and the CREATE has not, what
reason do we have to think the error is not permanent? If it's an
administrative rotation of some kind, why not alter-in-place or wrap it
in a transaction?
Or, perhaps these are just edge cases, and part (b) is not very
important?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-03 05:37 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 2 replies; 27+ messages in thread
From: Amit Kapila @ 2026-08-03 05:37 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers@postgresql.org, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Fri, Jul 31, 2026 at 9:39 PM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Fri, 2026-07-31 at 14:12 +0530, Amit Kapila wrote:
> > However, I feel it is better to detect the same
> > at DDL time whenever possible as well as it gives immediate,
> > synchronous feedback for interactive CREATE/ALTER, whereas a
> > worker-only failure just lands in the server log and the worker keeps
> > restarting. Removing it would also mean enabling retain_dead_tuples
> > no
> > longer validates the publisher at all in the common interactive case.
>
> I believe the only problem case is ALTER SUBSCRIPTION ... ENABLE,
> right?
>
> CREATE doesn't do the check when connect=false, so that's the same
> behavior.
>
> None of ALTER ... SERVER, ALTER ... CONNECTION, or ALTER ... SET
> (retain_dead_tuples) are called by restore because it sets those things
> with the CREATE statement.
>
>
> If you'd still like ALTER SUBSCRIPTION ... ENABLE to do the convenience
> check, then I think you could clarify the problem case in the comments:
>
> + /*
> + * During binary upgrade, we only recreate the catalog state and
> must not
> + * connect to the publisher. The publisher's suitability for
> + * retain_dead_tuples is validated authoritatively by the apply
> worker
> + * when it connects, so skip the opportunistic DDL-time check here.
> + */
> + if (IsBinaryUpgrade)
> + check_pub_rdt = false;
>
> During any restore we must not connect to the publisher. It's only a
> problem for binary upgrade because that's what issues the ENABLE.
>
> But the overall logic is more like "restore must not create any
> connections, therefore it must not issue any commands that set
> check_pub_rdt". We can't detect an ordinary restore (because it's
> treated the same as interactive SQL), so we just have to be sure not to
> introduce check_pub_rdt cases in the ordinary restore path later.
>
So, how about a comment like:
/*
* Skip the DDL-time retain_dead_tuples check during binary upgrade.
*
* A restore must not connect to the publisher, so it must not run any
* command that sets check_pub_rdt. We can only detect binary upgrade
(an
* ordinary restore is indistinguishable from interactive SQL), and
the
* only command it issues that would set check_pub_rdt is ALTER
* SUBSCRIPTION ... ENABLE (see dumpSubscription). Clear it here
* defensively. The apply worker validates the publisher
authoritatively
* when it connects.
*/
Feel free to suggest a different comment or an update to the above
comment if you don't like it.
OTOH, I am also fine if you prefer to remove the retain_dead_tuples
check entirely from the ENABLE path and keep it in other existing
paths as in attached. Actually, that will slightly simplify the code
as well.
--
With Regards,
Amit Kapila.
Attachments:
[application/octet-stream] v1-0001-approach-2-Validate-publisher-for-retain_dead_tuples-in-the-.patch (5.7K, ../../CAA4eK1LuhdhAOwi0NRad5oSuk1WD2UJ2if9=cmRRPd7345mBAQ@mail.gmail.com/2-v1-0001-approach-2-Validate-publisher-for-retain_dead_tuples-in-the-.patch)
download | inline diff:
From 451efe8a21621f9785c44730ab568bb3a1291643 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 3 Aug 2026 11:03:25 +0530
Subject: [PATCH v3] Validate publisher for retain_dead_tuples in the apply
worker.
Enabling retain_dead_tuples requires the publisher to run PostgreSQL 19
or later and to not be in recovery. Previously this was checked only at
DDL time. That forced ALTER SUBSCRIPTION ... ENABLE to connect to the
publisher, so pg_upgrade (which re-enables subscriptions during restore)
failed if the publisher was unreachable. It was also not authoritative,
since the publisher's version or recovery status can change afterwards,
for example after a failover.
Perform the check authoritatively in the apply worker when it connects,
and stop running it when enabling a subscription. ENABLE is the only
command issued during restore that triggered it, so this also fixes the
pg_upgrade failure. The DDL-time check is kept as a convenience for the
other paths, none of which are issued during restore.
---
src/backend/commands/subscriptioncmds.c | 22 +++++++++-------------
src/backend/replication/logical/worker.c | 15 +++++++++++++++
src/include/commands/subscriptioncmds.h | 4 ++++
3 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 67f5699b2c7..d3eef2a3efa 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -139,7 +139,6 @@ static void check_publications_origin_sequences(WalReceiverConn *wrconn,
Oid *subrel_local_oids,
int subrel_count,
char *subname);
-static void check_pub_dead_tuple_retention(WalReceiverConn *wrconn);
static void check_duplicates_in_publist(List *publist, Datum *datums);
static List *merge_publications(List *oldpublist, List *newpublist, bool addpub, const char *subname);
static void ReportSlotConnectionError(List *rstates, Oid subid, char *slotname, char *err);
@@ -977,7 +976,7 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
NULL, 0, stmt->subname);
if (opts.retaindeadtuples)
- check_pub_dead_tuple_retention(wrconn);
+ CheckPubDeadTupleRetention(wrconn);
/*
* Set sync state based on if we were asked to do data copy or
@@ -2084,14 +2083,6 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
ApplyLauncherWakeupAtCommit();
update_tuple = true;
-
- /*
- * The subscription might be initially created with
- * connect=false and retain_dead_tuples=true, meaning the
- * remote server's status may not be checked. Ensure this
- * check is conducted now.
- */
- check_pub_rdt = sub->retaindeadtuples && opts.enabled;
break;
}
@@ -2428,7 +2419,7 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
PG_TRY();
{
if (retain_dead_tuples)
- check_pub_dead_tuple_retention(wrconn);
+ CheckPubDeadTupleRetention(wrconn);
check_publications_origin_tables(wrconn, sub->publications, false,
retain_dead_tuples, origin, NULL, 0,
@@ -3355,10 +3346,15 @@ check_publications_origin_sequences(WalReceiverConn *wrconn, List *publications,
* than the PG19, or if the publisher is in recovery (i.e., it is a standby
* server).
*
+ * This is used both at DDL time (as a convenience, when a connection to the
+ * publisher is already being made) and by the apply worker when it connects,
+ * which is the authoritative check because the publisher's version and
+ * recovery status can change after the DDL command.
+ *
* See comments atop worker.c for a detailed explanation.
*/
-static void
-check_pub_dead_tuple_retention(WalReceiverConn *wrconn)
+void
+CheckPubDeadTupleRetention(WalReceiverConn *wrconn)
{
WalRcvExecResult *res;
Oid RecoveryRow[1] = {BOOLOID};
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 1ca19c1a7a8..79ec6fae59e 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5734,6 +5734,21 @@ run_apply_worker(void)
*/
(void) walrcv_identify_system(LogRepWorkerWalRcvConn, &startpointTLI, NULL);
+ /*
+ * If retain_dead_tuples is enabled, verify that the publisher is suitable,
+ * that is, it runs a version that supports the feature and is not in
+ * recovery. This is the authoritative check. Although the same validation
+ * is performed opportunistically at DDL time, the publisher's version or
+ * recovery status may have changed since then, for example after a
+ * failover.
+ */
+ if (MySubscription->retaindeadtuples)
+ {
+ StartTransactionCommand();
+ CheckPubDeadTupleRetention(LogRepWorkerWalRcvConn);
+ CommitTransactionCommand();
+ }
+
set_apply_error_context_origin(originname);
set_stream_options(&options, slotname, &origin_startpos);
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 63504232a14..c735db60020 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -18,6 +18,8 @@
#include "catalog/objectaddress.h"
#include "parser/parse_node.h"
+struct WalReceiverConn; /* avoid pulling in walreceiver.h here */
+
extern ObjectAddress CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
bool isTopLevel);
extern ObjectAddress AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt, bool isTopLevel);
@@ -36,4 +38,6 @@ extern void CheckSubDeadTupleRetention(bool check_guc, bool sub_disabled,
bool retention_active,
bool max_retention_set);
+extern void CheckPubDeadTupleRetention(struct WalReceiverConn *wrconn);
+
#endif /* SUBSCRIPTIONCMDS_H */
--
2.54.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-03 10:36 Amit Kapila <amit.kapila16@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-08-03 10:36 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers
On Sat, Aug 1, 2026 at 4:45 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> A question about Finding 5, which has two parts:
>
> (a) Disabling a SERVER subscription and dropping its user mapping in
> one transaction makes the running worker exit with 'ERROR: user mapping
> not found'
>
> (b) Rotating a live mapping via DROP+CREATE (separate commits) can
> permanently disable the subscription if the worker rereads in the gap.
>
> I already published a patch for (a).
>
> Part (b) is about the definition of disable_on_error, which is
> documented:
>
> "Specifies whether the subscription should be automatically disabled if
> any errors are detected by subscription workers during data replication
> from the publisher. The default is false."
>
> Finding 5 seems to interpret "during data replication" to mean
> "conflict on the remote side", but not other kinds of errors. Is that
> the right interpretation? Or should most kinds of errors result in the
> subscription being disabled?
>
As per my understanding, most kinds of errors result in the
subscription being disabled.
> Finding 5 frames DROP USER MAPPING + CREATE USER MAPPING (in different
> commits) as something that should not cause the subscription to be
> disabled. But if the DROP has happened and the CREATE has not, what
> reason do we have to think the error is not permanent?
>
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.
> 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).
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:
/*
* 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.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-03 17:17 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 27+ messages in thread
From: Jeff Davis @ 2026-08-03 17:17 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers@postgresql.org, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>
On Mon, 2026-08-03 at 11:07 +0530, Amit Kapila wrote:
> OTOH, I am also fine if you prefer to remove the retain_dead_tuples
> check entirely from the ENABLE path and keep it in other existing
> paths as in attached. Actually, that will slightly simplify the code
> as well.
The patch looks good to me.
For approach 1 versus 2, I'll defer to you and authors/reviewers of the
feature who understand the use cases better. I slightly prefer the
above approach (don't connect at ENABLE time) because side effects at
DDL time are hard to reason about.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-04 01:00 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 27+ messages in thread
From: Jeff Davis @ 2026-08-04 01:00 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers
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
Attachments:
[text/x-patch] v4-0001-Remove-Subscription-conninfo-field-generate-in-ca.patch (16.5K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/2-v4-0001-Remove-Subscription-conninfo-field-generate-in-ca.patch)
download | inline diff:
From 7631a896d83e08372509486387475360937f4c8d Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <amit.kapila16@gmail.com>
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
[text/x-patch] v4-0002-Build-subscription-conninfo-after-checking-that-i.patch (2.5K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/3-v4-0002-Build-subscription-conninfo-after-checking-that-i.patch)
download | inline diff:
From 1b8ea3d6a17741a1c007f8478810480ce56114f4 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <noah@leadboat.com>
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
[text/x-patch] v4-0003-Be-precise-about-when-ALTER-SUBSCRIPTION-needs-co.patch (6.4K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/4-v4-0003-Be-precise-about-when-ALTER-SUBSCRIPTION-needs-co.patch)
download | inline diff:
From ec7b48e48830c9ca35af1d1e3cc8bc9a92687c4f Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <shlok.kyal.oss@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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
[text/x-patch] v4-0004-Always-check-foreign-server-USAGE-when-resolving-.patch (6.3K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/5-v4-0004-Always-check-foreign-server-USAGE-when-resolving-.patch)
download | inline diff:
From 4eef163f2724ab4043fa39321aeeed5975da33c9 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <shlok.kyal.oss@gmail.com>
Reported-by: Noah Misch <noah@leadboat.com>
Reviewed-by: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
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
[text/x-patch] v4-0005-For-subscription-DDL-demote-user-mapping-checks-t.patch (6.4K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/6-v4-0005-For-subscription-DDL-demote-user-mapping-checks-t.patch)
download | inline diff:
From 65b089678c3724557f97ff0837cd202a35026e7d Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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
[text/x-patch] v4-0006-CREATE-SUBSCRIPTION-do-not-construct-conninfo-unn.patch (5.2K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/7-v4-0006-CREATE-SUBSCRIPTION-do-not-construct-conninfo-unn.patch)
download | inline diff:
From 1dc48bdab5c9375ed01eef7804e9b1e41c1baaee Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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
[text/x-patch] v4-0007-Revert-Validate-subscription-conninfo-on-owner-ch.patch (8.5K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/8-v4-0007-Revert-Validate-subscription-conninfo-on-owner-ch.patch)
download | inline diff:
From e879c3ad114c3f261c3869e517a1ee868a2f5cb8 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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
[text/x-patch] v4-0008-When-changing-owner-of-a-subscription-do-not-thro.patch (4.3K, ../../d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com/9-v4-0008-When-changing-owner-of-a-subscription-do-not-thro.patch)
download | inline diff:
From 6acc78cdf70263adc4e6cfe0fca1ebc7ac31dc2b Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
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 <noah@leadboat.com>
Discussion: https://postgr.es/m/20260710195902.4f.noahmisch%40microsoft.com
Discussion: https://postgr.es/m/e103ae8daf74485e0c0ebde297fae735d38f54d1.camel@j-davis.com
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-04 02:59 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 27+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-08-04 02:59 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>; Jeff Davis <pgsql@j-davis.com>
Dear Amit,
> OTOH, I am also fine if you prefer to remove the retain_dead_tuples
> check entirely from the ENABLE path and keep it in other existing
> paths as in attached. Actually, that will slightly simplify the code
> as well.
I also preferred the approach 2. I tested on PG19 and master, and confirmed
it could pass tests with the Jeff's reproducer. maybe_reread_subscription()
did not take care the parameter change, but it's ok because it cannot be
altered for the enabled subscription.
The patch LGTM.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-04 10:07 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 27+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-08-04 10:07 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers
Dear Jeff,
> New v4 series attached.
Thanks for updating the patch. While seeing 0001, I found that GetSubscription()
and SubscriptionConninfo() are called without acquiring the lock for the
subscription. Is there a possibility that DDL commands are executed concurrently
and the cache can be invalidated in-between, thus it causes inconsistent results?
I tested concurrent ALTER SUBSCRIPTION SET OWNER command in maybe_reread_subscription(),
but the cache is not invalidated thus changing the owner is not reflected.
Is it OK?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-04 12:53 Amit Kapila <amit.kapila16@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 0 replies; 27+ messages in thread
From: Amit Kapila @ 2026-08-04 12:53 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers; Shlok Kyal <shlok.kyal.oss@gmail.com>; Fujii Masao <masao.fujii@gmail.com>; yuanchao zhang <145zhangyc@gmail.com>; Jeff Davis <pgsql@j-davis.com>
On Tue, Aug 4, 2026 at 8:29 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > OTOH, I am also fine if you prefer to remove the retain_dead_tuples
> > check entirely from the ENABLE path and keep it in other existing
> > paths as in attached. Actually, that will slightly simplify the code
> > as well.
>
> I also preferred the approach 2. I tested on PG19 and master, and confirmed
> it could pass tests with the Jeff's reproducer. maybe_reread_subscription()
> did not take care the parameter change, but it's ok because it cannot be
> altered for the enabled subscription.
>
> The patch LGTM.
>
Thanks, I pushed the patch.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-04 21:30 Jeff Davis <pgsql@j-davis.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-08-04 21:30 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers
On Tue, 2026-08-04 at 10:07 +0000, Hayato Kuroda (Fujitsu) wrote:
> Thanks for updating the patch. While seeing 0001, I found that
> GetSubscription()
> and SubscriptionConninfo() are called without acquiring the lock for
> the
> subscription. Is there a possibility that DDL commands are executed
> concurrently
> and the cache can be invalidated in-between, thus it causes
> inconsistent results?
> I tested concurrent ALTER SUBSCRIPTION SET OWNER command in
> maybe_reread_subscription(),
> but the cache is not invalidated thus changing the owner is not
> reflected.
> Is it OK?
Can you explain in more detail the problem case? Is it unique to
server-based subscriptions?
Looking at the code it seems that invalidations can be missed if they
come between the time the catalogs are read and the time that
MySubscriptionValid is set. But I think that's a pre-existing issue --
perhaps we should start a new thread about that?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* RE: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-05 06:43 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 27+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-08-05 06:43 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers; Amit Kapila <amit.kapila16@gmail.com>
Dear Jeff,
> Can you explain in more detail the problem case? Is it unique to
> server-based subscriptions?
No, I initially thought that it could happen for both cases. But it seems no need to
consider - the invalidation messages can be accepted only at commit or while no
transactions are received. My worry that GetSubscription() and
GetSubscriptionConninfo() may refer the different tuple won't happen.
> Looking at the code it seems that invalidations can be missed if they
> come between the time the catalogs are read and the time that
> MySubscriptionValid is set. But I think that's a pre-existing issue --
> perhaps we should start a new thread about that?
I analyzed and I feel it's not problematic. Yes, if the ALTER SUBSCRIPTION DISABLE
is executed while in the maybe_reread_subscription(), it can be ignored once.
At the end of transaction or end of the loop, the invalidation message for the
subscription can be accepted and MySubscriptionValid can be false, the worker will
exit. This meant the worker can exit after handling the current transaction.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-05 10:08 Shlok Kyal <shlok.kyal.oss@gmail.com>
parent: Jeff Davis <pgsql@j-davis.com>
1 sibling, 1 reply; 27+ messages in thread
From: Shlok Kyal @ 2026-08-05 10:08 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
On Tue, 4 Aug 2026 at 06:31, Jeff Davis <pgsql@j-davis.com> wrote:
>
> 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.
>
Hi Jeff,
I have tested the patches and confirm that the issue reported by me in
[1] is addressed.
Also, I tested for the issue reported by Kuroda-san in [2]. And
confirm that it is also addressed.
[1]: https://www.postgresql.org/message-id/CANhcyEU9VsaLwo908ws_1MxNB79f%2Bcr-JVfig%3DZoaf4%2BKQe%2BGQ%40...
[2]: https://www.postgresql.org/message-id/OS9PR01MB12149C3ED34272966B25DB173F5C12@OS9PR01MB12149.jpnprd0...
Thanks,
Shlok Kyal
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-05 11:17 Amit Kapila <amit.kapila16@gmail.com>
parent: Shlok Kyal <shlok.kyal.oss@gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Amit Kapila @ 2026-08-05 11:17 UTC (permalink / raw)
To: Shlok Kyal <shlok.kyal.oss@gmail.com>; +Cc: Jeff Davis <pgsql@j-davis.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
On Wed, Aug 5, 2026 at 3:38 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Tue, 4 Aug 2026 at 06:31, Jeff Davis <pgsql@j-davis.com> wrote:
> >
> >
> > New v4 series attached.
> >
> Hi Jeff,
>
> I have tested the patches and confirm that the issue reported by me in
> [1] is addressed.
> Also, I tested for the issue reported by Kuroda-san in [2]. And
> confirm that it is also addressed.
>
BTW, I had also looked at the overall patch series, the idea and
high-level code looks good to me. Though I haven't done a detailed
testing or review of the same but as Shlok and Kuroda-San seem to have
reviewed/tested these patches, I think we can go-ahead with these
fixes.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-06 02:18 Jeff Davis <pgsql@j-davis.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-08-06 02:18 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; +Cc: Noah Misch <noah@leadboat.com>; pgsql-hackers
On Wed, 2026-08-05 at 16:47 +0530, Amit Kapila wrote:
> BTW, I had also looked at the overall patch series, the idea and
> high-level code looks good to me. Though I haven't done a detailed
> testing or review of the same but as Shlok and Kuroda-San seem to
> have
> reviewed/tested these patches, I think we can go-ahead with these
> fixes.
Thank you all, pushed.
AI-assisted summary of reported issues:
Finding 1: CREATE (connect=false) throws error during restore
fix: 5c39be9e6d, 254beb174e
Finding 2: OWNER TO could throw error during restore
fix: ac230cd3ce, 7f4fb05807
Finding 3: SCRAM passthrough
fix: d63f214d68
note: GSS delegation and other session state could still
cause differences between DDL and worker
Finding 4: REASSIGN OWNED fails across databases
fix: 03bcb6a777
Finding 5: error building conninfo before enabled check
fix: 8b73ceb78f
note: parallel worker fall-through left as edge case
Finding 6: concurrent DROP SERVER dangling subserver
note: withdrawn by Claude
Finding 7: ALTER that connects skipped foreign-server USAGE
fix: dcfb02deed, e90e198e3f
Finding 8: \dew omits fdwconnection
note: open
Finding 9: DROP SUBSCRIPTION hard-fails if mapping missing
with non-READY rels
note: discussed here:
https://postgr.es/m/CAA4eK1KzHQ_0rhN8uP+OB8dnB+uLBgod6tTx8j3GHxazz_=FiQ@m
ail.gmail.com
Finding 10: DROP SERVER CASCADE vs dependent subscription
fix: b66ea17a3a, b48caa7dfd
Finding 11: CREATE FDW docs omit CONNECTION return type
note: open
Finding 12: local ALTER forms fail without usable conninfo
fix: e90e198e3f, 6d04cd2b84
Finding 13: CREATE checks mapping before FDW capability
fix: 5c39be9e6d, 254beb174e
Finding 14: worker DEBUG1 logs subscription conninfo
fix: 448c4b3bfd
Finding 15: DROP SERVER errors before locking dependent
fix: b66ea17a3a
Finding 16: fdwhandler.sgml silent on connection functions
note: open
Finding 17: ALTER SUBSCRIPTION docs for SERVER / OWNER TO
note: partial
Finding 18: server application_name overrides subscription name
note: open; not clear whether this is correct or a bug
Finding 19: worker startup invalidation window for conninfo
note: open; similar issue is pre-existing
A few other problems were found and fixed along the way.
I believe the restore problem and other serious issues have been
addressed, so I will close the open item.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-10 02:15 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2026-08-10 02:15 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
Jeff Davis <pgsql@j-davis.com> writes:
> On Wed, 2026-08-05 at 16:47 +0530, Amit Kapila wrote:
>> BTW, I had also looked at the overall patch series, the idea and
>> high-level code looks good to me. Though I haven't done a detailed
>> testing or review of the same but as Shlok and Kuroda-San seem to
>> have
>> reviewed/tested these patches, I think we can go-ahead with these
>> fixes.
> Thank you all, pushed.
Coverity thinks there is a hole in this logic:
/srv/coverity/git/pgsql-git/postgresql/src/backend/commands/subscriptioncmds.c: 875 in CreateSubscription()
869 values[Anum_pg_subscription_submaxretention - 1] =
870 Int32GetDatum(opts.maxretention);
871 values[Anum_pg_subscription_subretentionactive - 1] =
872 BoolGetDatum(opts.retaindeadtuples);
873 values[Anum_pg_subscription_subserver - 1] = ObjectIdGetDatum(serverid);
874 if (!OidIsValid(serverid))
>>> CID 1699896: Null pointer dereferences (FORWARD_NULL)
>>> Passing null pointer "conninfo" to "cstring_to_text", which dereferences it.
875 values[Anum_pg_subscription_subconninfo - 1] =
876 CStringGetTextDatum(conninfo);
877 else
878 nulls[Anum_pg_subscription_subconninfo - 1] = true;
879 if (opts.slot_name)
880 values[Anum_pg_subscription_subslotname - 1] =
AFAICS, it's right: if stmt->servername is set while opts.connect is
not, we'll arrive at this step with serverid filled in but conninfo
still NULL. Even if there's some upstream reason why that combination
can't occur, this is pretty fragile-looking code. It's far from clear
why serverid has anything to do with conninfo being available.
regards, tom lane
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-10 15:04 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Davis @ 2026-08-10 15:04 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
On Sun, 2026-08-09 at 22:15 -0400, Tom Lane wrote:
> 874 if (!OidIsValid(serverid))
> > > > CID 1699896: Null pointer dereferences
> > > > (FORWARD_NULL)
> > > > Passing null pointer "conninfo" to "cstring_to_text", which
> > > > dereferences it.
> 875 values[Anum_pg_subscription_subconninfo - 1]
> =
> 876 CStringGetTextDatum(conninfo);
>
> AFAICS, it's right: if stmt->servername is set while opts.connect is
> not, we'll arrive at this step with serverid filled in but conninfo
> still NULL. Even if there's some upstream reason why that
> combination
> can't occur, this is pretty fragile-looking code.
If serverid is filled, that branch won't be taken.
The code relies on the grammar setting either stmt->servername or stmt-
>conninfo, but not both. I agree that the control flow shouldn't rely
on that, and the code could be more clear anyway. Patch attached.
Regards,
Jeff Davis
Attachments:
[text/x-patch] 0001-Clarify-logic-in-CreateSubscription.patch (1.5K, ../../68d2a8e0e373294cb04910dd080ad1d04421fde8.camel@j-davis.com/2-0001-Clarify-logic-in-CreateSubscription.patch)
download | inline diff:
From 1506505498b879b736aa2be718667c07b8e9cef3 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Mon, 10 Aug 2026 06:59:29 -0700
Subject: [PATCH] Clarify logic in CreateSubscription().
No bug found in previous code, but it unnecessarily relied on grammar
rules. Per complaint from Coverity.
Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/1787286.1786328153@sss.pgh.pa.us
Backpatch-through: 19
---
src/backend/commands/subscriptioncmds.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 8e8db08bd93..9938f4b7fe3 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -871,11 +871,17 @@ CreateSubscription(ParseState *pstate, CreateSubscriptionStmt *stmt,
values[Anum_pg_subscription_subretentionactive - 1] =
BoolGetDatum(opts.retaindeadtuples);
values[Anum_pg_subscription_subserver - 1] = ObjectIdGetDatum(serverid);
- if (!OidIsValid(serverid))
+ if (stmt->conninfo)
+ {
+ Assert(stmt->conninfo == conninfo && !OidIsValid(serverid));
values[Anum_pg_subscription_subconninfo - 1] =
- CStringGetTextDatum(conninfo);
+ CStringGetTextDatum(stmt->conninfo);
+ }
else
+ {
+ Assert(OidIsValid(serverid));
nulls[Anum_pg_subscription_subconninfo - 1] = true;
+ }
if (opts.slot_name)
values[Anum_pg_subscription_subslotname - 1] =
DirectFunctionCall1(namein, CStringGetDatum(opts.slot_name));
--
2.43.0
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-10 15:11 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2026-08-10 15:11 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
Jeff Davis <pgsql@j-davis.com> writes:
> On Sun, 2026-08-09 at 22:15 -0400, Tom Lane wrote:
>> AFAICS, it's right: if stmt->servername is set while opts.connect is
>> not, we'll arrive at this step with serverid filled in but conninfo
>> still NULL. Even if there's some upstream reason why that
>> combination can't occur, this is pretty fragile-looking code.
> The code relies on the grammar setting either stmt->servername or stmt-
> >conninfo, but not both. I agree that the control flow shouldn't rely
> on that, and the code could be more clear anyway. Patch attached.
Looks good to me, I guess we'll have to see whether it satisfies
Coverity. (But if not, we can just dismiss that complaint.)
regards, tom lane
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
@ 2026-08-11 00:26 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 27+ messages in thread
From: Jeff Davis @ 2026-08-11 00:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shlok Kyal <shlok.kyal.oss@gmail.com>; Noah Misch <noah@leadboat.com>; pgsql-hackers
On Mon, 2026-08-10 at 11:11 -0400, Tom Lane wrote:
> Looks good to me, I guess we'll have to see whether it satisfies
> Coverity. (But if not, we can just dismiss that complaint.)
Thank you. After we unfreeze, I will commit and backport to 19.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2026-08-11 00:26 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-10 19:59 CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc. Noah Misch <noah@leadboat.com>
2026-07-19 22:32 ` Jeff Davis <pgsql@j-davis.com>
2026-07-28 16:10 ` Jeff Davis <pgsql@j-davis.com>
2026-07-29 02:35 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-07-29 21:30 ` Jeff Davis <pgsql@j-davis.com>
2026-07-31 04:20 ` Jeff Davis <pgsql@j-davis.com>
2026-07-31 08:42 ` Amit Kapila <amit.kapila16@gmail.com>
2026-07-31 16:09 ` Jeff Davis <pgsql@j-davis.com>
2026-08-03 05:37 ` Amit Kapila <amit.kapila16@gmail.com>
2026-08-03 17:17 ` Jeff Davis <pgsql@j-davis.com>
2026-08-04 02:59 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-08-04 12:53 ` Amit Kapila <amit.kapila16@gmail.com>
2026-07-28 22:24 ` Jeff Davis <pgsql@j-davis.com>
2026-07-28 22:36 ` Jeff Davis <pgsql@j-davis.com>
2026-07-31 23:15 ` Jeff Davis <pgsql@j-davis.com>
2026-08-03 10:36 ` Amit Kapila <amit.kapila16@gmail.com>
2026-08-04 01:00 ` Jeff Davis <pgsql@j-davis.com>
2026-08-04 10:07 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-08-04 21:30 ` Jeff Davis <pgsql@j-davis.com>
2026-08-05 06:43 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2026-08-05 10:08 ` Shlok Kyal <shlok.kyal.oss@gmail.com>
2026-08-05 11:17 ` Amit Kapila <amit.kapila16@gmail.com>
2026-08-06 02:18 ` Jeff Davis <pgsql@j-davis.com>
2026-08-10 02:15 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-08-10 15:04 ` Jeff Davis <pgsql@j-davis.com>
2026-08-10 15:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2026-08-11 00:26 ` Jeff Davis <pgsql@j-davis.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox