On Sat, Sep 10, 2016 at 6:27 PM, Peter Eisentraut
<peter.eisentr...@2ndquadrant.com> wrote:
> In fsync_pgdata(), you have removed the progname from one error
> message, even though it is passed into the function.
Right. Good catch.
> Also, I think
> fsync_pgdata() should not be printing initdb's progress messages.
> That should stay in initdb.
That's a reference to that.
+ fputs(_("syncing data to disk ... "), stdout);
+ fflush(stdout);
Oops, fixed. I missed that now that I look at it. Perhaps I was
thinking something else when hacking that lastly, like getting this
message out for pg_basebackup as well.
> Worse, in 0002 you are then changing the
> output, presumably to suit pg_basebackup or something else, which
> messes up the initdb output.
Yes, fixed.
> durable_rename() does not fsync an existing newfile before renaming,
> in contrast to the backend code in fd.c.
That's in 0002. In the case of initdb it did not really matter, but
that matters for pg_receivexlog, so let's do it unconditionally. I
reworked the code to check if the existing new file is here, and
fsync() if it exists.
> I'm slightly concerned about lots of code duplication in
> durable_rename, fsync_parent_path between backend and standalone code.
Based on the ground of code readability, I am not sure that it would
be worth sharing the code of durable_rename or even fsync_parent_path
between the backend and the frontend, particularly because they are
actually not really duplicated... One good step in favor of that would
be to get a elog()/ereport() layer in src/common as well, but that's
really something that this set of patches should tackle, no?
> Also, I'm equally slightly concerned that having duplicate function
> names will mess up tag search for someone.
There are similar cases, take palloc().. Now perhaps some of those
functions could be renamed pg_fsync_... Thoughts?
> This is a preexisting mistake, but fsync_fname_ext() should just be
> fsync_fname() to match the backend naming. In the backend,
> fsync_fname_ext() "extends" fsync_fname() by accepting an ignore_perm
> argument, but the initdb code doesn't do that.
OK.
> I don't think tar file output in pg_basebackup needs to be fsynced.
> It's just a backup file like what pg_dump produces, and we don't fsync
> that either. The point of this change is to leave a data directory in
> a synced state equivalent to what initdb leaves behind.
Here I don't agree. The point of the patch is to make sure that what
gets generated by pg_basebackup is consistent on disk, for all the
formats. It seems weird to me that we could say that the plain format
makes things consistent while the tar format may cause some files to
be lost in case of power outage. The docs make it clear I think:
+ By default, <command>pg_basebackup</command> will wait for all files
+ to be written safely to disk.
But perhaps this should directly mention that this is done for all the formats?
> Some of the changes in receivelog.c seem excessive. Replacing a plain
> fsync() with fsync_fname_ext() plus fsync_parent_path() isn't
> justified by the references you have provided. This would need more
> explanation.
In mark_file_as_archived(), we had better do it or in case of a crash
the .done file would just be gone. open_walfile() as well need that,
to ensure that the segment is created and zeroed, and in the case
where it was padded (is this one overthinking it?).
Patch 0003 had conflicts caused by 9083353.
--
Michael
From 6158dc8f70dd100f59d7d2c4b7a60435b7c1cfac Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 13 Sep 2016 12:14:12 +0900
Subject: [PATCH 1/4] Relocation fsync routines of initdb into src/common
Those are aimed at being used by other utilities, pg_basebackup mainly,
and at some other degree by pg_dump and pg_receivexlog.
---
src/bin/initdb/initdb.c | 270 ++-------------------------------------
src/common/Makefile | 2 +-
src/common/file_utils.c | 276 ++++++++++++++++++++++++++++++++++++++++
src/include/common/file_utils.h | 22 ++++
src/tools/msvc/Mkvcbuild.pm | 2 +-
5 files changed, 311 insertions(+), 261 deletions(-)
create mode 100644 src/common/file_utils.c
create mode 100644 src/include/common/file_utils.h
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3350e13..e52e67d 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -61,6 +61,7 @@
#endif
#include "catalog/catalog.h"
+#include "common/file_utils.h"
#include "common/restricted_token.h"
#include "common/username.h"
#include "mb/pg_wchar.h"
@@ -70,13 +71,6 @@
#include "fe_utils/string_utils.h"
-/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
-#if defined(HAVE_SYNC_FILE_RANGE)
-#define PG_FLUSH_DATA_WORKS 1
-#elif defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED)
-#define PG_FLUSH_DATA_WORKS 1
-#endif
-
/* Ideally this would be in a .h file, but it hardly seems worth the trouble */
extern const char *select_default_timezone(const char *share_path);
@@ -237,13 +231,6 @@ static char **filter_lines_with_token(char **lines, const char *token);
#endif
static char **readfile(const char *path);
static void writefile(char *path, char **lines);
-static void walkdir(const char *path,
- void (*action) (const char *fname, bool isdir),
- bool process_symlinks);
-#ifdef PG_FLUSH_DATA_WORKS
-static void pre_sync_fname(const char *fname, bool isdir);
-#endif
-static void fsync_fname_ext(const char *fname, bool isdir);
static FILE *popen_check(const char *command, const char *mode);
static void exit_nicely(void);
static char *get_id(void);
@@ -270,7 +257,6 @@ static void load_plpgsql(FILE *cmdfd);
static void vacuum_db(FILE *cmdfd);
static void make_template0(FILE *cmdfd);
static void make_postgres(FILE *cmdfd);
-static void fsync_pgdata(void);
static void trapsig(int signum);
static void check_ok(void);
static char *escape_quotes(const char *src);
@@ -529,177 +515,6 @@ writefile(char *path, char **lines)
}
/*
- * walkdir: recursively walk a directory, applying the action to each
- * regular file and directory (including the named directory itself).
- *
- * If process_symlinks is true, the action and recursion are also applied
- * to regular files and directories that are pointed to by symlinks in the
- * given directory; otherwise symlinks are ignored. Symlinks are always
- * ignored in subdirectories, ie we intentionally don't pass down the
- * process_symlinks flag to recursive calls.
- *
- * Errors are reported but not considered fatal.
- *
- * See also walkdir in fd.c, which is a backend version of this logic.
- */
-static void
-walkdir(const char *path,
- void (*action) (const char *fname, bool isdir),
- bool process_symlinks)
-{
- DIR *dir;
- struct dirent *de;
-
- dir = opendir(path);
- if (dir == NULL)
- {
- fprintf(stderr, _("%s: could not open directory \"%s\": %s\n"),
- progname, path, strerror(errno));
- return;
- }
-
- while (errno = 0, (de = readdir(dir)) != NULL)
- {
- char subpath[MAXPGPATH];
- struct stat fst;
- int sret;
-
- if (strcmp(de->d_name, ".") == 0 ||
- strcmp(de->d_name, "..") == 0)
- continue;
-
- snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name);
-
- if (process_symlinks)
- sret = stat(subpath, &fst);
- else
- sret = lstat(subpath, &fst);
-
- if (sret < 0)
- {
- fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
- progname, subpath, strerror(errno));
- continue;
- }
-
- if (S_ISREG(fst.st_mode))
- (*action) (subpath, false);
- else if (S_ISDIR(fst.st_mode))
- walkdir(subpath, action, false);
- }
-
- if (errno)
- fprintf(stderr, _("%s: could not read directory \"%s\": %s\n"),
- progname, path, strerror(errno));
-
- (void) closedir(dir);
-
- /*
- * It's important to fsync the destination directory itself as individual
- * file fsyncs don't guarantee that the directory entry for the file is
- * synced. Recent versions of ext4 have made the window much wider but
- * it's been an issue for ext3 and other filesystems in the past.
- */
- (*action) (path, true);
-}
-
-/*
- * Hint to the OS that it should get ready to fsync() this file.
- *
- * Ignores errors trying to open unreadable files, and reports other errors
- * non-fatally.
- */
-#ifdef PG_FLUSH_DATA_WORKS
-
-static void
-pre_sync_fname(const char *fname, bool isdir)
-{
- int fd;
-
- fd = open(fname, O_RDONLY | PG_BINARY);
-
- if (fd < 0)
- {
- if (errno == EACCES || (isdir && errno == EISDIR))
- return;
- fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
- progname, fname, strerror(errno));
- return;
- }
-
- /*
- * We do what pg_flush_data() would do in the backend: prefer to use
- * sync_file_range, but fall back to posix_fadvise. We ignore errors
- * because this is only a hint.
- */
-#if defined(HAVE_SYNC_FILE_RANGE)
- (void) sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE);
-#elif defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED)
- (void) posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
-#else
-#error PG_FLUSH_DATA_WORKS should not have been defined
-#endif
-
- (void) close(fd);
-}
-
-#endif /* PG_FLUSH_DATA_WORKS */
-
-/*
- * fsync_fname_ext -- Try to fsync a file or directory
- *
- * Ignores errors trying to open unreadable files, or trying to fsync
- * directories on systems where that isn't allowed/required. Reports
- * other errors non-fatally.
- */
-static void
-fsync_fname_ext(const char *fname, bool isdir)
-{
- int fd;
- int flags;
- int returncode;
-
- /*
- * Some OSs require directories to be opened read-only whereas other
- * systems don't allow us to fsync files opened read-only; so we need both
- * cases here. Using O_RDWR will cause us to fail to fsync files that are
- * not writable by our userid, but we assume that's OK.
- */
- flags = PG_BINARY;
- if (!isdir)
- flags |= O_RDWR;
- else
- flags |= O_RDONLY;
-
- /*
- * Open the file, silently ignoring errors about unreadable files (or
- * unsupported operations, e.g. opening a directory under Windows), and
- * logging others.
- */
- fd = open(fname, flags);
- if (fd < 0)
- {
- if (errno == EACCES || (isdir && errno == EISDIR))
- return;
- fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
- progname, fname, strerror(errno));
- return;
- }
-
- returncode = fsync(fd);
-
- /*
- * Some OSes don't allow us to fsync directories at all, so we can ignore
- * 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));
-
- (void) close(fd);
-}
-
-/*
* Open a subcommand with suitable error messaging
*/
static FILE *
@@ -2276,77 +2091,6 @@ make_postgres(FILE *cmdfd)
PG_CMD_PUTS(*line);
}
-/*
- * Issue fsync recursively on PGDATA and all its contents.
- *
- * We fsync regular files and directories wherever they are, but we
- * follow symlinks only for pg_xlog and immediately under pg_tblspc.
- * Other symlinks are presumed to point at files we're not responsible
- * for fsyncing, and might not have privileges to write at all.
- *
- * Errors are reported but not considered fatal.
- */
-static void
-fsync_pgdata(void)
-{
- bool xlog_is_symlink;
- char pg_xlog[MAXPGPATH];
- char pg_tblspc[MAXPGPATH];
-
- fputs(_("syncing data to disk ... "), stdout);
- fflush(stdout);
-
- snprintf(pg_xlog, MAXPGPATH, "%s/pg_xlog", pg_data);
- snprintf(pg_tblspc, MAXPGPATH, "%s/pg_tblspc", pg_data);
-
- /*
- * If pg_xlog is a symlink, we'll need to recurse into it separately,
- * because the first walkdir below will ignore it.
- */
- xlog_is_symlink = false;
-
-#ifndef WIN32
- {
- struct stat st;
-
- if (lstat(pg_xlog, &st) < 0)
- fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
- progname, pg_xlog, strerror(errno));
- else if (S_ISLNK(st.st_mode))
- xlog_is_symlink = true;
- }
-#else
- if (pgwin32_is_junction(pg_xlog))
- xlog_is_symlink = true;
-#endif
-
- /*
- * If possible, hint to the kernel that we're soon going to fsync the data
- * directory and its contents.
- */
-#ifdef PG_FLUSH_DATA_WORKS
- walkdir(pg_data, pre_sync_fname, false);
- if (xlog_is_symlink)
- walkdir(pg_xlog, pre_sync_fname, false);
- walkdir(pg_tblspc, pre_sync_fname, true);
-#endif
-
- /*
- * Now we do the fsync()s in the same order.
- *
- * The main call ignores symlinks, so in addition to specially processing
- * pg_xlog if it's a symlink, pg_tblspc has to be visited separately with
- * process_symlinks = true. Note that if there are any plain directories
- * in pg_tblspc, they'll get fsync'd twice. That's not an expected case
- * so we don't worry about optimizing it.
- */
- walkdir(pg_data, fsync_fname_ext, false);
- if (xlog_is_symlink)
- walkdir(pg_xlog, fsync_fname_ext, false);
- walkdir(pg_tblspc, fsync_fname_ext, true);
-
- check_ok();
-}
/*
@@ -3512,7 +3256,10 @@ main(int argc, char *argv[])
exit_nicely();
}
- fsync_pgdata();
+ fputs(_("syncing data to disk ... "), stdout);
+ fflush(stdout);
+ fsync_pgdata(pg_data, progname);
+ check_ok();
return 0;
}
@@ -3574,7 +3321,12 @@ main(int argc, char *argv[])
initialize_data_directory();
if (do_sync)
- fsync_pgdata();
+ {
+ fputs(_("syncing data to disk ... "), stdout);
+ fflush(stdout);
+ fsync_pgdata(pg_data, progname);
+ check_ok();
+ }
else
printf(_("\nSync to disk skipped.\nThe data directory might become corrupt if the operating system crashes.\n"));
diff --git a/src/common/Makefile b/src/common/Makefile
index a5fa649..03dfaa1 100644
--- a/src/common/Makefile
+++ b/src/common/Makefile
@@ -44,7 +44,7 @@ OBJS_COMMON = config_info.o controldata_utils.o exec.o ip.o keywords.o \
md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o rmtree.o \
string.o username.o wait_error.o
-OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o restricted_token.o
+OBJS_FRONTEND = $(OBJS_COMMON) fe_memutils.o file_utils.o restricted_token.o
OBJS_SRV = $(OBJS_COMMON:%.o=%_srv.o)
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
new file mode 100644
index 0000000..b6f62f7
--- /dev/null
+++ b/src/common/file_utils.c
@@ -0,0 +1,276 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines.
+ *
+ * Assorted utility functions to work on files.
+ *
+ *
+ * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/common/file_utils.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres_fe.h"
+
+#include <dirent.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "common/file_utils.h"
+
+
+/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
+#if defined(HAVE_SYNC_FILE_RANGE)
+#define PG_FLUSH_DATA_WORKS 1
+#elif defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED)
+#define PG_FLUSH_DATA_WORKS 1
+#endif
+
+#ifdef PG_FLUSH_DATA_WORKS
+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),
+ bool process_symlinks, const char *progname);
+
+/*
+ * Issue fsync recursively on PGDATA and all its contents.
+ *
+ * We fsync regular files and directories wherever they are, but we
+ * follow symlinks only for pg_xlog and immediately under pg_tblspc.
+ * Other symlinks are presumed to point at files we're not responsible
+ * for fsyncing, and might not have privileges to write at all.
+ *
+ * Errors are reported but not considered fatal.
+ */
+void
+fsync_pgdata(const char *pg_data, const char *progname)
+{
+ bool xlog_is_symlink;
+ char pg_xlog[MAXPGPATH];
+ char pg_tblspc[MAXPGPATH];
+
+ snprintf(pg_xlog, MAXPGPATH, "%s/pg_xlog", pg_data);
+ snprintf(pg_tblspc, MAXPGPATH, "%s/pg_tblspc", pg_data);
+
+ /*
+ * If pg_xlog is a symlink, we'll need to recurse into it separately,
+ * because the first walkdir below will ignore it.
+ */
+ xlog_is_symlink = false;
+
+#ifndef WIN32
+ {
+ struct stat st;
+
+ if (lstat(pg_xlog, &st) < 0)
+ fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
+ progname, pg_xlog, strerror(errno));
+ else if (S_ISLNK(st.st_mode))
+ xlog_is_symlink = true;
+ }
+#else
+ if (pgwin32_is_junction(pg_xlog))
+ xlog_is_symlink = true;
+#endif
+
+ /*
+ * If possible, hint to the kernel that we're soon going to fsync the data
+ * directory and its contents.
+ */
+#ifdef PG_FLUSH_DATA_WORKS
+ walkdir(pg_data, pre_sync_fname, false, progname);
+ if (xlog_is_symlink)
+ walkdir(pg_xlog, pre_sync_fname, false, progname);
+ walkdir(pg_tblspc, pre_sync_fname, true, progname);
+#endif
+
+ /*
+ * Now we do the fsync()s in the same order.
+ *
+ * The main call ignores symlinks, so in addition to specially processing
+ * pg_xlog if it's a symlink, pg_tblspc has to be visited separately with
+ * process_symlinks = true. Note that if there are any plain directories
+ * in pg_tblspc, they'll get fsync'd twice. That's not an expected case
+ * so we don't worry about optimizing it.
+ */
+ walkdir(pg_data, fsync_fname, false, progname);
+ if (xlog_is_symlink)
+ walkdir(pg_xlog, fsync_fname, false, progname);
+ walkdir(pg_tblspc, fsync_fname, true, progname);
+}
+
+/*
+ * walkdir: recursively walk a directory, applying the action to each
+ * regular file and directory (including the named directory itself).
+ *
+ * If process_symlinks is true, the action and recursion are also applied
+ * to regular files and directories that are pointed to by symlinks in the
+ * given directory; otherwise symlinks are ignored. Symlinks are always
+ * ignored in subdirectories, ie we intentionally don't pass down the
+ * process_symlinks flag to recursive calls.
+ *
+ * Errors are reported but not considered fatal.
+ *
+ * See also walkdir in fd.c, which is a backend version of this logic.
+ */
+static void
+walkdir(const char *path,
+ void (*action) (const char *fname, bool isdir, const char *progname),
+ bool process_symlinks, const char *progname)
+{
+ DIR *dir;
+ struct dirent *de;
+
+ dir = opendir(path);
+ if (dir == NULL)
+ {
+ fprintf(stderr, _("%s: could not open directory \"%s\": %s\n"),
+ progname, path, strerror(errno));
+ return;
+ }
+
+ while (errno = 0, (de = readdir(dir)) != NULL)
+ {
+ char subpath[MAXPGPATH];
+ struct stat fst;
+ int sret;
+
+ if (strcmp(de->d_name, ".") == 0 ||
+ strcmp(de->d_name, "..") == 0)
+ continue;
+
+ snprintf(subpath, MAXPGPATH, "%s/%s", path, de->d_name);
+
+ if (process_symlinks)
+ sret = stat(subpath, &fst);
+ else
+ sret = lstat(subpath, &fst);
+
+ if (sret < 0)
+ {
+ fprintf(stderr, _("%s: could not stat file \"%s\": %s\n"),
+ progname, subpath, strerror(errno));
+ continue;
+ }
+
+ if (S_ISREG(fst.st_mode))
+ (*action) (subpath, false, progname);
+ else if (S_ISDIR(fst.st_mode))
+ walkdir(subpath, action, false, progname);
+ }
+
+ if (errno)
+ fprintf(stderr, _("%s: could not read directory \"%s\": %s\n"),
+ progname, path, strerror(errno));
+
+ (void) closedir(dir);
+
+ /*
+ * It's important to fsync the destination directory itself as individual
+ * file fsyncs don't guarantee that the directory entry for the file is
+ * synced. Recent versions of ext4 have made the window much wider but
+ * it's been an issue for ext3 and other filesystems in the past.
+ */
+ (*action) (path, true, progname);
+}
+
+/*
+ * Hint to the OS that it should get ready to fsync() this file.
+ *
+ * Ignores errors trying to open unreadable files, and reports other errors
+ * non-fatally.
+ */
+#ifdef PG_FLUSH_DATA_WORKS
+
+static void
+pre_sync_fname(const char *fname, bool isdir, const char *progname)
+{
+ int fd;
+
+ fd = open(fname, O_RDONLY | PG_BINARY);
+
+ if (fd < 0)
+ {
+ if (errno == EACCES || (isdir && errno == EISDIR))
+ return;
+ fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
+ progname, fname, strerror(errno));
+ return;
+ }
+
+ /*
+ * We do what pg_flush_data() would do in the backend: prefer to use
+ * sync_file_range, but fall back to posix_fadvise. We ignore errors
+ * because this is only a hint.
+ */
+#if defined(HAVE_SYNC_FILE_RANGE)
+ (void) sync_file_range(fd, 0, 0, SYNC_FILE_RANGE_WRITE);
+#elif defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED)
+ (void) posix_fadvise(fd, 0, 0, POSIX_FADV_DONTNEED);
+#else
+#error PG_FLUSH_DATA_WORKS should not have been defined
+#endif
+
+ (void) close(fd);
+}
+
+#endif /* PG_FLUSH_DATA_WORKS */
+
+/*
+ * fsync_fname -- Try to fsync a file or directory
+ *
+ * Ignores errors trying to open unreadable files, or trying to fsync
+ * directories on systems where that isn't allowed/required. Reports
+ * other errors non-fatally.
+ */
+void
+fsync_fname(const char *fname, bool isdir, const char *progname)
+{
+ int fd;
+ int flags;
+ int returncode;
+
+ /*
+ * Some OSs require directories to be opened read-only whereas other
+ * systems don't allow us to fsync files opened read-only; so we need both
+ * cases here. Using O_RDWR will cause us to fail to fsync files that are
+ * not writable by our userid, but we assume that's OK.
+ */
+ flags = PG_BINARY;
+ if (!isdir)
+ flags |= O_RDWR;
+ else
+ flags |= O_RDONLY;
+
+ /*
+ * Open the file, silently ignoring errors about unreadable files (or
+ * unsupported operations, e.g. opening a directory under Windows), and
+ * logging others.
+ */
+ fd = open(fname, flags);
+ if (fd < 0)
+ {
+ if (errno == EACCES || (isdir && errno == EISDIR))
+ return;
+ fprintf(stderr, _("%s: could not open file \"%s\": %s\n"),
+ progname, fname, strerror(errno));
+ return;
+ }
+
+ returncode = fsync(fd);
+
+ /*
+ * Some OSes don't allow us to fsync directories at all, so we can ignore
+ * 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));
+
+ (void) close(fd);
+}
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
new file mode 100644
index 0000000..d3794df
--- /dev/null
+++ b/src/include/common/file_utils.h
@@ -0,0 +1,22 @@
+/*-------------------------------------------------------------------------
+ *
+ * File-processing utility routines for frontend code
+ *
+ * Assorted utility functions to work on files.
+ *
+ *
+ * Portions Copyright (c) 1996-2016, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/common/file_utils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef FILE_UTILS_H
+#define FILE_UTILS_H
+
+extern void fsync_fname(const char *fname, bool isdir,
+ const char *progname);
+extern void fsync_pgdata(const char *pg_data, const char *progname);
+
+#endif /* FILE_UTILS_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 93dfd24..3b925b6 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -115,7 +115,7 @@ sub mkvcbuild
string.c username.c wait_error.c);
our @pgcommonfrontendfiles = (
- @pgcommonallfiles, qw(fe_memutils.c
+ @pgcommonallfiles, qw(file_utils.c fe_memutils.c
restricted_token.c));
our @pgcommonbkndfiles = @pgcommonallfiles;
--
2.10.0
From 4eb14fd2b6eaf7ea61932e2d9684a8b4dfc1e097 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 13 Sep 2016 13:00:51 +0900
Subject: [PATCH 2/4] Issue fsync more carefully in pg_receivexlog and
pg_basebackup [-X] stream.
Several places weren't careful about fsyncing in the way. See 1d4a0ab1
and 606e0f98 for details about required fsyns.
This adds a couple of routines in fe_utils which have an equivalent in the
backend:
- durable_rename
- fsync_parent_path
---
src/bin/pg_basebackup/pg_basebackup.c | 27 +++++++++
src/bin/pg_basebackup/receivelog.c | 56 ++++++++++-------
src/common/file_utils.c | 110 ++++++++++++++++++++++++++++++++--
src/include/common/file_utils.h | 7 ++-
4 files changed, 171 insertions(+), 29 deletions(-)
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 42f3b27..2266e34 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -25,6 +25,7 @@
#include <zlib.h>
#endif
+#include "common/file_utils.h"
#include "common/string.h"
#include "fe_utils/string_utils.h"
#include "getopt_long.h"
@@ -1194,6 +1195,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);
}
@@ -1470,6 +1475,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.
+ */
}
/*
@@ -1948,6 +1958,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 062730b..0a740ce 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -23,6 +23,7 @@
#include "libpq-fe.h"
#include "access/xlog_internal.h"
+#include "common/file_utils.h"
/* fd and filename for currently open WAL file */
@@ -68,17 +69,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;
}
@@ -116,6 +113,10 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
/*
* Verify that the file is either empty (just created), or a complete
* XLogSegSize segment. Anything in between indicates a corrupt file.
+ *
+ * XXX: This means that we might not restart if a crash occurs before the
+ * fsync below. We probably should create the file in a temporary path
+ * like the backend does...
*/
if (fstat(f, &statbuf) != 0)
{
@@ -129,6 +130,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)
@@ -157,6 +168,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,
@@ -217,10 +239,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;
}
}
@@ -338,14 +359,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"),
@@ -356,10 +369,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 b6f62f7..4e622cb 100644
--- 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,108 @@ 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)
+ {
+ int save_errno;
+
+ /* close file upon error */
+ save_errno = errno;
+ close(fd);
+ errno = save_errno;
+
+ fprintf(stderr, _("%s: could not fsync file \"%s\": %s\n"),
+ progname, newfile, strerror(errno));
+ return -1;
+ }
+ }
+
+ /* 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 d3794df..1cb263d 100644
--- 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 */
--
2.10.0
From 1f7f5a664d15065aee2a431d1e1a3a3657dbd4f3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 13 Sep 2016 13:07:12 +0900
Subject: [PATCH 3/4] Add --no-sync option to pg_basebackup
This makes pg_basebackup, and is useful for testing.
---
doc/src/sgml/ref/pg_basebackup.sgml | 15 +++++++++++++++
src/bin/pg_basebackup/pg_basebackup.c | 28 +++++++++++++++++++---------
src/bin/pg_basebackup/pg_receivexlog.c | 1 +
src/bin/pg_basebackup/receivelog.c | 20 +++++++++++---------
src/bin/pg_basebackup/receivelog.h | 4 +++-
5 files changed, 49 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml
index 9f1eae1..3f1938a 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -439,6 +439,21 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-N</option></term>
+ <term><option>--no-sync</option></term>
+ <listitem>
+ <para>
+ By default, <command>pg_basebackup</command> will wait for all files
+ to be written safely to disk. This option causes
+ <command>pg_basebackup</command> to return without waiting, which is
+ faster, but means that a subsequent operating system crash can leave
+ the base backup corrupt. Generally, this option is useful for testing
+ but should not be used when creating a production installation.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>-v</option></term>
<term><option>--verbose</option></term>
<listitem>
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 2266e34..e2f1629 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -67,6 +67,7 @@ static bool includewal = false;
static bool streamwal = false;
static bool fastcheckpoint = false;
static bool writerecoveryconf = false;
+static bool do_sync = true;
static int standby_message_timeout = 10 * 1000; /* 10 sec = default */
static pg_time_t last_progress_report = 0;
static int32 maxrate = 0; /* no limit by default */
@@ -326,6 +327,7 @@ usage(void)
printf(_(" -c, --checkpoint=fast|spread\n"
" set fast or spread checkpointing\n"));
printf(_(" -l, --label=LABEL set backup label\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written safely to disk\n"));
printf(_(" -n, --noclean do not clean up after errors\n"));
printf(_(" -P, --progress show progress information\n"));
printf(_(" -v, --verbose output verbose messages\n"));
@@ -458,6 +460,7 @@ LogStreamerMain(logstreamer_param *param)
stream.stream_stop = reached_end_position;
stream.standby_message_timeout = standby_message_timeout;
stream.synchronous = false;
+ stream.do_sync = do_sync;
stream.mark_done = true;
stream.basedir = param->xlogdir;
stream.partial_suffix = NULL;
@@ -1197,7 +1200,7 @@ ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
PQfreemem(copybuf);
/* sync the resulting tar file, errors are not considered fatal */
- if (strcmp(basedir, "-") != 0)
+ if (do_sync && strcmp(basedir, "-") != 0)
(void) fsync_fname(filename, false, progname);
}
@@ -1965,14 +1968,17 @@ BaseBackup(void)
* all the data of the base directory is synced, taking into account
* all the tablespaces. Errors are not considered fatal.
*/
- if (format == 't')
+ if (do_sync)
{
- if (strcmp(basedir, "-") != 0)
- (void) fsync_fname(basedir, true, progname);
- }
- else
- {
- (void) fsync_pgdata(basedir, progname);
+ if (format == 't')
+ {
+ if (strcmp(basedir, "-") != 0)
+ (void) fsync_fname(basedir, true, progname);
+ }
+ else
+ {
+ (void) fsync_pgdata(basedir, progname);
+ }
}
if (verbose)
@@ -1999,6 +2005,7 @@ main(int argc, char **argv)
{"compress", required_argument, NULL, 'Z'},
{"label", required_argument, NULL, 'l'},
{"noclean", no_argument, NULL, 'n'},
+ {"no-sync", no_argument, NULL, 'N'},
{"dbname", required_argument, NULL, 'd'},
{"host", required_argument, NULL, 'h'},
{"port", required_argument, NULL, 'p'},
@@ -2035,7 +2042,7 @@ main(int argc, char **argv)
atexit(cleanup_directories_atexit);
- while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nzZ:d:c:h:p:U:s:S:wWvP",
+ while ((c = getopt_long(argc, argv, "D:F:r:RT:xX:l:nNzZ:d:c:h:p:U:s:S:wWvP",
long_options, &option_index)) != -1)
{
switch (c)
@@ -2059,6 +2066,9 @@ main(int argc, char **argv)
case 'r':
maxrate = parse_max_rate(optarg);
break;
+ case 'N':
+ do_sync = false;
+ break;
case 'R':
writerecoveryconf = true;
break;
diff --git a/src/bin/pg_basebackup/pg_receivexlog.c b/src/bin/pg_basebackup/pg_receivexlog.c
index 7f7ee9d..a58a251 100644
--- a/src/bin/pg_basebackup/pg_receivexlog.c
+++ b/src/bin/pg_basebackup/pg_receivexlog.c
@@ -336,6 +336,7 @@ StreamLog(void)
stream.stream_stop = stop_streaming;
stream.standby_message_timeout = standby_message_timeout;
stream.synchronous = synchronous;
+ stream.do_sync = true;
stream.mark_done = false;
stream.basedir = basedir;
stream.partial_suffix = ".partial";
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 0a740ce..0c01bfc 100644
--- a/src/bin/pg_basebackup/receivelog.c
+++ b/src/bin/pg_basebackup/receivelog.c
@@ -53,7 +53,7 @@ static bool ReadEndOfStreamingResult(PGresult *res, XLogRecPtr *startpos,
uint32 *timeline);
static bool
-mark_file_as_archived(const char *basedir, const char *fname)
+mark_file_as_archived(const char *basedir, const char *fname, bool do_sync)
{
int fd;
static char tmppath[MAXPGPATH];
@@ -71,10 +71,10 @@ mark_file_as_archived(const char *basedir, const char *fname)
close(fd);
- if (fsync_fname(tmppath, false, progname) != 0)
+ if (do_sync && fsync_fname(tmppath, false, progname) != 0)
return false;
- if (fsync_parent_path(tmppath, progname) != 0)
+ if (do_sync && fsync_parent_path(tmppath, progname) != 0)
return false;
return true;
@@ -135,9 +135,9 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
* fsync, in case of a previous crash between padding and fsyncing the
* file.
*/
- if (fsync_fname(fn, false, progname) != 0)
+ if (stream->do_sync && fsync_fname(fn, false, progname) != 0)
return false;
- if (fsync_parent_path(fn, progname) != 0)
+ if (stream->do_sync && fsync_parent_path(fn, progname) != 0)
return false;
return true;
@@ -174,9 +174,9 @@ open_walfile(StreamCtl *stream, XLogRecPtr startpoint)
* using synchronous mode, where the file is modified and fsynced
* in-place, without a directory fsync.
*/
- if (fsync_fname(fn, false, progname) != 0)
+ if (stream->do_sync && fsync_fname(fn, false, progname) != 0)
return false;
- if (fsync_parent_path(fn, progname) != 0)
+ if (stream->do_sync && fsync_parent_path(fn, progname) != 0)
return false;
if (lseek(f, SEEK_SET, 0) != 0)
@@ -259,7 +259,8 @@ close_walfile(StreamCtl *stream, XLogRecPtr pos)
if (currpos == XLOG_SEG_SIZE && stream->mark_done)
{
/* writes error message if failed */
- if (!mark_file_as_archived(stream->basedir, current_walfile_name))
+ if (!mark_file_as_archived(stream->basedir, current_walfile_name,
+ stream->do_sync))
return false;
}
@@ -379,7 +380,8 @@ writeTimeLineHistoryFile(StreamCtl *stream, char *filename, char *content)
if (stream->mark_done)
{
/* writes error message if failed */
- if (!mark_file_as_archived(stream->basedir, histfname))
+ if (!mark_file_as_archived(stream->basedir, histfname,
+ stream->do_sync))
return false;
}
diff --git a/src/bin/pg_basebackup/receivelog.h b/src/bin/pg_basebackup/receivelog.h
index 554ff8b..7a3bbc5 100644
--- a/src/bin/pg_basebackup/receivelog.h
+++ b/src/bin/pg_basebackup/receivelog.h
@@ -34,8 +34,10 @@ typedef struct StreamCtl
* timeline */
int standby_message_timeout; /* Send status messages this
* often */
- bool synchronous; /* Flush data on write */
+ bool synchronous; /* Flush immediately WAL data on write */
bool mark_done; /* Mark segment as done in generated archive */
+ bool do_sync; /* Flush to disk to ensure consistent state
+ * of data */
stream_stop_callback stream_stop; /* Stop streaming when returns true */
--
2.10.0
From ec8fcb31c71b354fe6c29124bbe2757765911235 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 13 Sep 2016 13:10:45 +0900
Subject: [PATCH 4/4] Switch pg_basebackup commands in Postgres.pm to use
--no-sync
On low-spec machines, this greatly reduces the I/O pressure induced by the
tests.
---
src/test/perl/PostgresNode.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index fede1e6..7210f30 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -476,7 +476,7 @@ sub backup
print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
TestLib::system_or_bail('pg_basebackup', '-D', $backup_path, '-p', $port,
- '-x');
+ '-x', '--no-sync');
print "# Backup finished\n";
}
--
2.10.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
