[Alvaro Herrera <alvherre@alvh.no-ip.org>]

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/