On Fri, Oct 14, 2022 at 1:54 PM Alvaro Herrera <alvhe...@alvh.no-ip.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
v6-0001-Use-do_pg_abort_backup-consistently-across-the-ba.patch
Description: Binary data
v6-0002-Add-functions-for-xlogbackup.c-to-call-back-into-.patch
Description: Binary data
v6-0003-Move-backup-related-code-from-xlog.c-to-xlogbacku.patch
Description: Binary data
v6-0004-Move-backup-related-code-from-xlogfuncs.c-to-xlog.patch
Description: Binary data
