On Thu, Oct 13, 2022 at 4:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
Thanks for reviewing.
> > 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.
Moved them to xlog_internal.h without xlogbackup.h included, please see below.
> > 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.
It's not required now, please see below.
> 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.
The pg_backup_start_callback() can just go ahead and reset
sessionBackupState. However, it leads us to the complete removal of
pg_backup_start_callback() itself and use do_pg_abort_backup()
consistently across, saving 20 LOC attached as v5-0001.
With this, the other patches would get simplified a bit too,
xlogbackup.h footprint got reduced now.
Please find the v5 patch-set. 0002-0004 moves the backup code to
xlogbackup.c/.h.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
