Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

>
> 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