On Wed, Mar 20, 2019 at 01:55:51PM +0800, Andrey Borodin wrote: > I'm a bit confused by by console output routines. E.g. in > pg_rewind's main() you call pg_fatal()s, and printf(), and pg_log() > with various levels. Shouldn't we use all the pg_* functions?
pg_fatal() would exit immediately, and sometimes the error code paths want to have multiple lines, which is why printf() gets used. > But most of this printing usages were there before your patch. > > I'm marking the patch as RFC, since I have no more notices and patch > really looks good. That does not look fully baked yet, at least in my opinion. + * This is a simplified and adapted to frontend version of + * RestoreArchivedFile function from transam/xlogarchive.c + */ +static int +RestoreArchivedWAL(const char *path, const char *xlogfname, I don't think that we should have duplicates for that, so I would recommend refactoring the code so as a unique code path is taken by both, especially since the user can fetch the command from postgresql.conf. Why two options? Wouldn't actually be enough use-postgresql-conf to do the job? Note that "postgres" should always be installed if pg_rewind is present because it is a backend-side utility, so while I don't like adding a dependency to other binaries in one binary, having an option to pass out a command directly via the command line of pg_rewind stresses me more. Don't we need to worry about signals interrupting the restore command? It seems to me that some refactoring from the stuff in xlogarchive.c would be in order. -- Michael
signature.asc
Description: PGP signature
