Re: Resetting recovery target parameters in pg_createsubscriber

On Mon, Sep 29, 2025 at 04:57:09PM +0700, Alena Vinter wrote:
> I got your point, thanks for pointing to the `pg_rewind` case. I've
> attached a new version of the patches. I've changed `ReplaceRecoveryConfig`
> a little bit -- now it returns false in case of an error instead of exiting.

#include "common/logging.h"
+#include "common/file_utils.h"

Incorrect include file ordering.

+GetRecoveryConfig(PGconn *pgconn, const char *target_dir)
[...]
+ char data[1024];
[...]
+ while ((bytes_read = fread(data, 1, sizeof(data), cf)) > 0)
+ {
+ data[bytes_read] = '\0';
+ appendPQExpBufferStr(contents, data);
+ }

You are assuming that this will never overflow. However, recovery
parameters could include commands, which are mostly limited to
MAXPGPATH, itself 1024. So that's unsafe. The in-core routine
pg_get_line(), or the rest in pg_get_line.c is safer to use, relying
on malloc() for the frontend and the lines fetched.

+ pg_log_warning_hint("Manual removal of recovery parameters is required from 'postgresql.auto.conf' (PostgreSQL %d+) or 'recovery.conf' (older versions)", + MINIMUM_VERSION_FOR_RECOVERY_GUC / 10000);

Hmm, okay here. You would need that hint anyway if you cannot connect
to determine to which file the recovery parameters need to go to, the
other code paths failures in ReplaceRecoveryConfig() would include the
file name, which offers a sufficient hint about the version, but a
connect_database() failure does not.

+static bool recovery_params_set = false;
+static bool recovery_params_reset = false;

Hmm. We may need an explanation about these, in the shape of a
comment, to document what's expected from them. Rather than two
booleans, using an enum tracking the state of the parameters would be
cleaner? And actually, you do not need two flags. Why not just
switch recovery_params_set to false once ReplaceRecoveryConfig() is
called?

+reset_recovery_params(const struct LogicalRepInfo *dbinfo, const char
*datadir)
[...]
+ recoveryconfcontents = GenerateRecoveryConfig(conn, NULL, NULL);

Why do we need to call again GenerateRecoveryConfig() when resetting
recovery.conf/postgresql.conf.sample with its original contents before
switching the system ID of the new replica? I may be missing
something, of course, but we're done with recovery so I don't quite
see the point in appending the recovery config generated with the
original contents. If this is justified (don't think it is), this
deserves a comment to explain the reason behind this logic.
--
Michael