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.fu...@gmail.com> wrote: > > On Tue, Aug 30, 2016 at 1:32 PM, Michael Paquier > > <michael.paqu...@gmail.com> wrote: > >> On Mon, Aug 29, 2016 at 11:16 PM, Fujii Masao <masao.fu...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
