On Mon, Oct 24, 2022 at 1:00 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Oct 19, 2022 at 09:07:04PM +0530, Bharath Rupireddy wrote:
> > XLogBackupResetRunning() seemed better. +1 for above function names.
>
> I see what you are doing here. XLogCtl would still live in xlog.c,
> but we want to have functions that are able to manipulate some of its
> fields.
Right.
> I am not sure to like that much because it introduces a
> circling dependency between xlog.c and xlogbackup.c. As of HEAD,
> xlog.c calls build_backup_content() from xlogbackup.c, which is fine
> as xlog.c is kind of a central piece that feeds on the insert and
> recovery pieces. However your patch makes some code paths of
> xlogbackup.c call routines from xlog.c, and I don't think that we
> should do that.
If you're talking about header file dependency, there's already header
file dependency between them - xlog.c includes xlogbackup.h for
build_backup_content() and xlogbackup.c includes xlog.h for
wal_segment_size. And, I think the same kind of dependency exists
between xlog.c and xlogrecovery.c.
Please note that we're trying to reduce xlog.c file size apart from
centralizing backup related code.
> > I'm okay either way.
> >
> > Please see the attached v8 patch set.
>
> Among all that, CleanupBackupHistory() is different, still it has a
> dependency with some of the archiving pieces..
Is there a problem with that? This function is used solely by backup
functions and it happens to use one of the archiving utility
functions. Please see the other archiving utility functions being used
elsewhere in the code, not only in xlog.c -
for instance, KeepFileRestoredFromArchive() and XLogArchiveNotify().
I'm attaching the v9 patch set herewith after rebasing. Please review
it further.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
