On Tue, Dec 13, 2016 at 10:24 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <masao.fu...@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 == 
>> +        {
>> +            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)
                     errmsg("a backup 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.



Fujii Masao

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

Reply via email to