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

On 2022-Oct-06, Bharath Rupireddy wrote:

> On Thu, Oct 6, 2022 at 4:50 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > I'm doubtful it's a good idea to expose these to outside of xlog.c - they are
> > very low level, and it's very easy to break stuff by using them wrongly.
>
> 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:

+/*
+ * Get the ControlFile.
+ */
+ControlFileData *
+GetControlFile(void)
+{
+ return ControlFile;
+}

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.

+/*
+ * 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. And the comment says "it doesn't set stuff
directly", and then it goes and sets stuff directly. What gives?

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 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?

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/