Feedback-ID: ia2694551:Fastmail
Date: Thu, 13 Oct 2022 13:13:30 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael@paquier.xyz>,
	Nathan Bossart <nathandbossart@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Move backup-related code to xlogbackup.c/.h
Message-ID: <20221013111330.564fk5tkwe3ha77l@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALj2ACVtyrWO+L-8MtzRVO_5yLgTGZqVur+D1+7T2ebgaT5Z5w@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221013111330.564fk5tkwe3ha77l%40alvherre.pgsql>
Precedence: bulk

On 2022-Oct-13, Bharath Rupireddy wrote:

> Hm. Agree. But, that requires us to include xlogbackup.h in
> xlog_internal.h for SessionBackupState enum in
> ResetXLogBackupActivity(). Is that okay?

It's not great, but it's not *that* bad, ISTM, mainly because
xlog_internal.h will affect less stuff than xlog.h.

> SessionBackupState and it needs to be set before we release WAL insert
> locks, see the comment [1].

I see.  Maybe we could keep that enum in xlog.h, instead.

While looking at how that works: I think calling a local variable
"session_backup_state" is super confusing, seeing that we have a
file-global variable called sessionBackupState.  I recommend naming the
local "newstate" or something along those lines instead.

I wonder why does pg_backup_start_callback() not change the backup state
before your patch.  This seems a gratuitous difference, or is it?  If
you change that code so that it also sets the status to BACKUP_NONE,
then you can pass a bare SessionBackupState to ResetXLogBackupActivity
rather than a pointer to one, which is a very strange arrangement that
exists only so that you can have a third state (NULL) meaning "don't
change state" -- that looks quite weird.

Alternatively, if you don't want or can't change
pg_backup_start_callback to pass a valid state value, another solution
might be to pass a separate boolean "change state".

But I would look at having another patch before your series that changes
pg_backup_start_callback to make the code identical for the three
callers, then you can simplify the patched code.

> 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. Do you
> have any thoughts on this?

No, please, no passing of unadorned magic numbers.

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