On Wed, Nov 15, 2017 at 5:21 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Wed, Nov 15, 2017 at 2:38 PM, Michael Paquier > <michael.paqu...@gmail.com> wrote: >> On Wed, Nov 15, 2017 at 12:12 PM, Masahiko Sawada <sawada.m...@gmail.com> >> wrote: >>> I think we need to check only sessionBackupState and don't need to >>> check XLogCtl->Insert.exclusiveBackupState in do_pg_abort_backup(). We >>> can quickly return if sessionBackupState != >>> SESSION_BACKUP_NON_EXCLUSIVE. In your suggestion, I think we can still >>> get an assertion failure when pg_stop_backup(false) waiting for >>> archiving is terminated while concurrent an exclusive backup is in >>> progress. >> >> I have just gone through the thread once again, and noticed that it is >> actually what I suggested upthread: >> https://www.postgresql.org/message-id/cab7npqtm5cdrr5y7yyfky+pvdz6dws_jkg1kstan5m95gam...@mail.gmail.com >> But your v2 posted here did not do that so it is incorrect from the start: >> https://www.postgresql.org/message-id/cad21aoa+isxyl1_zxmnk9xjhyel5h6rxjttovli7fumqfmc...@mail.gmail.com > > Sorry, it's my fault. I didn't mean it but I forgot.
My review was wrong as well :) >> We both got a bit confused here. As do_pg_abort_backup() is only used >> for non-exclusive backups (including those taken through the >> replication protocol), going through the session lock for checks is >> fine. Could you update your patch accordingly please? > > One question is, since we need to check only the session lock I think > that the following change is not necessary. Even if calling > CHECK_FOR_INTERRUPTS after set sessionBackupState = > SESSION_BACKUP_EXCLUSIVE; we never call do_pg_abort_backup(). Is that > right? Yeah, this one is not strictly necessary for this bug, but it seems to me that it would be a good idea for robustness wiht interrupts to be consistent with the stop phase when updating the session lock. -- Michael
