Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-24 Thread Michael Paquier 
On Fri, Mar 24, 2017 at 7:58 PM, Teodor Sigaev  wrote:
> Thank you all, pushed.

Thanks.
-- 
Michael


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-24 Thread Teodor Sigaev 

Thank you all, pushed

Michael Paquier wrote:

On Fri, Mar 24, 2017 at 1:45 AM, Teodor Sigaev  wrote:

I believe patch looks good and it's ready to commit.


Thanks for the review!


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


Indeed.


And, suppose, it should be backpatched to 9.6?


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



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-23 Thread Michael Paquier 
On Fri, Mar 24, 2017 at 1:45 AM, Teodor Sigaev  wrote:
> I believe patch looks good and it's ready to commit.

Thanks for the review!

> 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

Indeed.

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

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


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-23 Thread Teodor Sigaev 

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: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-15 Thread Michael Paquier 
On Thu, Mar 16, 2017 at 12:46 AM, Andres Freund  wrote:
> 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?

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


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-15 Thread Andres Freund 
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


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-15 Thread David Steele 
On 3/15/17 2:28 AM, Michael Paquier wrote:
> On Wed, Mar 15, 2017 at 12:27 AM, Anastasia Lubennikova
>  wrote:
>> 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?
> 
> 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.
> 
>> 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
> 
> Agreed on both points.

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
da...@pgmasters.net


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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-15 Thread Anastasia Lubennikova 
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

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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-15 Thread Michael Paquier 
On Wed, Mar 15, 2017 at 12:27 AM, Anastasia Lubennikova
 wrote:
> 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?

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.

> 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

Agreed on both points.
-- 
Michael


backup-session-locks-fixes-v2.patch
Description: Binary data

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

Re: [HACKERS] Backend crash on non-exclusive backup cancel

2017-03-14 Thread Anastasia Lubennikova 
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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers