Re: Move backup-related code to xlogbackup.c/.h

On Wed, Oct 12, 2022 at 1:04 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> > Hm. Here's the v3 patch set without exposing WAL insert lock related
> > functions. Please have a look.
>
> Hmm, I don't like your 0001 very much. This sort of thing:

Thanks for reviewing.

> +ControlFileData *
> +GetControlFile(void)
>
> looks too easy to misuse; what about locking? Also, isn't the addition
> of ControlFile as a variable in do_pg_backup_start going to cause shadow
> variable warnings? Given the locking requirements, I think it would be
> feasible to copy stuff out of ControlFile under lock, then return the
> copies.

+1. Done that way.

> +/*
> + * Increment runningBackups and forcePageWrites.
> + *
> + * NOTE: This function is tailor-made for use in xlogbackup.c. It doesn't set
> + * the respective XLogCtl members directly, and acquires and releases locks.
> + * Hence be careful when using it elsewhere.
> + */
> +void
> +SetXLogBackupRelatedInfo(void)
>
> I understand that naming is difficult, but I think "Set foo Related
> Info" seems way too vague.

I've used SetXLogBackupActivity() and ResetXLogBackupActivity()
because they match with the members that these functions deal with.

> And the comment says "it doesn't set stuff
> directly", and then it goes and sets stuff directly. What gives?

My bad. That comment was meant for the reset function above. However,
I've removed it entirely now because one can look at the function and
infer that the forcePageWrites isn't set directly but only when
runningBackups is 0.

> You added some commentary that these functions are tailor-made for
> internal operations, and then declared them in the most public header
> function that xlog has? I think at the bare minimum, these prototypes
> should be in xlog_internal.h, not xlog.h.

I removed such comments. These are functions used by xlogbackup.c to
call back into xlog.c similar to the call back functions defined in
xlog.h for xlogrecovery.c. And, most of the XLogCtl set/get sort of
function declarations are in xlog.h. So, I'm retaining them in xlog.h.

> I didn't look at 0002 and 0003 other than to notice that xlogbackup.h is
> no longer removed from xlog.h. So what is the point of all this?

The whole idea is to move as much as possible backup related code to
xlogbackup.c/.h because xlog.c has already grown.

I've earlier moved macros BACKUP_LABEL_FILE, TABLESPACE_MAP to
xlogbackup.h, but I think they're good to stay in xlog.h as they're
being used in xlog.c and xlogrecovery.c. This reduces the xlogbackup.h
footprint a bit - we don't need xlogbackup.h in xlogrecovery.c.

Another reason we need xlogbackup.h in xlog.h is for
SessionBackupState and it needs to be set before we release WAL insert
locks, see the comment [1]. Well, for this reason, should we move all
xlogbackup.c callbacks for xlog.c to xlog_internal.h? Or should we
just remove the SessionBackupState enum and convert
SESSION_BACKUP_NONE and SESSION_BACKUP_RUNNING to just macros in
xlogbackup.h and use integer type to pass the state across? I don't
know what's better here. Thoughts?

I'm attaching the v4 patch set, please review it further.

[1]
* You might think that WALInsertLockRelease() can be called before
* cleaning up session-level lock because session-level lock doesn't need
* to be protected with WAL insertion lock. But since
* CHECK_FOR_INTERRUPTS() can occur in it, session-level lock must be
* cleaned up before it.
*/

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com