On Wed, Nov 15, 2017 at 5:21 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Wed, Nov 15, 2017 at 2:38 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Nov 15, 2017 at 12:12 PM, Masahiko Sawada <sawada.mshk@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_jKG1KStaN5m95gAMTFQ@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_ZXMnk9xJhYEL5h6rxJtTovLi7fumqfmCYgg@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
