Re: Add system identifier to backup manifest

On Mon, Mar 4, 2024 at 2:47 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 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.

So with that in mind, here's my proposal. This is an adjustment of
Amit's previous refactoring patch. He renamed the existing
get_controlfile() to get_dir_controlfile() and made a new
get_controlfile() that accepted the path to the control file itself. I
chose to instead leave the existing get_controlfile() alone and add a
new get_controlfile_by_exact_path(). I think this is better, because
most of the existing callers find it more convenient to pass the path
to the data directory rather than the path to the controlfile, so the
patch is smaller this way, and less prone to cause headaches for
people back-patching or maintaining out-of-core code. But it still
gives us a way to avoid repeatedly constructing the same pathname.

If nobody objects, I plan to commit this version.

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