From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(at)paquier(dot)xyz>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Move backup-related code to xlogbackup.c/.h |
Date: | 2022-10-14 13:21:05 |
Message-ID: | CALj2ACWEkVvCTk2ebYkwEbGG2CjzpbF5ARQYU8eGNTotFedFHw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
v6-0001-Use-do_pg_abort_backup-consistently-across-the-ba.patch | application/octet-stream | 4.3 KB |
v6-0002-Add-functions-for-xlogbackup.c-to-call-back-into-.patch | application/octet-stream | 10.1 KB |
v6-0003-Move-backup-related-code-from-xlog.c-to-xlogbacku.patch | application/octet-stream | 51.1 KB |
v6-0004-Move-backup-related-code-from-xlogfuncs.c-to-xlog.patch | application/octet-stream | 10.5 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dagfinn Ilmari Mannsåker | 2022-10-14 14:31:26 | Re: [PATCH] Improve tab completion for ALTER TABLE on identity columns |
Previous Message | Richard Guo | 2022-10-14 13:07:22 | Re: Fix error message for MERGE foreign tables |