Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.

From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted.
Date: 2017-11-16 01:57:43
Message-ID: CAD21AoA2jSYP561J_iKLm7bKgmkWM+crKRv+C-SHNMvWhFcM6w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Agreed. Attached the updated patch, please review it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachment Content-Type Size
fix_do_pg_abort_backup_v5.patch application/octet-stream 1.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-11-16 02:12:05 Re: Further simplification of c.h's #include section
Previous Message Andrew Dunstan 2017-11-16 01:26:35 Re: pgsql: Add hooks for session start and session end