Thank for rebasing.
I didn't like 0001 very much.
* It seems now would be the time to stop pretending we're using a file
called recovery.conf; I know we still support older server versions that
use that file, but it sounds like we should take the opportunity to
rename the function to be less misleading once those versions vanish out
of existance.
How about renaming the function names to
GenerateRecoveryConf -> GenerateRecoveryConfContents
WriteRecoveryConf -> WriteRecoveryConfInfo <- it writes standby.signal on pg12+, so function name WriteRecoveryConfContents is not accurate.
and
variable writerecoveryconf -> write_recovery_conf_info?
* disconnect_and_exit seems a bit out of place compared to the other
parts of this new module. I think you only put it there so that the
'conn' can be a global, and that you can stop passing 'conn' as a
variable to GenerateRecoveryConf. It seems more modular to me to keep
it as a separate variable in each program and have it passed down to the
routine that writes the file.
* From modularity also seems better to me to avoid a global variable
'recoveryconfcontents' and instead return the string from
GenerateRecoveryConf to pass as a param to WriteRecoveryConf.
(In fact, I wonder why the current structure is like it is, namely to
have ReceiveAndUnpackTarFile write the file; why wouldn't be its caller
be responsible for writing it?)
Reasonable to make common code include less variables. I can try modifying
the patches to remove the previously added variables below in the common code.
+/* Contents of configuration file to be generated */
+extern PQExpBuffer recoveryconfcontents;
+
+extern bool writerecoveryconf;
+extern char *replication_slot;
+PGconn *conn;
I wonder about putting this new file in src/fe_utils instead of keeping
it in pg_basebackup and symlinking to pg_rewind. Maybe if we make it a
true module (recovery_config_gen.c) it makes more sense there.
I thought some about where to put the common code also. It seems pg_rewind
and pg_basebackup are the only consumers of the small common code. I doubt
it deserves a separate file under src/fe_utils.
0003:
I still don't understand why we need a command-line option to do this.
Why should it not be the default behavior?
This was discussed but frankly speaking I do not know how other postgres
users or enterprise providers handle this (probably some have own scripts?).
I could easily remove the option code if more and more people agree on that
or at least we could turn it on by default?
Thanks