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