pg_basebackup pg_receivexlog: Issue fsync more carefully

Several places weren't careful about fsyncing in the way.  See 1d4a0ab1
and 606e0f98 for details about required fsyncs.

This adds a couple of functions in src/common/ that have an equivalent
in the backend: durable_rename(), fsync_parent_path()

From: Michael Paquier <michael.paquier@gmail.com>

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index d077544d62e00ce06716defc76a448600c7150a3..cd7d095103df953f1d1e68ed63be42d83ee84d47 100644 (file)
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -27,6 +27,7 @@
 #include <zlib.h>
 #endif
 
+#include "common/file_utils.h"
 #include "common/string.h"
 #include "fe_utils/string_utils.h"
 #include "getopt_long.h"
@@ -1196,6 +1197,10 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
 
        if (copybuf != NULL)
                PQfreemem(copybuf);
+
+       /* sync the resulting tar file, errors are not considered fatal */
+       if (strcmp(basedir, "-") != 0)
+               (void) fsync_fname(filename, false, progname);
 }
 
 
@@ -1472,6 +1477,11 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res, int rownum)
 
        if (basetablespace && writerecoveryconf)
                WriteRecoveryConf();
+
+       /*
+        * No data is synced here, everything is done for all tablespaces at the
+        * end.
+        */
 }
 
 /*
@@ -1950,6 +1960,23 @@ BaseBackup(void)
        PQclear(res);
        PQfinish(conn);
 
+       /*
+        * Make data persistent on disk once backup is completed. For tar
+        * format once syncing the parent directory is fine, each tar file
+        * created per tablespace has been already synced. In plain format,
+        * all the data of the base directory is synced, taking into account
+        * all the tablespaces. Errors are not considered fatal.
+        */
+       if (format == 't')
+       {
+               if (strcmp(basedir, "-") != 0)
+                       (void) fsync_fname(basedir, true, progname);
+       }
+       else
+       {
+               (void) fsync_pgdata(basedir, progname);
+       }
+
        if (verbose)
                fprintf(stderr, "%s: base backup completed\n", progname);
 }

diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 3a921ebf2db03f7a19df3ede7854c0d67c618188..6b78a60f2780bf9a4cbafa80e5a64215dd57cdfc 100644 (file)
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -26,6 +26,7 @@
 
 #include "libpq-fe.h"
 #include "access/xlog_internal.h"
+#include "common/file_utils.h"
 
 
 /* fd and filename for currently open WAL file */
@@ -71,17 +72,13 @@ mark_file_as_archived(const char *basedir, const char *fname)
                return false;
        }
 
-       if (fsync(fd) != 0)
-       {
-               fprintf(stderr, _("%s: could not fsync file \"%s\": %s\n"),
-                               progname, tmppath, strerror(errno));
-
-               close(fd);
+       close(fd);
 
+       if (fsync_fname(tmppath, false, progname) != 0)
                return false;
-       }
 
-       close(fd);
+       if (fsync_parent_path(tmppath, progname) != 0)
+               return false;
 
        return true;
 }
@@ -132,6 +129,16 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
        {
                /* File is open and ready to use */
                walfile = f;
+
+               /*
+                * fsync, in case of a previous crash between padding and fsyncing the
+                * file.
+                */
+               if (fsync_fname(fn, false, progname) != 0)
+                       return false;
+               if (fsync_parent_path(fn, progname) != 0)
+                       return false;
+
                return true;
        }
        if (statbuf.st_size != 0)
@@ -160,6 +167,17 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
        }
        free(zerobuf);
 
+       /*
+        * fsync WAL file and containing directory, to ensure the file is
+        * persistently created and zeroed. That's particularly important when
+        * using synchronous mode, where the file is modified and fsynced
+        * in-place, without a directory fsync.
+        */
+       if (fsync_fname(fn, false, progname) != 0)
+               return false;
+       if (fsync_parent_path(fn, progname) != 0)
+               return false;
+
        if (lseek(f, SEEK_SET, 0) != 0)
        {
                fprintf(stderr,
@@ -220,10 +238,9 @@ close_walfile(StreamCtl *stream, XLogRecPtr pos)
 
                snprintf(oldfn, sizeof(oldfn), "%s/%s%s", stream->basedir, current_walfile_name, stream->partial_suffix);
                snprintf(newfn, sizeof(newfn), "%s/%s", stream->basedir, current_walfile_name);
-               if (rename(oldfn, newfn) != 0)
+               if (durable_rename(oldfn, newfn, progname) != 0)
                {
-                       fprintf(stderr, _("%s: could not rename file \"%s\": %s\n"),
-                                       progname, current_walfile_name, strerror(errno));
+                       /* durable_rename produced a log entry */
                        return false;
                }
        }
@@ -341,14 +358,6 @@ writeTimeLineHistoryFile(StreamCtl *stream, char *filename, char *content)
                return false;
        }
 
-       if (fsync(fd) != 0)
-       {
-               close(fd);
-               fprintf(stderr, _("%s: could not fsync file \"%s\": %s\n"),
-                               progname, tmppath, strerror(errno));
-               return false;
-       }
-
        if (close(fd) != 0)
        {
                fprintf(stderr, _("%s: could not close file \"%s\": %s\n"),
@@ -359,10 +368,9 @@ writeTimeLineHistoryFile(StreamCtl *stream, char *filename, char *content)
        /*
         * Now move the completed history file into place with its final name.
         */
-       if (rename(tmppath, path) < 0)
+       if (durable_rename(tmppath, path, progname) < 0)
        {
-               fprintf(stderr, _("%s: could not rename file \"%s\" to \"%s\": %s\n"),
-                               progname, tmppath, path, strerror(errno));
+               /* durable_rename produced a log entry */
                return false;
        }
 

diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index b6f62f7bf142d5844d323a25dd8d7126a19d7600..04cd365e765e7a6300033a69145579793143403b 100644 (file)
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -34,7 +34,7 @@ static void pre_sync_fname(const char *fname, bool isdir,
                                                   const char *progname);
 #endif
 static void walkdir(const char *path,
-       void (*action) (const char *fname, bool isdir, const char *progname),
+       int (*action) (const char *fname, bool isdir, const char *progname),
        bool process_symlinks, const char *progname);
 
 /*
@@ -120,7 +120,7 @@ fsync_pgdata(const char *pg_data, const char *progname)
  */
 static void
 walkdir(const char *path,
-               void (*action) (const char *fname, bool isdir, const char *progname),
+               int (*action) (const char *fname, bool isdir, const char *progname),
                bool process_symlinks, const char *progname)
 {
        DIR                *dir;
@@ -228,7 +228,7 @@ pre_sync_fname(const char *fname, bool isdir, const char *progname)
  * directories on systems where that isn't allowed/required.  Reports
  * other errors non-fatally.
  */
-void
+int
 fsync_fname(const char *fname, bool isdir, const char *progname)
 {
        int                     fd;
@@ -256,10 +256,10 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
        if (fd < 0)
        {
                if (errno == EACCES || (isdir && errno == EISDIR))
-                       return;
+                       return 0;
                fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
                                progname, fname, strerror(errno));
-               return;
+               return -1;
        }
 
        returncode = fsync(fd);
@@ -269,8 +269,103 @@ fsync_fname(const char *fname, bool isdir, const char *progname)
         * those errors. Anything else needs to be reported.
         */
        if (returncode != 0 && !(isdir && errno == EBADF))
+       {
                fprintf(stderr, _("%s: could not fsync file \"%s\": %s\n"),
                                progname, fname, strerror(errno));
+               return -1;
+       }
 
        (void) close(fd);
+       return 0;
+}
+
+/*
+ * fsync_parent_path -- fsync the parent path of a file or directory
+ *
+ * This is aimed at making file operations persistent on disk in case of
+ * an OS crash or power failure.
+ */
+int
+fsync_parent_path(const char *fname, const char *progname)
+{
+       char            parentpath[MAXPGPATH];
+
+       strlcpy(parentpath, fname, MAXPGPATH);
+       get_parent_directory(parentpath);
+
+       /*
+        * get_parent_directory() returns an empty string if the input argument is
+        * just a file name (see comments in path.c), so handle that as being the
+        * current directory.
+        */
+       if (strlen(parentpath) == 0)
+               strlcpy(parentpath, ".", MAXPGPATH);
+
+       if (fsync_fname(parentpath, true, progname) != 0)
+               return -1;
+
+       return 0;
+}
+
+/*
+ * durable_rename -- rename(2) wrapper, issuing fsyncs required for durability
+ *
+ * Wrapper around rename, similar to the backend version.
+ */
+int
+durable_rename(const char *oldfile, const char *newfile, const char *progname)
+{
+       int             fd;
+
+       /*
+        * First fsync the old and target path (if it exists), to ensure that they
+        * are properly persistent on disk. Syncing the target file is not
+        * strictly necessary, but it makes it easier to reason about crashes;
+        * because it's then guaranteed that either source or target file exists
+        * after a crash.
+        */
+       if (fsync_fname(oldfile, false, progname) != 0)
+               return -1;
+
+       fd = open(newfile, PG_BINARY | O_RDWR, 0);
+       if (fd < 0)
+       {
+               if (errno != ENOENT)
+               {
+                       fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
+                                       progname, newfile, strerror(errno));
+                       return -1;
+               }
+       }
+       else
+       {
+               if (fsync(fd) != 0)
+               {
+                       fprintf(stderr, _("%s: could not fsync file \"%s\": %s\n"),
+                                       progname, newfile, strerror(errno));
+                       close(fd);
+                       return -1;
+               }
+               close(fd);
+       }
+
+       /* Time to do the real deal... */
+       if (rename(oldfile, newfile) != 0)
+       {
+               fprintf(stderr, _("%s: could not rename file \"%s\" to \"%s\": %s\n"),
+                               progname, oldfile, newfile, strerror(errno));
+               return -1;
+       }
+
+       /*
+        * To guarantee renaming the file is persistent, fsync the file with its
+        * new name, and its containing directory.
+        */
+       if (fsync_fname(newfile, false, progname) != 0)
+               return -1;
+
+       if (fsync_parent_path(newfile, progname) != 0)
+               return -1;
+
+       return 0;
 }

diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index d3794df7a0c1653356acc76238ad5cd851e548ae..1cb263d9e208281e23ea6e2dbbfeac07bbe05cb0 100644 (file)
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -15,8 +15,11 @@
 #ifndef FILE_UTILS_H
 #define FILE_UTILS_H
 
-extern void fsync_fname(const char *fname, bool isdir,
-                                               const char *progname);
+extern int fsync_fname(const char *fname, bool isdir,
+                                          const char *progname);
 extern void fsync_pgdata(const char *pg_data, const char *progname);
+extern int durable_rename(const char *oldfile, const char *newfile,
+                                                 const char *progname);
+extern int fsync_parent_path(const char *fname, const char *progname);
 
 #endif   /* FILE_UTILS_H */