On Thu, Sep 27, 2018 at 11:10:08PM +0200, Peter Eisentraut wrote:
> On 26/09/2018 08:44, Michael Paquier wrote:
>> Could you rebase once again? I am going through the patch and wanted to
>> test pg_upgrade on Linux with XFS, but it does not apply anymore.
>
> attached
Thanks for the rebase. At the end I got my hands on only an APFS using
a mac. I ran a test with an instance holding a database with pgbench at
scaling factor 500, which gives close to 6.5GB. The results are nice on
my laptop:
- --reflink=never runs in 15s
- --reflink=always runs in 4s
So that's a very nice gain!
+ 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().
The --help output of pg_upgrade has not been updated.
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.
After that, the implementation of cloneFile() looks logically correct to
me.
--
Michael
