CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.

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

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:

/messages/by-id/CAHGQGwFGa6+wWVgUmZPFwN=fBY59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail.com
/messages/by-id/OS9PR01MB12149C3ED34272966B25DB173F5C12@OS9PR01MB12149.jpnprd01.prod.outlook.com

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

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

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

Dear Jeff,

There are two other threads discussing closely-related problems:

/messages/by-id/CAHGQGwFGa6+wWVgUmZPFwN=fB
Y59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail.com
/messages/by-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]: /messages/by-id/CANhcyEU9VsaLwo908ws_1MxNB79f+cr-JVfig=Zoaf4+KQe+GQ@mail.gmail.com

Best regards,
Hayato Kuroda
FUJITSU LIMITED

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]/messages/by-id/OS9PR01MB121493DA4C1A7748B11A646D8F5C02@OS9PR01MB12149.jpnprd01.prod.outlook.com, we could tighten the checks. But I'm not sure we want
to do that for v19.

Regards,
Jeff Davis

[1]: /messages/by-id/OS9PR01MB121493DA4C1A7748B11A646D8F5C02@OS9PR01MB12149.jpnprd01.prod.outlook.com
/messages/by-id/OS9PR01MB121493DA4C1A7748B11A646D8F5C02@OS9PR01MB12149.jpnprd01.prod.outlook.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

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.

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

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

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.

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.