Subject: Re: Backend crash on non-exclusive backup cancel

From: Michael Paquier <[email-masked]>
Date: Wed, 1 Mar 2017 11:08:55 +0900

On Tue, Feb 28, 2017 at 10:21 PM, David Steele <[email-masked]> wrote:
> On 2/27/17 10:05 PM, Michael Paquier wrote:
>> That's not necessarily true, I can see a stop backup able to finish as
>> well by issuing a cancel request. It seems to me that we just need to
>> have the shmem information updated at the same time as the
>> session-level switches for consistency and we're good. The
>> inconsistency in places when updating the session-level flags and the
>> shmem-level flags is what is causing harm.
>
> I'm sure this could be done but it will require quite a bit of
> refactoring and I'm not sure that it's worth it.  In my mind it would be
> enough to document that cancelled backups should be considered invalid
> whether the stop backup record was written or not.  However, I'm willing
> to go with the majority opinion.

I would think that addressing the problem is the way to go, hitting an
assertion in this code path means that we are doing something wrong so
simply removing it does not sound like a correct answer to me.

>>> If that makes sense I'm happy to work up a patch.  This is definitely an
>>> edge case and I seriously doubt it is causing any issues in the field.
>>
>> <...>
>>
>> David, are you willing to write a patch or should I? Changing
>> nonexclusive_backup_running/exclusive_backup_running to be an enum
>> would make the code more readable as well IMO.
>
> I would like to see if anyone else weighs in on this first, but yes I am
> planning to write a patch.  Agreed on the enum.

This was itching me yesterday so I wrote a patch able to address this
problem... At least it can be registered to the CF on time and give it
more visibility. By the way, I have noticed a second bug in the
current logic while looking at the problem you have reported:
1) Start backup in session 1:
=# select pg_start_backup('popo');
 pg_start_backup
-----------------
 0/2000028
(1 row)
2) pg_stop_backup() in session 2
3) And now when trying to work on backups with session 1:
=# select pg_start_backup('popo');
ERROR:  55000: a backup is already in progress in this session
LOCATION:  pg_start_backup, xlogfuncs.c:87
=# select pg_stop_backup();
ERROR:  55000: exclusive backup not in progress
LOCATION:  do_pg_stop_backup, xlog.c:10642

So the handling around exclusive_backup_running is broken now as it is
possible to lock a session easily when handling backups. And the root
of the problem is that checks on exclusive_backup_running are not
necessary. I have fixed as well this problem as per the attached. Note
however that SESSION_BACKUP_EXCLUSIVE still makes sense in the patch
attached when doing checks with pg_stop_backup_v2() for a
non-exclusive backup run. It is also useful for debugging.
-- 
Michael

-- 
Sent via pgsql-bugs mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

---

From: David Steele <[email-masked]>
Date: Tue, 28 Feb 2017 22:44:53 -0500

On 2/28/17 9:08 PM, Michael Paquier wrote:
> problem... 

I know that feeling.

> At least it can be registered to the CF on time and give it
> more visibility. 

Indeed.

> By the way, I have noticed a second bug in the

Yeah, that doesn't look good.

> non-exclusive backup run. It is also useful for debugging.

Excellent! I'll have a look, and thanks for working up a patch.

-- 
-David
[email-masked]

---

From: Anastasia Lubennikova <[email-masked]>
Date: Tue, 14 Mar 2017 15:27:14 +0000

As far as I understand, in this thread were discussed two bugs of pg_stop_backup().
Thanks to the clear descriptions above, I easily reproduced both of them.

BUG#1:
Server crashes on assertion on call of pg_stop_backup(false) after interrupted call of pg_stop_backup(false).
TRAP: FailedAssertion("!(XLogCtl->Insert.nonExclusiveBackups > 0)", File: "xlog.c", Line: 10747)

BUG#2:
Failure to start an exclusive backup with the same name, if previous exclusive backup was stopped in another session.

Both problems seem to be fixed with patch "backup-session-locks-fixes.patch".
Speaking of the patch itself, I have a question: shouldn't we also update sessionBackupState
in pg_stop_backup_callback() along with XLogCtl->Insert.exclusiveBackupState?

And couple of minor notes:
1) + * Routines to starting stop, and get status of a base backup
Probably should be: + * Routines to start, stop and get status of a base backup
And also this comment should be moved below the enum.

2) This is used in parallel of the shared memory status 
s/ in parallel of/ in parallel with

The new status of this patch is: Waiting on Author

-- 
Sent via pgsql-hackers mailing list ([email-masked])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

---

From: Michael Paquier <[email-masked]>
Date: Wed, 15 Mar 2017 15:28:03 +0900

> in pg_stop_backup_callback() along with XLogCtl->Insert.exclusiveBackupState?

No, that's not necessary. sessionBackupState is not changed until the
code path where pg_stop_backup_callback() is active, and remains
unchanged until it gets deactivated.

> s/ in parallel of/ in parallel with

Agreed on both points.
-- 
Michael

---

From: Anastasia Lubennikova <[email-masked]>
Date: Wed, 15 Mar 2017 10:45:39 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

As I see, this bugfix works as expected and the patch is small and clear,
so I marked it "Ready for committer".
Anyway, it would be great if David could also have a look at the patch.

And again, thank you for fixing this issue!

The new status of this patch is: Ready for Committer

---

From: David Steele <[email-masked]>
Date: Wed, 15 Mar 2017 11:41:40 -0400

>> s/ in parallel of/ in parallel with

I have tested this patch and it behaves as expected and fixes the
original issue I reported.  One nit, I think:

+	 * SESSION_BACKUP_EXCLUSIVE in this one. Actual verification that an

Would be better phrased as:

+	 * SESSION_BACKUP_EXCLUSIVE in this function. Actual verification that an

Thanks,
-- 
-David
[email-masked]

---

From: Andres Freund <[email-masked]>
Date: Wed, 15 Mar 2017 08:46:38 -0700

Hi,

On 2017-03-15 15:28:03 +0900, Michael Paquier wrote:
> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 64335f909e..eaf8e32fe1 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> --- a/src/backend/access/transam/xlogfuncs.c
> +++ b/src/backend/access/transam/xlogfuncs.c
> diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
> index 104ee7dd5e..d4abf94862 100644
> --- a/src/include/access/xlog.h
> +++ b/src/include/access/xlog.h
> @@ -288,8 +288,26 @@ extern void assign_max_wal_size(int newval, void *extra);
>  extern void assign_checkpoint_completion_target(double newval, void *extra);

This seems like something easy enough to exercise in a tap test, could
we get one?

Greetings,

Andres Freund

---

From: Michael Paquier <[email-masked]>
Date: Thu, 16 Mar 2017 10:52:57 +0900

> This seems like something easy enough to exercise in a tap test, could

The first problem needs to have a cancel request sent when
pg_stop_backup() is running, the second needs to have a session held
to see an the inconsistent status of the session lock, which are two
concepts foreign to the TAP tests without being able to run the
queries asynchronously and keep the sessions alive.
-- 
Michael

---

From: Teodor Sigaev <[email-masked]>
Date: Thu, 23 Mar 2017 19:45:37 +0300

Hi!

I believe patch looks good and it's ready to commit. As I understand, it fixes 
bug introduced by
commit 7117685461af50f50c03f43e6a622284c8d54694
Date:   Tue Apr 5 20:03:49 2016 +0200

     Implement backup API functions for non-exclusive backups

And, suppose, it should be backpatched to 9.6?

-- 
Teodor Sigaev                                   E-mail: [email-masked]
                                                    WWW: http://www.sigaev.ru/

---

From: Michael Paquier <[email-masked]>
Date: Fri, 24 Mar 2017 09:21:40 +0900

On Fri, Mar 24, 2017 at 1:45 AM, Teodor Sigaev <[email-masked]> wrote:
> I believe patch looks good and it's ready to commit.

Thanks for the review!

> As I understand, it fixes bug introduced by

Indeed.

> And, suppose, it should be backpatched to 9.6?

Yes, that's where non-exclusive backups have been introduced.
-- 
Michael