On 2019-Sep-09, Paul Guo wrote: > > > > 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.
GenerateRecoveryConfig / WriteRecoveryConfig ? > > 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. Hmm, but other things there are also used by only two programs, say psqlscan.l and conditional.c are just for psql and pgbench. > > 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? Well, I've seen no contrary votes, and frankly I see no use for the opposite (current) behavior. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services