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-15 08:21:52 |
Message-ID: | CAD21AoBhDEgmCOvjBdwvrcp9HrMSYm9vuJwvQn7JS3OeDkL55g@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> 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?
@@ -10636,8 +10636,14 @@ do_pg_start_backup(const char *backupidstr,
bool fast, TimeLineID *starttli_p,
{
WALInsertLockAcquireExclusive();
XLogCtl->Insert.exclusiveBackupState =
EXCLUSIVE_BACKUP_IN_PROGRESS;
- WALInsertLockRelease();
+
+ /*
+ * Clean up session-level lock. To avoid calling
CHECK_FOR_INTERRUPTS
+ * by LWLockReleaseClearVar before changing the backup
state we change
+ * it while holding the WAL insert lock.
+ */
sessionBackupState = SESSION_BACKUP_EXCLUSIVE;
+ WALInsertLockRelease();
}
else
sessionBackupState = SESSION_BACKUP_NON_EXCLUSIVE;
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2017-11-15 08:39:27 | Re: [HACKERS] Assertion failure when the non-exclusive pg_stop_backup aborted. |
Previous Message | Michael Paquier | 2017-11-15 08:20:59 | Re: [HACKERS] Simplify ACL handling for large objects and removal of superuser() checks |