Re: file cloning in pg_upgrade and CREATE DATABASE

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