Re: [HACKERS] Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)",File: "xlog.c", Line: 10200)

On Tue, Dec 13, 2016 at 10:24 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>
> Thanks for the review.

Thanks for the updated version of the patch!

>> +                    (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +                     errmsg("a backup is already starting")));
>> +        }
>> +        if (XLogCtl->Insert.exclusiveBackupState == EXCLUSIVE_BACKUP_STOPPING)
>> +        {
>> +            WALInsertLockRelease();
>> +            ereport(ERROR,
>> +                    (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>> +                     errmsg("a backup is currently stopping")));
>>
>> Isn't it better to use "an exclusive backup" explicitly rather than
>> "a backup" here?
>
> Yes. It would be better to not touch the original in-progress messages
> though.

On second thought, do users really want to distinguish those three errornous
states? I'm inclined to merge the checks for those three conditions into one,
that is,
       if (XLogCtl->Insert.exclusiveBackupState != EXCLUSIVE_BACKUP_IN_NONE)       {           WALInsertLockRelease();
        ereport(ERROR,                   (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("abackup is already in progress"),
 

Also it may be better to handle the similar checks in pg_stop_backup,
in the same way.

>> You changed the code so that pg_stop_backup() removes backup_label before
>> it marks the exclusive-backup-state as not-in-progress. Could you tell me
>> why you did this change? It's better to explain it in the comment.
>
> That's actually mentioned in the comments :)
>
> This is to ensure that there cannot be any other pg_stop_backup() running
> in parallel and trying to remove the backup label file. The key point here
> is that the backup_label file is removed during EXCLUSIVE_BACKUP_STOPPING,
> that the removal of the backup_label file is kept last on purpose (that's
> what HEAD does anyway), and that we can rollback to an in-progress state
> in case of failure.

Okay.

Regards,

-- 
Fujii Masao