Re: pgsql: Map basebackup tablespaces using a tablespace_map file

On 05/12/2015 10:33 AM, Heikki Linnakangas wrote:
> On 05/12/2015 04:42 PM, Andrew Dunstan wrote:
>> +
>> +               /*
>> +                * Remove the existing symlink if any and Create the
>> symlink
>> +                * under PGDATA.  We need to use rmtree instead of
>> rmdir as
>> +                * the link location might contain directories or files
>> +                * corresponding to the actual path. Some tar
>> utilities do
>> +                * things that way while extracting symlinks.
>> +                */
>> +               if (lstat(linkloc, &st) == 0 && S_ISDIR(st.st_mode))
>> +               {
>> +                   if (!rmtree(linkloc,true))
>> +                       ereport(ERROR,
>> +                               (errcode_for_file_access(),
>> +                                errmsg("could not remove directory
>> \"%s\": %m",
>> +                                       linkloc)));
>> +               }
>> +               else
>> +               {
>> +                   if (unlink(linkloc) < 0 && errno != ENOENT)
>> +                       ereport(ERROR,
>> +                               (errcode_for_file_access(),
>> +                                errmsg("could not remove symbolic
>> link \"%s\": %m",
>> +                                       linkloc)));
>> +               }
>> +
>
> That's scary. What tar utilitiy replaces the symlink with a non-empty
> directory?
>
> What if you call pg_start_backup() and take the backup with a utility
> that follows symlinks? I wouldn't recommend using such a tool, but
> with this patch, it will zap all the tablespaces. Before this, you at
> least got a working database and could read out all the data or fix
> the symlinks afterwards.
>
> Why is the RelationCacheInitFileRemove() call moved?

I will let Amit comment on this.

>
>>  XLogRecPtr
>>  do_pg_start_backup(const char *backupidstr, bool fast, TimeLineID
>> *starttli_p,
>> -                  char **labelfile)
>> +                  char **labelfile, DIR *tblspcdir, List **tablespaces,
>> +                  char **tblspcmapfile, bool infotbssize,
>> +                  bool needtblspcmapfile)
>
> Why does this need to take tblspcdir as argument? Wouldn't it make
> more sense to open the directory inside do_pg_start_backup?


Good point. It does look cleaner to do that.


>
> In the BASE_BACKUP command, is there any reason to not always include
> the tablespace map? IOW, do we really need the TABLESPACE_MAP option?
>
>
>


I assume it was done for minimal disturbance, i.e. that's where we need
it. Amit, comments?

cheers

andrew