Re: Add system identifier to backup manifest - Mailing list pgsql-hackers

From Robert Haas
Subject Re: Add system identifier to backup manifest
Date
Msg-id CA+TgmoasZBbHDFK-FPKpz4mpMOxb3shWLsukQ+v4xQ_CxifOKA@mail.gmail.com
Whole thread Raw
In response to Re: Add system identifier to backup manifest  (Amul Sul <sulamul@gmail.com>)
Responses Re: Add system identifier to backup manifest
List pgsql-hackers
On Mon, Mar 4, 2024 at 12:35 AM Amul Sul <sulamul@gmail.com> wrote:
> Yes, you are correct. Both the current caller of get_controlfile() has
> access to the root directory.
>
> I have dropped the 0002 patch -- I don't have a very strong opinion to refactor
> get_controlfile() apart from saying that it might be good to have both versions :) .

I don't have an enormously strong opinion on what the right thing to
do is here either, but I am not convinced that the change proposed by
Michael is an improvement. After all, that leaves us with the
situation where we know the path to the control file in three
different places. First, verify_backup_file() does a strcmp() against
the string "global/pg_control" to decide whether to call
verify_backup_file(). Then, verify_system_identifier() uses that
string to construct a pathname to the file that it will be read. Then,
get_controlfile() reconstructs the same pathname using it's own logic.
That's all pretty disagreeable. Hard-coded constants are hard to avoid
completely, but here it looks an awful lot like we're trying to
hardcode the same constant into as many different places as we can.
The now-dropped patch seems like an effort to avoid this, and while
it's possible that it wasn't the best way to avoid this, I still think
avoiding it somehow is probably the right idea.

I get a compiler warning with 0002, too:

../pgsql/src/backend/backup/basebackup_incremental.c:960:22: warning:
call to undeclared function 'GetSystemIdentifier'; ISO C99 and later
do not support implicit function declarations
[-Wimplicit-function-declaration]
        system_identifier = GetSystemIdentifier();
                            ^
1 warning generated.

But I've committed 0001.

--
Robert Haas
EDB: http://www.enterprisedb.com



pgsql-hackers by date:

Previous
From: Michael Banck
Date:
Subject: Re: [PATCH] Exponential backoff for auth_delay
Next
From: Tom Lane
Date:
Subject: Re: postgres_fdw fails to see that array type belongs to extension