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

On Fri, Jul 31, 2026 at 9:39 PM Jeff Davis <pgsql(at)j-davis(dot)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.