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
signature.asc
Description: PGP signature