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

Hello,

(the header of this message is crafted so it might be isolate
this message from the thread)

The patch still applies on the current master with disaplacements.

> On Tue, Aug 30, 2016 at 1:44 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > On Tue, Aug 30, 2016 at 1:32 PM, Michael Paquier
> > <michael(dot)paquier(at)gmail(dot)com> wrote:
> >> On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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 backup 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.

So, everything other than the XXX comment looks fine for me, and
this is rather straghtforward patch. So after deciding what to do
for the issue and anyone opposed to this patch, I'll make this
'Ready for commiter'.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center