On Fri, Jul 31, 2026 at 9:39 PM Jeff Davis <[email protected]> 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.
v1-0001-approach-2-Validate-publisher-for-retain_dead_tuples-in-the-.patch
Description: Binary data
