On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
> Uh, is XLOGDIR the only reason to include xlog_internal.h? Maybe it
> would be easier to have a routine in xlog.c that returns the path?
> There's a few functions in xlog.c that could use it, as well.
Yep, that's all. We could also just hardcode the path as we did when
we worked on the exclusion filter lists for pg_rewind and basebackup.c,
though I'd prefer avoid that if possible.
> The patch downthread looks decent cleanup, but I'm not sure how it helps
> further the objective.
I actually think it does, because you get out of the way the conflicts
caused by RestoreArchivedFile() in the backend, as we are targetting
for fe_archive.c to be a frontend-only file, though I agree that it is
not the full of it.
> (A really good cleanup could be a situation where frontend files don't
> need xlog_internal.h -- for example, maybe a new file xlogpage.h could
> contain struct defs that relate to page and segment headers and the
> like, as well as useful macros. I don't know if this can be made to
> work -- but xlog_internal.h contains stuff like xl_parameter_change etc
> as well as RmgrData which surely are of no interest to readers of wal
> files ... or, say, RequestXLogSwitch.)
A second header that could be created is xlogpaths.h (or xlogfiles.h?)
that includes all the routines and variables we use to build WAL
segment names and such, with XLogFileNameById, IsTLHistoryFileName,
etc. I agree that splitting the record-related parts may make sense,
say xlogrecovery.h?
> I don't think any such cleanup should hamper the patch at hand anyway.
I don't think either, so I would do the split for the archive routines
at least. It still feels strange to me to have a different routine
name for the frontend-side of RestoreArchivedFile(). That's not
really consistent with the current practice we have palloc() & co, as
well as the sync routines.
--
Michael
