From: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, Andreas Seltenreich <seltenreich(at)gmx(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Magnus Hagander <magnus(at)hagander(dot)net> |
Subject: | Re: Re: [sqlsmith] FailedAssertion("!(XLogCtl->Insert.exclusiveBackup)", File: "xlog.c", Line: 10200) |
Date: | 2016-12-13 13:24:37 |
Message-ID: | 20161213132437.GA642@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Dec 13, 2016 at 12:49 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
base-backup-crash-v6.patch | text/x-diff | 11.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jesper Pedersen | 2016-12-13 13:43:12 | pg_catversion builtin function |
Previous Message | Robert Haas | 2016-12-13 13:07:46 | Re: Broken SSL tests in master |