Re: file cloning in pg_upgrade and CREATE DATABASE

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