On Wed, Jan 11, 2023 at 11:29:01AM -0800, Nathan Bossart wrote:
> I'm having trouble thinking of any practical advantage of providing the
> redo LSN and TLI. If the main use-case is removing older archives as the
> documentation indicates, it seems better to provide the file name so that
> you can plug it straight into strcmp() to determine whether the file can be
> removed (i.e., what pg_archivecleanup does). If we provided the LSN and
> TLI instead, you'd either need to convert that into a WAL file name for
> strcmp(), or you'd need to convert the candidate file name into an LSN and
> TLI and compare against those.
Logging was one thing that came immediately in mind, to let the module
know the redo LSN and TLI the segment name was built from without
having to recalculate it back. If you don't feel strongly about that,
I am fine to discard this remark. It is not like this hook should be
set in stone across major releases, in any case.
> I initially created a separate basic_restore module, but decided to fold it
> into basic_archive to simplify the patch and tests. I hesitated to rename
> it because it already exists in v15, and since it deals with creating and
> restoring archive files, the name still seemed somewhat accurate. That
> being said, I don't mind renaming it if that's what folks want.
I've done that in the past for pg_verify_checksums -> pg_checksums, so
I would not mind renaming it so as it reflects better its role.
(Being outvoted is fine for me if this suggestion sounds bad).
Saying that, 0001 seems fine on its own (minus the redo LSN/TLI with
the duplication for the segment name build), so I would be tempted to
get this one done. My gut tells me that we'd better remove the
duplication and just pass down the two fields to
shell_archive_cleanup() and shell_recovery_end(), with the segment
name given to ExecuteRecoveryCommand()..
--
Michael