Re: Resetting recovery target parameters in pg_createsubscriber

On Tue, Sep 23, 2025 at 12:04:04PM +0700, Alena Vinter wrote:
> About the recovery parameters cleanup: I thought about adding an exit
> callback, but it doesn't really make sense because once the target server
> gets promoted (which happens soon after we set the parameters), there's no
> point in cleaning up - the server is already promoted and can't be used as
> a replica again and must be recreated. Also, `reset_recovery_params()`
> might call `exit()` itself, which could cause problems with the cleanup
> callback.

Your argument does not consider one case, which is very common:
pg_rewind. Even if the standby finishes recovery and is promoted with
its new recovery parameters, we could rewind it rather than recreate a
new standby from scratch. That's cheaper than recreating a new
physical replica from scratch. Keeping the recovery parameters added
by pg_createsubscriber around would make pg_rewind's work more
complicated, because it does similar manipulations, for different
requirements.

The tipping point where we would not be able to reuse the promoted
standby happens as the last step of pg_createsuscriber in
modify_subscriber_sysid() where its system ID is changed. Before
that, the code also makes an effort of cleaning up anything that's
been created in-betwee.

Even the system ID argument is not entirely true, actually. One could
also decide to switch the system ID back to what it was previously to
match with the primary. That requires a bit more magic, but that's
not impossible.

> So I think it's better to just warn users about leftover parameters and let
> them handle the cleanup manually if needed.

Warnings tend to be ignored and missed, especially these days where
vendors automate these actions. It is true that there could be an
argument about requiring extra implementation steps on each vendor
side, but they would also need to keep up with any new GUCs that
pg_createsubscriber may add in the future when setting up its recovery
parameters, which would mean extra work for everybody, increasing the
range of problems for some logic that's isolated to
pg_createsubscriber.

In short, I disagree with what you are doing here: we should take the
extra step and clean up anything that's been created by the tool when
we know we can safely do so (aka adding a static flag that the
existing cleanup callback should rely on, which is already what your
patch 0003 does to show a warning).

> By the way, is it ok that the second patch includes both code and test
> changes together, or should I split them into separate commits?

The tests and the fix touch entirely separate code paths, keeping them
together is no big deal.
--
Michael