On Mon, Sep 15, 2025 at 10:29:47AM +0300, Alexander Korotkov wrote: > I went though this patches. > 1) I've removed the array of parameters. I see it was proposed by > Michael upthread. But I think his proposal came from the fact we walk > trough the parameters twice. But we end up walking trough the > parameter once in setup_recovery(), while reset_recovery_params() just > restores the previous contents. I think it makes sense to keep the > changes minimal.
Yeah, my concern was about the duplication of the list. As long as a
fix does not do any of that, I'm OK. Sorry if my idea of a list of
parameters felt misguided if we make recovery_gen.c smarter with the
handling of the on-disk files.
> 2) I reordered patches so that helper function goes first. I think it
> essential to order commit in the way that every commit leaves our tree
> in working state.
Yep. That would create some noise if one bisects for example. These
are always annoying because they make analysis of a range of commits
longer with more false positives. If you have a large range of
commits, the odds are usually very low, but who knows..
> 3) I make pgpreltidy run over 040_pg_createsubscriber.pl.
> Any thought?
GetRecoveryConfig() and ReplaceRecoveryConfig() should have some
documentation, regarding what the callers of these functions can
expect from them.
+ use_recovery_conf =
+ PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_RECOVERY_GUC;
+
+ snprintf(tmp_filename, MAXPGPATH, "%s/%s.tmp", target_dir,
+ use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf");
+
+ snprintf(filename, MAXPGPATH, "%s/%s", target_dir,
+ use_recovery_conf ? "recovery.conf" : "postgresql.auto.conf"
No need for use_recovery_conf. You could just set a pointer to the
file name instead and avoid the duplication.
+ cf = fopen(tmp_filename, "w");
+ if (cf == NULL)
+ pg_fatal("could not open file \"%s\": %m", tmp_filename);
"a" is used in fopen() when calling WriteRecoveryConfig() when under
use_recovery_conf. Perhaps this inconsistency should be specified as
a comment because we are generating a temporary file from scratch with
the new recovery GUC contents?
This patch also means that pg_createsubscriber is written so as the
contents added to recovery.conf/postgresql.auto.conf by
setup_recovery() are never reset if there is a failure in-flight. Is
that OK or should we also have an exit callback to do the cleanup work
in such cases?
Perhaps these internal manipulations should be documented as well, to
make the users of this tool aware of steps they may need to take in
the event of an in-flight failure? pg_createsubscriber includes a
"How it works" section that explains how the tool works, including the
part about the recovery parameters. The changes of this patch become
implied facts, and are not reflected in the docs. That sounds like a
problem to me because we are hiding some of the the internal logic,
but the docs are written so as they explain all these details.
--
Michael
signature.asc
Description: PGP signature
