Re: Refactor pg_rewind code and make it work against a standby

On Tue, Aug 25, 2020 at 04:32:02PM +0300, Heikki Linnakangas wrote:
> On 20/08/2020 11:32, Kyotaro Horiguchi wrote:
> > 0002: Rewording that old->target and new->source makes the meaning far
> > clearer. Moving decisions core code into filemap_finalize is
> > reasonable.
> >
> > By the way, some of the rules are remaining in
> > process_source/target_file. For example, pg_wal that is a symlink,
> > or tmporary directories. and excluded files. The number of
> > excluded files doesn't seem so large so it doesn't seem that the
> > exclusion give advantage so much. They seem to me movable to
> > filemap_finalize, and we can get rid of the callbacks by doing
> > so. Is there any reason that the remaining rules should be in the
> > callbacks?
>
> Good idea! I changed the patch that way.
>
> > 0003: Thomas is propsing sort template. It could be used if committed.
> >
> > 0004:
> >
> > The names of many of the functions gets an additional word "local"
> > but I don't get the meaning clearly. but its about linguistic sense
> > and I'm not fit to that..
> > -rewind_copy_file_range(const char *path, off_t begin, off_t end, bool trunc)
> > +local_fetch_file_range(rewind_source *source, const char *path, uint64 off,
> >
> > The function actually copying the soruce range to the target file. So
> > "fetch" might give somewhat different meaning, but its about
> > linguistic (omitted..).
>
> Hmm. It is "fetching" the range from the source server, and writing it to
> the target. The term makes more sense with a libpq source. Perhaps this
> function should be called "local_copy_range" or something, but it'd also be
> nice to have "fetch" in the name because the function pointer it's assigned
> to is called "queue_fetch_range".
>
> > I'm going to continue reviewing this later.
>
> Thanks! Attached is a new set of patches. The only meaningful change is in
> the 2nd patch, which I modified per your suggestion. Also, I moved the logic
> to decide each file's fate into a new subroutine called
> decide_file_action().

The patch set fails to apply from 0002~, so this needs a rebase. I
have not looked at all that in details, but no objections to apply
0001 from me. It makes sense to move the sync subroutine for the
target folder to file_ops.c.
--
Michael