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

On Fri, Oct 14, 2022 at 1:54 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2022-Oct-13, Bharath Rupireddy wrote:
>
> > 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.
>
> OK, that's not bad -- but there is a fatal flaw here: do_pg_backup_start
> only sets sessionBackupState *after* it has finished setting things up,
> so if you only change it like this, do_pg_abort_backup will indeed run,
> but it'll do nothing because it hits the "quick exit" test. Therefore,
> if a backup aborts while setting up, you'll keep running with forced
> page writes until next postmaster crash or restart. Not good.

Ugh.

> ISTM we need to give another flag to the callback function besides
> emit_warning: one that says whether to test sessionBackupState or not.

I think this needs a new structure, something like below, which makes
things complex.
typedef struct pg_abort_backup_params
{
/* This tells whether or not the do_pg_abort_backup callback can
quickly exit. */
bool can_quick_exit;
/* This tells whether or not the do_pg_abort_backup callback can
emit a warning. */
bool emit_warning;
} pg_abort_backup_params;

> I suppose the easiest way to do it with no other changes is to turn
> 'arg' into a bitmask.

This one too isn't good IMO.

> But alternatively, we could just remove emit_warning as a flag and have
> the warning be emitted always; then we can use the boolean for the other
> purpose. I don't think the extra WARNING thrown during backup set-up is
> going to be a problem, since it will mostly never be seen anyway (and if
> you do see it, it's not a lie.)

+1 for this.

Please review the v6 patch-set further.

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