On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fu...@gmail.com> wrote: >> >> >> Thanks! Pushed. >> > > Thanks to you as well for committing the patch. > >> BTW, while reading the code related to tablespace_map, I found that >> CancelBackup() emits the WARNING message "online backup mode was not >> canceled" >> when rename() fails. Isn't this confusing (or incorrect)? > > Yes, it looks confusing. > >> ISTM that we can >> see that the online backup mode has already been canceled if backup_label >> file >> is successfully removed whether tablespace_map file remains or not. No? >> > > I think what we should do is that display successful cancellation message > only when both the files are renamed.
Please imagine the case where backup_label was successfully renamed but tablespace_map was not. Even in this case, I think that we can see that the backup mode was canceled because the remaining tablespace_map file will be ignored in the subsequent recovery. So we should emit the successful cancellation message when backup_label is renamed whether tablespace_map is successfully renamed or not? > I have drafted a patch (still I needs > to verify/test it, I will do that if you think the fix is in right > direction) to show > what I have in mind. Thanks for the patch! - /* if the file is not there, return */ - if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0) + /* if the backup_label or tablespace_map file is not there, return */ + if (stat(BACKUP_LABEL_FILE, &stat_buf) < 0 || + stat(TABLESPACE_MAP, &stat_buf) < 0) return; Seems problematic. This change always prevents the backup mode from being canceled when there is no tablespace. Because in that case tablespace_map file is not created when the backup mode is started, and then the above condition is always true. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers