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

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center