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

On Tue, Nov 8, 2016 at 11:18 AM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hello,
>
> At Sat, 5 Nov 2016 21:18:42 +0900, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote in <CAB7nPqS8FQf3T3ZLw=v-FMMbUEM_kKcVzfxybTnG68u-SfZJ7Q(at)mail(dot)gmail(dot)com>
>> > I don't see any problem on the state-transition of
>> > exclusiveBackupState. For the following part
>> >
>> > @@ -10217,7 +10255,7 @@ do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID *starttli_p,
>> > {
>> > /*
>> > * Check for existing backup label --- implies a back>
>> up is already
>> > - * running. (XXX given that we checked exclusiveBackup above,
>> > + * running. (XXX given that we checked exclusiveBackupState above,
>> > * maybe it would be OK to just unlink any such label file?)
>> >
>> > The issue in the XXX comment should be settled by this
>> > chance. PostgreSQL no longer (or should not) places the file by
>> > mistake of itself. It is only possible by someone evil crafting
>> > it, or crash-then-restarted. For the former it can be safely
>> > removed with some notice. For the latter we should remove it
>> > since the backup taken through the restarting is not
>> > reliable. Addition to that, issueing pg_start_backup indicates
>> > that the operator already have forgotten that he/she issued it
>> > previously. So it seems to me that it can be removed with some
>> > WARNING.
>> >
>> > The error checking on exclusiveBackupState looks somewhat
>> > redandunt but I don't come up with a better coding.
>>
>> Yes, that's on purpose to make the error messages more verbose for the user.
>>
>> > So, everything other than the XXX comment looks fine for me, and
>> > this is rather straghtforward patch.
>>
>> I agree that we could do something better, but that would be an
>> optimization, not a bug fix which is what this patch is aiming at.
>
> Ok, I understand that.
>
>> > So after deciding what to do
>> > for the issue and anyone opposed to this patch, I'll make this
>> > 'Ready for commiter'.
>>
>> Thanks for the input.
>>
>> By the way, as long as I look at that, the patch applies cleanly on
>> master and 9.6 but not further down. If a committer shows up to push
>> this patch, I can prepare versions for back branches as needed.
>
> Ok, the attached is the version just rebased to the current
> master. It is clieanly appliable without whitespace errors on
> master and 9.6 with some displacements but fails on 9.5.
>
> I will mark this as "Ready for Committer".

Thanks for updating the patch! Here are the review comments;

+ * EXCLUSIVE_BACKUP_NONE means that there is no exclusive backup actually
+ * running, to be more precise pg_start_backup() is not being executed for
+ * an exclusive backup and there is exclusive backup in progress.

"there is exclusive" should be "there is no exclusive"?
ISTM that the description following "to be more" doesn't seem to be necessary.

+ * EXCLUSIVE_BACKUP_STARTING means that pg_start_backup() is running and
+ * that is is not done yet.
+ * EXCLUSIVE_BACKUP_STOPPING means that pg_stop_backup() is running and
+ * that is is not done yet.

Typo: "is is"

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.

I think that it's worth explaining why ExclusiveBackupState is necessary,
in the comment.

- * 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.

+ (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?

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.

Regards,

--
Fujii Masao