On 28/09/2018 07:19, Michael Paquier wrote:
> + static bool cloning_ok = true;
> +
> + pg_log(PG_VERBOSE, "copying \"%s\" to \"%s\"\n",
> + old_file, new_file);
> + if (cloning_ok &&
> + !cloneFile(old_file, new_file, map->nspname, map->relname, true))
> + {
> + pg_log(PG_VERBOSE, "cloning not supported, switching to copying\n");
> + cloning_ok = false;
> + copyFile(old_file, new_file, map->nspname, map->relname);
> + }
> + else
> + copyFile(old_file, new_file, map->nspname, map->relname);
>
> This part overlaps with the job that check_reflink() already does.
> Wouldn't it be more simple to have check_reflink do a one-time check
> with the automatic mode, enforcing to REFLINK_NEVER if cloning test did
> not pass when REFLINK_AUTO is used? This would simplify
> transfer_relfile().
I'll look into that.
> The --help output of pg_upgrade has not been updated.
will fix
> I am not a fan of the --reflink interface to be honest, even if this
> maps to what cp offers, particularly because there is already the --link
> mode, and that the new option interacts with it. Here is an idea of
> interface with an option named, named say, --transfer-method:
> - "link", maps to the existing --link, which is just kept as a
> deprecated alias.
> - "clone" is the new mode you propose.
> - "copy" is the default, and copies directly files. This automatic mode
> also makes the implementation around transfer_relfile more difficult to
> apprehend in my opinion, and I would think that all the different
> transfer modes ought to be maintained within it. pg_upgrade.h also has
> logic for such transfer modes.
I can see the argument for that. But I don't understand where the
automatic mode fits into this. I would like to keep all three modes
from my patch: copy, clone-if-possible, clone-or-fail, unless you want
to argue against that.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
