On Fri, Nov 11, 2016 at 11:03 PM, Albe Laurenz <laurenz.a...@wien.gv.at> wrote:
> - In pg_dumpall.c, the result of fsync_fname() is cast to "void" to show that
> the return code is ignored, but not anywhere else. Is that by design?
Right. The patch is lacking consistency in this area. The main thought
regarding this design is to not consider a fsync failure as critical,
and just issue a warning in stderr. I have updated the two other call
sites with a (void) cast.
> - For pg_dumpall, a short option "-N" is added for "--no-sync", but not for
> pg_dump (because -N is already taken there).
> I'd opt for either using the same short option for both or (IMO better)
> only offering a long option for both.
Okay. For consistency's sake let's do that. I was a bit hesitant
regarding that to be honest.
> This would avoid confusion, and we expect that few people will want to use
> this option anyway, right?
Definitely a good point.
--
Michael
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 371a614..dcad095 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -815,6 +815,20 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>--no-sync</option></term>
+ <listitem>
+ <para>
+ By default, <command>pg_dump</command> will wait for all files
+ to be written safely to disk. This option causes
+ <command>pg_dump</command> to return without waiting, which is
+ faster, but means that a subsequent operating system crash can leave
+ the dump corrupt. Generally, this option is useful for testing
+ but should not be used when dumping data from production installation.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--quote-all-identifiers</></term>
<listitem>
<para>
diff --git a/doc/src/sgml/ref/pg_dumpall.sgml b/doc/src/sgml/ref/pg_dumpall.sgml
index 97168a0..4e6839b 100644
--- a/doc/src/sgml/ref/pg_dumpall.sgml
+++ b/doc/src/sgml/ref/pg_dumpall.sgml
@@ -365,6 +365,21 @@ PostgreSQL documentation
</varlistentry>
<varlistentry>
+ <term><option>-N</option></term>
+ <term><option>--no-sync</option></term>
+ <listitem>
+ <para>
+ By default, <command>pg_dumpall</command> will wait for all files
+ to be written safely to disk. This option causes
+ <command>pg_dumpall</command> to return without waiting, which is
+ faster, but means that a subsequent operating system crash can leave
+ the dump corrupt. Generally, this option is useful for testing
+ but should not be used when dumping data from production installation.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><option>--quote-all-identifiers</></term>
<listitem>
<para>
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 0a28124..becbc56 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -268,7 +268,7 @@ extern Archive *OpenArchive(const char *FileSpec, const
ArchiveFormat fmt);
/* Create a new archive */
extern Archive *CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
- const int compression, ArchiveMode mode,
+ const int compression, bool dosync, ArchiveMode mode,
SetupWorkerPtr setupDumpWorker);
/* The --list option */
diff --git a/src/bin/pg_dump/pg_backup_archiver.c
b/src/bin/pg_dump/pg_backup_archiver.c
index 0e20985..5b60da6 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -56,7 +56,8 @@ static const char *modulename = gettext_noop("archiver");
static ArchiveHandle *_allocAH(const char *FileSpec, const ArchiveFormat fmt,
- const int compression, ArchiveMode mode, SetupWorkerPtr
setupWorkerPtr);
+ const int compression, bool dosync, ArchiveMode mode,
+ SetupWorkerPtr setupWorkerPtr);
static void _getObjectDescription(PQExpBuffer buf, TocEntry *te,
ArchiveHandle *AH);
static void _printTocEntry(ArchiveHandle *AH, TocEntry *te, bool isData, bool
acl_pass);
@@ -202,10 +203,12 @@ setupRestoreWorker(Archive *AHX)
/* Public */
Archive *
CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
- const int compression, ArchiveMode mode, SetupWorkerPtr
setupDumpWorker)
+ const int compression, bool dosync, ArchiveMode mode,
+ SetupWorkerPtr setupDumpWorker)
{
- ArchiveHandle *AH = _allocAH(FileSpec, fmt, compression, mode,
setupDumpWorker);
+ ArchiveHandle *AH = _allocAH(FileSpec, fmt, compression, dosync,
+ mode,
setupDumpWorker);
return (Archive *) AH;
}
@@ -215,7 +218,7 @@ CreateArchive(const char *FileSpec, const ArchiveFormat fmt,
Archive *
OpenArchive(const char *FileSpec, const ArchiveFormat fmt)
{
- ArchiveHandle *AH = _allocAH(FileSpec, fmt, 0, archModeRead,
setupRestoreWorker);
+ ArchiveHandle *AH = _allocAH(FileSpec, fmt, 0, true, archModeRead,
setupRestoreWorker);
return (Archive *) AH;
}
@@ -2223,7 +2226,8 @@ _discoverArchiveFormat(ArchiveHandle *AH)
*/
static ArchiveHandle *
_allocAH(const char *FileSpec, const ArchiveFormat fmt,
- const int compression, ArchiveMode mode, SetupWorkerPtr
setupWorkerPtr)
+ const int compression, bool dosync, ArchiveMode mode,
+ SetupWorkerPtr setupWorkerPtr)
{
ArchiveHandle *AH;
@@ -2277,6 +2281,7 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt,
AH->mode = mode;
AH->compression = compression;
+ AH->dosync = dosync;
memset(&(AH->sqlparse), 0, sizeof(AH->sqlparse));
diff --git a/src/bin/pg_dump/pg_backup_archiver.h
b/src/bin/pg_dump/pg_backup_archiver.h
index 50cf452..4492542 100644
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -307,6 +307,7 @@ struct _archiveHandle
* values for
compression: -1
*
Z_DEFAULT_COMPRESSION 0 COMPRESSION_NONE
* 1-9 levels
for gzip compression */
+ bool dosync; /* data requested to be synced
on sight */
ArchiveMode mode; /* File mode - r or w */
void *formatData; /* Header data specific to file format
*/
diff --git a/src/bin/pg_dump/pg_backup_custom.c
b/src/bin/pg_dump/pg_backup_custom.c
index 5388c08..99998a9 100644
--- a/src/bin/pg_dump/pg_backup_custom.c
+++ b/src/bin/pg_dump/pg_backup_custom.c
@@ -28,6 +28,7 @@
#include "compress_io.h"
#include "parallel.h"
#include "pg_backup_utils.h"
+#include "common/file_utils.h"
/*--------
* Routines in the format interface
@@ -721,6 +722,10 @@ _CloseArchive(ArchiveHandle *AH)
if (fclose(AH->FH) != 0)
exit_horribly(modulename, "could not close archive file: %s\n",
strerror(errno));
+ /* Sync the output file if one is defined */
+ if (AH->dosync && AH->mode == archModeWrite && AH->fSpec)
+ (void) fsync_fname(AH->fSpec, false, progname);
+
AH->FH = NULL;
}
diff --git a/src/bin/pg_dump/pg_backup_directory.c
b/src/bin/pg_dump/pg_backup_directory.c
index ae44371..f02fd33 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -37,6 +37,7 @@
#include "compress_io.h"
#include "parallel.h"
#include "pg_backup_utils.h"
+#include "common/file_utils.h"
#include <dirent.h>
#include <sys/stat.h>
@@ -593,6 +594,13 @@ _CloseArchive(ArchiveHandle *AH)
WriteDataChunks(AH, ctx->pstate);
ParallelBackupEnd(AH, ctx->pstate);
+
+ /*
+ * In directory mode, there is no need to sync all the entries
+ * individually. Just recurse once through all the files
generated.
+ */
+ if (AH->dosync)
+ fsync_dir_recurse(ctx->directory, progname);
}
AH->FH = NULL;
}
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 9cadd0c..a2b320f 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -33,6 +33,7 @@
#include "pg_backup_tar.h"
#include "pg_backup_utils.h"
#include "pgtar.h"
+#include "common/file_utils.h"
#include "fe_utils/string_utils.h"
#include <sys/stat.h>
@@ -901,6 +902,10 @@ _CloseArchive(ArchiveHandle *AH)
if (fputc(0, ctx->tarFH) == EOF)
WRITE_ERROR_EXIT;
}
+
+ /* Sync the output file if one is defined */
+ if (AH->dosync && AH->fSpec)
+ (void) fsync_fname(AH->fSpec, false, progname);
}
AH->FH = NULL;
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 4da297f..c2fdc13 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -92,6 +92,8 @@ typedef enum OidOptions
/* global decls */
bool g_verbose; /* User wants verbose narration
of our
* activities.
*/
+static bool dosync = true; /* Issue fsync() to make dump durable
+ * on disk. */
/* subquery used to convert user ID (eg, datdba) to user name */
static const char *username_subquery;
@@ -343,6 +345,7 @@ main(int argc, char **argv)
{"no-security-labels", no_argument, &dopt.no_security_labels,
1},
{"no-synchronized-snapshots", no_argument,
&dopt.no_synchronized_snapshots, 1},
{"no-unlogged-table-data", no_argument,
&dopt.no_unlogged_table_data, 1},
+ {"no-sync", no_argument, NULL, 7},
{NULL, 0, NULL, 0}
};
@@ -519,6 +522,10 @@ main(int argc, char **argv)
dumpsnapshot = pg_strdup(optarg);
break;
+ case 7: /* no-sync */
+ dosync = false;
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more
information.\n"), progname);
exit_nicely(1);
@@ -610,8 +617,8 @@ main(int argc, char **argv)
exit_horribly(NULL, "parallel backup only supported by the
directory format\n");
/* Open the output file */
- fout = CreateArchive(filename, archiveFormat, compressLevel,
archiveMode,
- setupDumpWorker);
+ fout = CreateArchive(filename, archiveFormat, compressLevel, dosync,
+ archiveMode, setupDumpWorker);
/* Make dump options accessible right away */
SetArchiveOptions(fout, &dopt, NULL);
@@ -859,6 +866,7 @@ help(const char *progname)
printf(_(" -V, --version output version information,
then exit\n"));
printf(_(" -Z, --compress=0-9 compression level for
compressed formats\n"));
printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for
a table lock\n"));
+ printf(_(" --no-sync do not wait for changes to be
written safely to disk\n"));
printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nOptions controlling the output content:\n"));
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 48bfca7..623d5ed 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -26,6 +26,7 @@
#include "dumputils.h"
#include "pg_backup.h"
+#include "common/file_utils.h"
#include "fe_utils/string_utils.h"
/* version string we expect back from pg_dump */
@@ -67,6 +68,7 @@ static PQExpBuffer pgdumpopts;
static char *connstr = "";
static bool skip_acls = false;
static bool verbose = false;
+static bool dosync = true;
static int binary_upgrade = 0;
static int column_inserts = 0;
@@ -126,6 +128,7 @@ main(int argc, char *argv[])
{"role", required_argument, NULL, 3},
{"use-set-session-authorization", no_argument,
&use_setsessauth, 1},
{"no-security-labels", no_argument, &no_security_labels, 1},
+ {"no-sync", no_argument, NULL, 4},
{"no-unlogged-table-data", no_argument,
&no_unlogged_table_data, 1},
{NULL, 0, NULL, 0}
@@ -193,7 +196,7 @@ main(int argc, char *argv[])
pgdumpopts = createPQExpBuffer();
- while ((c = getopt_long(argc, argv, "acd:f:gh:l:oOp:rsS:tU:vwWx",
long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "acd:f:gh:l:NoOp:rsS:tU:vwWx",
long_options, &optindex)) != -1)
{
switch (c)
{
@@ -295,6 +298,11 @@ main(int argc, char *argv[])
appendShellString(pgdumpopts, use_role);
break;
+ case 4:
+ dosync = false;
+ appendPQExpBufferStr(pgdumpopts, " --no-sync");
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more
information.\n"), progname);
exit_nicely(1);
@@ -528,8 +536,14 @@ main(int argc, char *argv[])
fprintf(OPF, "--\n-- PostgreSQL database cluster dump
complete\n--\n\n");
if (filename)
+ {
fclose(OPF);
+ /* sync the resulting file, errors are not fatal */
+ if (dosync)
+ (void) fsync_fname(filename, false, progname);
+ }
+
exit_nicely(0);
}
@@ -543,6 +557,7 @@ help(void)
printf(_("\nGeneral options:\n"));
printf(_(" -f, --file=FILENAME output file name\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be
written safely to disk\n"));
printf(_(" -V, --version output version information,
then exit\n"));
printf(_(" --lock-wait-timeout=TIMEOUT fail after waiting TIMEOUT for
a table lock\n"));
printf(_(" -?, --help show this help, then exit\n"));
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index 2fdb469..bcc3442 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -116,6 +116,25 @@ fsync_pgdata(const char *pg_data,
}
/*
+ * Issue fsync recursively on the given directory and all its contents.
+ *
+ * This is a convenient wrapper on top of walkdir().
+ */
+void
+fsync_dir_recurse(const char *dir, const char *progname)
+{
+ /*
+ * 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(dir, pre_sync_fname, false, progname);
+#endif
+
+ walkdir(dir, fsync_fname, false, progname);
+}
+
+/*
* walkdir: recursively walk a directory, applying the action to each
* regular file and directory (including the named directory itself).
*
diff --git a/src/include/common/file_utils.h b/src/include/common/file_utils.h
index b83c398..87a1df4 100644
--- a/src/include/common/file_utils.h
+++ b/src/include/common/file_utils.h
@@ -19,6 +19,7 @@ extern int fsync_fname(const char *fname, bool isdir,
const char *progname);
extern void fsync_pgdata(const char *pg_data, const char *progname,
int serverVersion);
+extern void fsync_dir_recurse(const char *dir, 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);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
