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

On 3/15/17 2:28 AM, Michael Paquier wrote:
> On Wed, Mar 15, 2017 at 12:27 AM, Anastasia Lubennikova
> <lubennikovaav@gmail.com> 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
david@pgmasters.net