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



--
Sent via pgsql-committers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Reply via email to