Re: pg_upgrade --copy-file-range - Mailing list pgsql-hackers
From | Jakub Wartak |
---|---|
Subject | Re: pg_upgrade --copy-file-range |
Date | |
Msg-id | CAKZiRmx+ENhxPb9L0Rhwc+MgiQi=GL3SrK2cHC5s_ioB+kbu=Q@mail.gmail.com Whole thread Raw |
In response to | Re: pg_upgrade --copy-file-range (Tomas Vondra <tomas.vondra@enterprisedb.com>) |
Responses |
Re: pg_upgrade --copy-file-range
|
List | pgsql-hackers |
Hi Tomas, > I took a quick look at the remaining part adding copy_file_range to > pg_combinebackup. The patch no longer applies, so I had to rebase it. > Most of the issues were trivial, but I had to fix a couple missing > prototypes - I added them to copy_file.h/c, mostly. > > 0001 is the minimal rebase + those fixes > > 0002 has a couple review comments in copy_file, and it also undoes a lot > of unnecessary formatting changes (already pointed out by Peter a couple > days ago). > Thank you very much for this! As discussed privately, I'm not in position right now to pursue this further at this late stage (at least for v17, which would require an aggressive schedule ). My plan was more for v18 after Peter's email, due to other obligations. But if you have cycles and want to continue, please do so without hesitation - I'll try to chime in a long way to test and review for sure. > A couple review comments: > > 1) AFAIK opt_errinfo() returns pointer to the local "buf" variable. > > 2) I wonder if we even need opt_errinfo(). I'm not sure it actually > makes anything simpler. Yes, as it stands it's broken (somewhat I've missed gcc warning), should be pg_malloc(). I hardly remember, but I wanted to avoid code duplication. No strong opinion, maybe that's a different style, I'll adapt as necessary. > 3) I think it'd be nice to make CopyFileMethod more consistent with > transferMode in pg_upgrade.h (I mean, it seems wise to make the naming > more consistent, it's probably not worth unifying this somehow). > > 4) I wonder how we came up with copying the files by 50 blocks, but I > now realize it's been like this before this patch. I only noticed > because the patch adds a comment before buffer_size calculation. It looks like it was like that before pg_upgrade even was moved into the core. 400kB is indeed bit strange value, so we can leave it as it is or make the COPY_BUF_SIZ 128kb - see [1] (i've double checked cp(1) uses still 128kB today), or maybe just stick to something like 256 or 512 kBs. > 5) I dislike the renaming of copy_file_blocks to pg_copyfile. The new > name is way more generic / less descriptive - it's clear it copies the > file block by block (well, in chunks). pg_copyfile is pretty vague. > > 6) This leaves behind copy_file_copyfile, which is now unused. > > 7) The patch reworks how combinebackup deals with alternative copy > implementations - instead of setting strategy_implementation and calling > that, the decisions now happen in pg_copyfile_offload with a lot of > conditions / ifdef / defined ... I find it pretty hard to understand and > reason about. I liked the strategy_implementation approach, as it forces > us to keep each method in a separate function. Well some context (maybe it was my mistake to continue in this ./thread rather starting a new one): my plan was 3-in-1: in the original proposal (from Jan) to provide CoW as generic facility for other to use - in src/common/file_utils.c as per v3-0002-Confine-various-OS-copy-on-write-and-other-copy-a.patch - to unify & confine CoW methods and their quirkiness between pg_combinebackup and pg_upgrade and other potential CoW uses too. That was before Thomas M. pushed CoW just for pg_upgrade as d93627bcbe5001750e7611f0e637200e2d81dcff. I had this idea back then to have pg_copyfile() [normal blocks copy] and pg_copyfile_offload_supported(), pg_copyfile_offload(PG_COPYFILE_IOCTL_FICLONE , PG_COPYFILE_COPY_FILE_RANGE, PG_COPYFILE_who_has_idea_what_they_come_up_with_in_future). In Your's version of the patch it's local to pg_combinebackup, so it might make no sense after all. If you look at the pg_upgrade and pg_combinebackup they both have code duplication with lots of those ifs/IFs (assuming user wants to have it as drop-in [--clone/--copy/--copyfile] and platform may / may not have it). I've even considered --cow=ficlone|copy_file_range to sync both tools from CLI arguments point of view, but that would break backwards compatibility, so I did not do that. Also there's a problem with pg_combinebackup's strategy_implementation that it actually cannot on its own decide (I think) which CoW to use or not. There were some longer discussions that settled on one thing (for pg_upgrade): it's the user who is in control HOW the copy gets done (due to potential issues in OS CoW() implementations where e.g. if NFS would be involved on one side). See pg_upgrade --clone/--copy/--copy-file-range/--sync-method options. I wanted to stick to that, so pg_combinebackup also needs to give the same options to the user. That's was for the historical context, now you wrote "it's probably not worth unifying this somehow" few sentences earlier, so my take is the following: we can just concentrate on getting the copy_file_range() and ioctl_ficlone to pg_combinebackup at the price of duplicating some complexity for now (in short to start with clear plate , it doesn't necessary needs to be my patch as base if we think it's worthwhile for v17 - or stick to your reworked patch of mine). Later (v18?) some bigger than this refactor could unify and move the copy methods to some more central place (so then we would have sync as there would be no doubling like you mentioned e.g.: pg_upgrade's enum transferMode <-> patch enum CopyFileMethod. So for now I'm +1 to renaming all the things as you want -- indeed pg_copy* might not be a good fit in a localized version. -J. [1] - https://eklitzke.org/efficient-file-copying-on-linux
pgsql-hackers by date: