On 19/10/2018 01:50, Michael Paquier wrote: > On Thu, Oct 18, 2018 at 11:59:00PM +0200, Peter Eisentraut wrote: >> New patch that removes all the various reflink modes and adds a new >> option --clone that works similar to --link. I think it's much cleaner now. > > Thanks Peter for the new version. > > + > + {"clone", no_argument, NULL, 1}, > + > {NULL, 0, NULL, 0} > > Why newlines here?
fixed > @@ -293,6 +300,7 @@ usage(void) > printf(_(" -U, --username=NAME cluster superuser (default > \"%s\")\n"), os_info.user); > printf(_(" -v, --verbose enable verbose internal > logging\n")); > printf(_(" -V, --version display version information, > then exit\n")); > + printf(_(" --clone clone instead of copying > files to new cluster\n")); > printf(_(" -?, --help show this help, then > exit\n")); > printf(_("\n" > > An idea for a one-letter option could be -n. This patch can live > without. -n is often used for something like "dry run", so it didn't go for that here. I suspect the cloning will remain a marginal option for some time, so having only a long option is acceptable. > + pg_fatal("error while cloning relation \"%s.%s\": could not open file > \"%s\": %s\n", > + schemaName, relName, src, strerror(errno)); > > The tail of those error messages "could not open file" and "could not > create file" are already available as translatable error messages. > Would it be better to split those messages in two for translators? One > is generated with pg_fatal("error while cloning relation \"%s.%s\": > could not open file \"%s\": %s\n", > + schemaName, relName, src, strerror(errno)); I think this is too complicated for a few messages. > Those are all minor points, the patch looks good to me. Committed, thanks. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services