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