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

On Fri, Nov 4, 2016 at 5:36 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>> On Tue, Aug 30, 2016 at 1:44 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> > On Tue, Aug 30, 2016 at 1:32 PM, Michael Paquier
>> > <michael.paquier@gmail.com> wrote:
>> >> On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> >>> You seem to add another entry for this patch into CommitFest.
>> >>> Either of them needs to be removed.
>> >>> https://commitfest.postgresql.org/10/698/
>> >>
>> >> Indeed. I just removed this one.
>> >>
>> >>> This patch prevents pg_stop_backup from starting while pg_start_backup
>> >>> is running. OTOH, we also should prevent pg_start_backup from starting
>> >>> until "previous" pg_stop_backup has completed? What happens if
>> >>> pg_start_backup starts while pg_stop_backup is running?
>> >>> As far as I read the current code, ISTM that there is no need to do that,
>> >>> but it's better to do the double check.
>> >>
>> >> I don't agree about doing that.
>> >
>> > Hmm... my previous comment was confusing. I wanted to comment "it's better
>> > *also for you* to do the double check whether we need to prevent pg_start_backup
>> > while pg_stop_backup is running or not". Your answer seems to be almost the same
>> > as mine, i.e., not necessary. Right?
>>
>> Yes, that's not necessary to do more. In the worst case, say
>> pg_start_backup starts when pg_stop_backup is running. And
>> pg_stop_backup has decremented the backup counters, but not yet
>> removed the backup_label, then pg_start_backup would just choke on the
>> presence of the backup_label file
>
> 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.

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