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

From: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, 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-12-11 05:03:23
Message-ID: CAD21AoAj+gtSaespqaMLZ5kfJm0h=Oap8WnPFWXCcbcvvr84zQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Sat, Dec 9, 2017 at 2:24 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Dec 8, 2017 at 4:13 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Fri, Dec 8, 2017 at 5:38 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>> After off-discussion with Fujii-san, I've updated the comment of why
>>> we should disallow interrupts before setting/cleanup the session-level
>>> lock. Please review it.
>>
>> + /*
>> + * Set session-level lock. If we allow interrupts before setting
>> + * session-level lock, we could call callbacks with an inconsistent
>> + * state. To avoid calling CHECK_FOR_INTERRUPTS by LWLockReleaseClearVar
>> + * which is called by WALInsertLockRelease before changing the backup
>> + * state we change it while holding the WAL insert lock.
>> + */
>> So you are just adding the reference to WALInsertLockRelease.. Instead
>> of writing the function names for LWLocks,

I also added a sentence "If we allow interrupts before cleanup
session-level lock, we could call do_pg_abort_backup with an
inconsistent state" at two places: setting and cleanup session-level
lock.

>> I would just write "To
>> avoid calling CHECK_FOR_INTERRUPTS which can happen when releasing a
>> LWLock" and be done with it. There is no point to list a full function
>> dependency list, which could change in the future with static routines
>> of lwlock.c.

Agreed. Updated the comment.

>
> I think it's actually good to be explicit here. I looked at this
> patch a bit last week and had great difficulty understanding how the
> CHECK_FOR_INTERRUPTS() could happen.
>

Attached the updated version patch.

Regards,

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

Attachment Content-Type Size
fix_do_pg_abort_backup_v11.patch application/octet-stream 3.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-12-11 05:13:21 Re: BUG #14941: Vacuum crashes
Previous Message Masahiko Sawada 2017-12-11 04:54:35 Re: [HACKERS] What does it mean by XLOG_BACKUP_RECORD?