Re: Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file

On Tue, Aug 4, 2015 at 12:15 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao <masao.fujii@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