Re: Add system identifier to backup manifest

On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Thu, Feb 15, 2024 at 05:41:46PM +0530, Robert Haas wrote:
> > On Thu, Feb 15, 2024 at 3:05 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > > Kindly have a look at the attached version.
> >
> > IMHO, 0001 looks fine, except probably the comment could be phrased a
> > bit more nicely.
>
> And the new option should be documented at the top of the init()
> routine for perldoc.
>

Added in the attached version.

> > That can be left for whoever commits this to
> > wordsmith. Michael, what are your plans?
>
> Not much, so feel free to not wait for me. I've just read through the
> patch because I like the idea/feature :)
>

Thank you, that helped a lot.

> 0002 seems like a reasonable approach, but there's a hunk in the wrong
> > patch: 0004 modifies pg_combinebackup's check_control_files to use
> > get_dir_controlfile() rather than git_controlfile(), but it looks to
> > me like that should be part of 0002.
>

Fixed in the attached version.

> I'm slightly concerned about 0002 that silently changes the meaning of
> get_controlfile(). That would impact extension code without people
> knowing about it when compiling, just when they run their stuff under
> 17~.
>

Agreed, now they will have an error as _could not read file "<DataDir>": Is
a
directory_. But, IIUC, that what usually happens with the dev version, and
the
extension needs to be updated for compatibility with the newer version for
the
same reason.

Regards,
Amul