On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Thanks for the review.
> Isn't it better to mention "an exclusive backup" here? What about
>
> EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is starting an exclusive
> backup.
> EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is stopping an exclusive
> backup.
OK, changed this way.
> I think that it's worth explaining why ExclusiveBackupState is necessary,
> in the comment.
Changed like that at the top of the declaration of ExclusiveBackupState:
* State of an exclusive backup, necessary to control concurrent activities
* across sessions when working on exclusive backups.
> - * exclusiveBackup is true if a backup started with pg_start_backup() is
> - * in progress, and nonExclusiveBackups is a counter indicating the number
> + * exclusiveBackupState is changed to EXCLUSIVE_BACKUP_STARTING for the
> + * duration of pg_start_backup(), then to EXCLUSIVE_BACKUP_IN_PROGRESS once
> + * it is done, and nonExclusiveBackups is a counter indicating the number
>
> Why didn't you mention EXCLUSIVE_BACKUP_STOPPING and _NONE?
> Basically it's better to explain fully how the state changes.
Let's simplify things instead and just say that the meaning of the values is
described where ExclusiveBackupState is declared.
> + (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.
> 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.
I have rewritten this block like that. Does that sound better?
+ * Remove backup_label for an exclusive backup. Before doing anything
+ * the status of the exclusive backup is switched to
+ * EXCLUSIVE_BACKUP_STOPPING to ensure that there cannot be concurrent
+ * operations. In case of failure, in which case the status of the
+ * exclusive backup is switched back to in-progress. The removal of the
+ * backup_label file is kept last as it is the critical piece proving
+ * if an exclusive backup is running or not in the event of a system
+ * crash.
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