Re: pgsql: Map basebackup tablespaces using a tablespace_map file

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?

> 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?

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?

- Heikki