Re: Resetting recovery target parameters in pg_createsubscriber

On Mon, Oct 06, 2025 at 01:25:12PM +0700, Alena Vinter wrote:
> Michael, thanks for helping! This fact simplifies the code. I put resetting
> the parameters exclusively in the `atexit` callback -- this approach seems
> neater to me. What do you think?

I have been looking at this patch for a couple of hours, and I don't
really like the result, for a variety of reasons.  Some of the reasons
come with the changes in recovery_gen.c themselves, as proposed in the
patch, where the only thing we want to do it replace the contents of
one file by the other, some other reasons come from the way
pg_createsubscriber complicates its life on HEAD.  There is no need to
read the contents line by line and write them back, we can just do
file manipulations.

The reason why the patch does things this way currently is that it has
zero knowledge of the file location where the recovery parameters
contents are written, because this location is internal in
recovery_gen.c, at least based on how pg_createsubscriber is written.
And well, this fact is wrong even on HEAD: we know where the recovery
parameters are written because pg_createsubscriber is documented as
only supporting the same major version as the one where the tool has
been compiled.  So it is pointless to call WriteRecoveryConfig() with
a connection object (using a PGconn pointer in this API is an artifact
of pg_basebackup, where we support base backups taken from older major
versions when using a newer version of the tool).  pg_createsubscriber
has no need to bind to this limitation, but we don't need to improve
this point for the sake of this thread.

The proposed patch is written without taking into account this issue,
and the patch has a lot of logic that's not necessary.  There is no
point in referring to recovery.conf in the code and the tests, as
well.

Anyway, a second reason why I am not cool with the patch is that the
contents written by pg_createsubscriber are entirely erased from
existence, and I see a good point in keeping a trace of them at least
for post-operation debugging purposes.  With all that in mind, I came
up with the following solution, which is able to fix what you want to
address (aka not load any of the recovery parameters written by the
tool if you reactivate a standby with a new signal file), while also
satisfying my condition, which is to keep a track of the parameters
written.  Hence, let's:
- forget about the changes in recovery_gen.c.
- call WriteRecoveryConfig() with only one line added in the contents
written to the "recovery" file (which is postgresql.conf.auto, okay):
include_if_exists = 'pg_createsubscriber.conf'
- Write the parameters generated by pg_createsubscriber to this new
configuration file.
- In the exit callback, call durable_rename() and rename
pg_createsubscriber.conf to a pg_createsubscriber.conf.old.  There is
no need to cache the backend version or rely on a connection.  We'll
unlikely see a failure.  Even if there is a failure, fixing the
problem would be just to move or delete the extra file, and
documenting that is simpler.

All that points to the direction that we may not want to backpatch any
of this, considering these changes as improvements in usability.
--
Michael