Fixing pg_basebackup with tablespaces found in $PGDATA - Mailing list pgsql-hackers

Hi,

As much as I've seen people frown upon $subject, it still happens in the
wild, and Magnus seems to agree that the current failure mode of our
pg_basebackup tool when confronted to the situation is a bug.

So here's a fix, attached.

To reproduce, mkdir -p $PGDATA/tbs/foo then CREATE TABLESPACE there, and
then pg_basebackup your server. If doing so from the same server, as I
did, then pick the tar format, as here:

  pg_basebackup -Ft -z -c fast -v -X fetch -D /tmp/backup

Then use tar to see that the base backup contains the whole content of
your foo tablespace, and if you did create another tablespace within
$PGDATA/pg_tblspc (which is the other common way to trigger that issue)
then add it to what you want to see:

  tar tzvf /tmp/backup/base.tar.gz pg_tblspc tbs/foo pg_tblspc/bar

Note that empty directories are expected, so tar should output their
entries. Those directories are where you need to be restoring the
tablespace tarballs.

When using pg_basebackup in plain mode, the error is that you get a copy
of all your tablespaces first, then the main PGDATA is copied over and
as the destination directories already do exists (and not empty) the
whole backup fails there.

The bug should be fixed against all revisions of pg_basebackup, though I
didn't try to apply this very patch on all target branches.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***************
*** 45,51 **** typedef struct
  } basebackup_options;


! static int64 sendDir(char *path, int basepathlen, bool sizeonly);
  static int64 sendTablespace(char *path, bool sizeonly);
  static bool sendFile(char *readfilename, char *tarfilename,
           struct stat * statbuf, bool missing_ok);
--- 45,52 ----
  } basebackup_options;


! static int64 sendDir(char *path, int basepathlen, int rootpathlen,
!                      bool sizeonly, List *tablespaces);
  static int64 sendTablespace(char *path, bool sizeonly);
  static bool sendFile(char *readfilename, char *tarfilename,
           struct stat * statbuf, bool missing_ok);
***************
*** 100,105 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
--- 101,114 ----
      XLogRecPtr    endptr;
      TimeLineID    endtli;
      char       *labelfile;
+     char        cwd[MAXPGPATH];
+     int         rootpathlen;
+
+     /* we need to compute rootpathlen to allow for skipping tablespaces
+      * installed within PGDATA
+      */
+     getcwd(cwd, MAXPGPATH);
+     rootpathlen = strlen(cwd) + 1;

      backup_started_in_recovery = RecoveryInProgress();

***************
*** 165,171 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)

          /* Add a node for the base directory at the end */
          ti = palloc0(sizeof(tablespaceinfo));
!         ti->size = opt->progress ? sendDir(".", 1, true) : -1;
          tablespaces = lappend(tablespaces, ti);

          /* Send tablespace header */
--- 174,181 ----

          /* Add a node for the base directory at the end */
          ti = palloc0(sizeof(tablespaceinfo));
!         ti->size = opt->progress ?
!             sendDir(".", 1, rootpathlen, true, tablespaces) : -1;
          tablespaces = lappend(tablespaces, ti);

          /* Send tablespace header */
***************
*** 191,197 **** perform_base_backup(basebackup_options *opt, DIR *tblspcdir)
                  sendFileWithContent(BACKUP_LABEL_FILE, labelfile);

                  /* ... then the bulk of the files ... */
!                 sendDir(".", 1, false);

                  /* ... and pg_control after everything else. */
                  if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
--- 201,207 ----
                  sendFileWithContent(BACKUP_LABEL_FILE, labelfile);

                  /* ... then the bulk of the files ... */
!                 sendDir(".", 1, rootpathlen, false, tablespaces);

                  /* ... and pg_control after everything else. */
                  if (lstat(XLOG_CONTROL_FILE, &statbuf) != 0)
***************
*** 779,785 **** sendTablespace(char *path, bool sizeonly)
      size = 512;                    /* Size of the header just added */

      /* Send all the files in the tablespace version directory */
!     size += sendDir(pathbuf, strlen(path), sizeonly);

      return size;
  }
--- 789,795 ----
      size = 512;                    /* Size of the header just added */

      /* Send all the files in the tablespace version directory */
!     size += sendDir(pathbuf, strlen(path), 0, sizeonly, NIL);

      return size;
  }
***************
*** 788,796 **** sendTablespace(char *path, bool sizeonly)
   * Include all files from the given directory in the output tar stream. If
   * 'sizeonly' is true, we just calculate a total length and return it, without
   * actually sending anything.
   */
  static int64
! sendDir(char *path, int basepathlen, bool sizeonly)
  {
      DIR           *dir;
      struct dirent *de;
--- 798,810 ----
   * Include all files from the given directory in the output tar stream. If
   * 'sizeonly' is true, we just calculate a total length and return it, without
   * actually sending anything.
+  *
+  * Omit any directory listed in tablepaces, so as to avoid backuping
+  * tablespaces twice when users did create their tablespaces inside PGDATA.
   */
  static int64
! sendDir(char *path, int basepathlen, int rootpathlen,
!         bool sizeonly, List *tablespaces)
  {
      DIR           *dir;
      struct dirent *de;
***************
*** 931,936 **** sendDir(char *path, int basepathlen, bool sizeonly)
--- 945,954 ----
          }
          else if (S_ISDIR(statbuf.st_mode))
          {
+             bool skip_this_dir = false;
+             int pathbuflen = strlen(pathbuf);
+             ListCell *lc;
+
              /*
               * Store a directory entry in the tar file so we can get the
               * permissions right.
***************
*** 939,946 **** sendDir(char *path, int basepathlen, bool sizeonly)
                  _tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
              size += 512;        /* Size of the header just added */

!             /* call ourselves recursively for a directory */
!             size += sendDir(pathbuf, basepathlen, sizeonly);
          }
          else if (S_ISREG(statbuf.st_mode))
          {
--- 957,981 ----
                  _tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf);
              size += 512;        /* Size of the header just added */

!             /* call ourselves recursively for a directory, only when that
!              * directory is not listed in the tablespaces list
!              */
!             foreach(lc, tablespaces)
!             {
!                 tablespaceinfo *ti = (tablespaceinfo *) lfirst(lc);
!
!                 /* we need to compare the relative parts only */
!                 skip_this_dir = ti->path
!                     && pathbuflen > 2 /* ./ */
!                     && strlen(ti->path) > rootpathlen
!                     && strcmp(ti->path + rootpathlen, pathbuf + 2) == 0;
!
!                 if (skip_this_dir)
!                     break;
!             }
!             if (!skip_this_dir)
!                 size += sendDir(pathbuf, basepathlen, rootpathlen,
!                                 sizeonly, tablespaces);
          }
          else if (S_ISREG(statbuf.st_mode))
          {