From 761792297fffbcabf356cdcb5471845845e54ca7 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Fri, 14 Jul 2023 21:51:44 +1200 Subject: [PATCH 2/2] Remove fsync_writethrough, add fsync=full (macOS only). Previously, wal_sync_method=fsync_writethrough affected pg_fsync() of WAL, control, data and other files, by causing pg_fsync() to use the fcntl(F_FULLFSYNC) call that Apple recommends for databases. That's a little confusing, because the GUC purports to affect only WAL files, and it was also not used by default, exposing users to data loss risk. New approach: 1. wal_sync_method=fsync_writethrough is removed. Instead, we will make the existing fsync and fdatasync methods do what Apple recommends. 2. Introduce a new setting fsync=full. This refactoring reflects the fact that all pg_fsync() calls are affected, not just those for WAL files. According to expectations established by other modern platforms, fcntl(F_FULLFSYNC) is the "true" fsync operation on this platform, and fsync=full will select that. The traditional fsync() system call is still available by setting fsync=on. 3. Use fcntl(F_FULLSYNC) for pg_fdatasync() too, if fsync=full. Apple doesn't exactly implement fdatasync(). It's not documented, but exists in some partially implemented form, without a declaration. As far as we can tell, it behaves just like fsync(). We'll still use it if you ask for it with fsync=on. 4. Use fsync=full by default on Macs, following Apple's recommendation for applications such as databases. It is not available on non-Macs, to avoid confusing people on other operating systems. Since wal_sync_method's default was changed to fdatasync by an earlier commit, the net effect is that Macs now use F_FULLSYNC for all file synchronization (data, control, WAL, ...) by default. While here, also get rid of configure probes for F_FULLFSYNC, and just test for its existence with #ifdef. --- doc/src/sgml/config.sgml | 31 ++++++-- doc/src/sgml/monitoring.sgml | 8 +- doc/src/sgml/wal.sgml | 14 +--- meson.build | 1 - src/backend/access/transam/xlog.c | 18 +---- src/backend/storage/file/fd.c | 74 +++++++------------ src/backend/storage/sync/sync.c | 2 +- src/backend/utils/init/globals.c | 2 +- src/backend/utils/misc/guc_tables.c | 41 +++++++--- src/backend/utils/misc/postgresql.conf.sample | 2 +- src/bin/pg_test_fsync/pg_test_fsync.c | 33 ++------- src/include/access/xlog.h | 3 +- src/include/miscadmin.h | 15 +++- src/include/port/darwin.h | 5 -- src/include/storage/fd.h | 2 - src/tools/msvc/Solution.pm | 1 - 16 files changed, 118 insertions(+), 134 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index acb6666e0b..b34d3901f2 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2898,7 +2898,7 @@ include_dir 'conf.d' - fsync (boolean) + fsync (enum) fsync configuration parameter @@ -2913,6 +2913,22 @@ include_dir 'conf.d' consistent state after an operating system or hardware crash. + + On macOS only, the parameter can also be set to + full, and that is the default. + This value causes PostgreSQL to replace all + calls to fsync() with + fcntl(fd, F_FULLFSYNC), as recommended by + Apple's documentation. It also does the same for calls to + fdatasync(), a function that exists but is not + documented by Apple. + Setting it to on causes macOS's regular + fsync() and fdatasync() + functions to be used as on other platforms. They are known to be + considerably faster, but users should consult the warnings in Apple's + manual page for fsync first. + + While turning off fsync is often a performance benefit, this can result in unrecoverable data corruption in @@ -3144,11 +3160,6 @@ include_dir 'conf.d' - - fsync_writethrough (call fsync() at each commit, forcing write-through of any disk write cache) - - - open_sync (write WAL files with open() option O_SYNC) @@ -3165,6 +3176,14 @@ include_dir 'conf.d' This parameter can only be set in the postgresql.conf file or on the server command line. + + Note that on macOS, the setting can modify + the behavior of the fdatasync and + fsync levels, since + it can cause all calls to fdatasync() and + fsync() to be replaced with an Apple-specific + fcntl() call. + diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 588b720f57..87dd026bc9 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3055,8 +3055,8 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i issue_xlog_fsync request (if is on and is either - fdatasync, fsync or - fsync_writethrough, otherwise zero). + fdatasync or fsync, + otherwise zero). See for more information about the internal WAL function issue_xlog_fsync. @@ -3086,8 +3086,8 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i (if track_wal_io_timing is enabled, fsync is on, and wal_sync_method is either - fdatasync, fsync or - fsync_writethrough, otherwise zero). + fdatasync or fsync, + otherwise zero). diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml index 4aad0e1a07..342fc4c6c8 100644 --- a/doc/src/sgml/wal.sgml +++ b/doc/src/sgml/wal.sgml @@ -114,12 +114,6 @@ - - - On macOS, write caching can be prevented by - setting wal_sync_method to fsync_writethrough. - - @@ -758,10 +752,6 @@ The parameter determines how PostgreSQL will ask the kernel to force WAL updates out to disk. - All the options should be the same in terms of reliability, with - the exception of fsync_writethrough, which can sometimes - force a flush of the disk cache even when other options do not do so. - However, it's quite platform-specific which one will be the fastest. You can test the speeds of different options using the program. Note that this parameter is irrelevant if fsync @@ -795,8 +785,8 @@ open_datasync or open_sync, a write operation in XLogWrite guarantees to sync written WAL data to disk and issue_xlog_fsync does nothing. - If wal_sync_method is either fdatasync, - fsync, or fsync_writethrough, + If wal_sync_method is either fdatasync + or fsync, the write operation moves WAL buffers to kernel cache and issue_xlog_fsync syncs them to disk. Regardless of the setting of track_wal_io_timing, the number diff --git a/meson.build b/meson.build index 04ea348852..c37332b4d5 100644 --- a/meson.build +++ b/meson.build @@ -2163,7 +2163,6 @@ endforeach decl_checks = [ - ['F_FULLFSYNC', 'fcntl.h'], ['fdatasync', 'unistd.h'], ['posix_fadvise', 'fcntl.h'], ['strlcat', 'string.h'], diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 8b0710abe6..d7241766de 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -173,9 +173,6 @@ static bool check_wal_consistency_checking_deferred = false; */ const struct config_enum_entry sync_method_options[] = { {"fsync", SYNC_METHOD_FSYNC, false}, -#ifdef HAVE_FSYNC_WRITETHROUGH - {"fsync_writethrough", SYNC_METHOD_FSYNC_WRITETHROUGH, false}, -#endif {"fdatasync", SYNC_METHOD_FDATASYNC, false}, #ifdef O_SYNC {"open_sync", SYNC_METHOD_OPEN, false}, @@ -2613,7 +2610,7 @@ XLogFlush(XLogRecPtr record) * We do not sleep if enableFsync is not turned on, nor if there are * fewer than CommitSiblings other backends with active transactions. */ - if (CommitDelay > 0 && enableFsync && + if (CommitDelay > 0 && enableFsync != ENABLE_FSYNC_OFF && MinimumActiveBackends(CommitSiblings)) { pg_usleep(CommitDelay); @@ -8084,7 +8081,7 @@ get_sync_bit(int method) o_direct_flag = PG_O_DIRECT; /* If fsync is disabled, never open in sync mode */ - if (!enableFsync) + if (enableFsync == ENABLE_FSYNC_OFF) return o_direct_flag; switch (method) @@ -8096,7 +8093,6 @@ get_sync_bit(int method) * be seen here. */ case SYNC_METHOD_FSYNC: - case SYNC_METHOD_FSYNC_WRITETHROUGH: case SYNC_METHOD_FDATASYNC: return o_direct_flag; #ifdef O_SYNC @@ -8171,7 +8167,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli) * Quick exit if fsync is disabled or write() has already synced the WAL * file. */ - if (!enableFsync || + if (enableFsync == ENABLE_FSYNC_OFF || sync_method == SYNC_METHOD_OPEN || sync_method == SYNC_METHOD_OPEN_DSYNC) return; @@ -8186,15 +8182,9 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli) switch (sync_method) { case SYNC_METHOD_FSYNC: - if (pg_fsync_no_writethrough(fd) != 0) + if (pg_fsync(fd) != 0) msg = _("could not fsync file \"%s\": %m"); break; -#ifdef HAVE_FSYNC_WRITETHROUGH - case SYNC_METHOD_FSYNC_WRITETHROUGH: - if (pg_fsync_writethrough(fd) != 0) - msg = _("could not fsync write-through file \"%s\": %m"); - break; -#endif case SYNC_METHOD_FDATASYNC: if (pg_fdatasync(fd) != 0) msg = _("could not fdatasync file \"%s\": %m"); diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c index a027a8aabc..f8df7dbeae 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -355,11 +355,12 @@ static int fsync_parent_path(const char *fname, int elevel); /* - * pg_fsync --- do fsync with or without writethrough + * pg_fsync --- do fsync, if enabled, or redirect to Apple's F_FULLFSYNC. */ int pg_fsync(int fd) { + int rc; #if !defined(WIN32) && defined(USE_ASSERT_CHECKING) struct stat st; @@ -398,30 +399,20 @@ pg_fsync(int fd) errno = 0; #endif - /* #if is to skip the sync_method test if there's no need for it */ -#if defined(HAVE_FSYNC_WRITETHROUGH) - if (sync_method == SYNC_METHOD_FSYNC_WRITETHROUGH) - return pg_fsync_writethrough(fd); - else -#endif - return pg_fsync_no_writethrough(fd); -} - - -/* - * pg_fsync_no_writethrough --- same as fsync except does nothing if - * enableFsync is off - */ -int -pg_fsync_no_writethrough(int fd) -{ - int rc; - - if (!enableFsync) + if (enableFsync == ENABLE_FSYNC_OFF) return 0; retry: - rc = fsync(fd); +#if defined(ENABLE_FSYNC_FULL) + if (enableFsync == ENABLE_FSYNC_FULL) + { + rc = fcntl(fd, F_FULLFSYNC); + } + else +#endif + { + rc = fsync(fd); + } if (rc == -1 && errno == EINTR) goto retry; @@ -430,37 +421,28 @@ retry: } /* - * pg_fsync_writethrough - */ -int -pg_fsync_writethrough(int fd) -{ - if (enableFsync) - { -#if defined(F_FULLFSYNC) - return (fcntl(fd, F_FULLFSYNC, 0) == -1) ? -1 : 0; -#else - errno = ENOSYS; - return -1; -#endif - } - else - return 0; -} - -/* - * pg_fdatasync --- same as fdatasync except does nothing if enableFsync is off + * pg_fdatasync --- same as fdatasync except does nothing if enableFsync is off, + * and redirects to Apple's F_FULLSYNC if configured. */ int pg_fdatasync(int fd) { int rc; - if (!enableFsync) + if (enableFsync == ENABLE_FSYNC_OFF) return 0; retry: - rc = fdatasync(fd); +#if defined(ENABLE_FSYNC_FULL) + if (enableFsync == ENABLE_FSYNC_FULL) + { + rc = fcntl(fd, F_FULLFSYNC); + } + else +#endif + { + rc = fdatasync(fd); + } if (rc == -1 && errno == EINTR) goto retry; @@ -482,7 +464,7 @@ pg_flush_data(int fd, off_t offset, off_t nbytes) * if fsyncs are disabled - that's a decision we might want to make * configurable at some point. */ - if (!enableFsync) + if (enableFsync == ENABLE_FSYNC_OFF) return; /* @@ -3491,7 +3473,7 @@ SyncDataDirectory(void) bool xlog_is_symlink; /* We can skip this whole thing if fsync is disabled. */ - if (!enableFsync) + if (enableFsync == ENABLE_FSYNC_OFF) return; /* diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c index 04fcb06056..1f8d368e08 100644 --- a/src/backend/storage/sync/sync.c +++ b/src/backend/storage/sync/sync.c @@ -386,7 +386,7 @@ ProcessSyncRequests(void) * all. (We delay checking until this point so that changing fsync on * the fly behaves sensibly.) */ - if (enableFsync) + if (enableFsync != ENABLE_FSYNC_OFF) { /* * If in checkpointer, we want to absorb pending requests every so diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index 011ec18015..78d5ef6a63 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -120,7 +120,7 @@ int DateStyle = USE_ISO_DATES; int DateOrder = DATEORDER_MDY; int IntervalStyle = INTSTYLE_POSTGRES; -bool enableFsync = true; +int enableFsync = DEFAULT_ENABLE_FSYNC; bool allowSystemTableMods = false; int work_mem = 4096; double hash_mem_multiplier = 2.0; diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 93dc2e7680..df44f853d4 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -477,6 +477,22 @@ static const struct config_enum_entry wal_compression_options[] = { {NULL, 0, false} }; +static const struct config_enum_entry enable_fsync_options[] = { + {"on", ENABLE_FSYNC_ON, false}, + {"off", ENABLE_FSYNC_OFF, false}, + {"true", ENABLE_FSYNC_ON, true}, + {"false", ENABLE_FSYNC_OFF, true}, + {"yes", ENABLE_FSYNC_ON, true}, + {"no", ENABLE_FSYNC_OFF, true}, + {"1", ENABLE_FSYNC_ON, true}, + {"0", ENABLE_FSYNC_OFF, true}, +#ifdef ENABLE_FSYNC_FULL + {"full", ENABLE_FSYNC_FULL, false}, +#endif + {NULL, 0, false} +}; + + /* * Options for enum values stored in other modules */ @@ -1095,18 +1111,6 @@ struct config_bool ConfigureNamesBool[] = true, NULL, NULL, NULL }, - { - {"fsync", PGC_SIGHUP, WAL_SETTINGS, - gettext_noop("Forces synchronization of updates to disk."), - gettext_noop("The server will use the fsync() system call in several places to make " - "sure that updates are physically written to disk. This insures " - "that a database cluster will recover to a consistent state after " - "an operating system or hardware crash.") - }, - &enableFsync, - true, - NULL, NULL, NULL - }, { {"ignore_checksum_failure", PGC_SUSET, DEVELOPER_OPTIONS, gettext_noop("Continues processing after a checksum failure."), @@ -4643,6 +4647,19 @@ struct config_enum ConfigureNamesEnum[] = NULL, NULL, NULL }, + { + {"fsync", PGC_SIGHUP, WAL_SETTINGS, + gettext_noop("Forces synchronization of updates to disk."), + gettext_noop("Whether to use syncronization APIs to make " + "sure that updates are physically written to disk. This insures " + "that a database cluster will recover to a consistent state after " + "an operating system or hardware crash.") + }, + &enableFsync, + DEFAULT_ENABLE_FSYNC, enable_fsync_options, + NULL, NULL, NULL + }, + { {"transaction_isolation", PGC_USERSET, CLIENT_CONN_STATEMENT, gettext_noop("Sets the current transaction's isolation level."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index d466143e4a..f2ec3b68dd 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -215,7 +215,7 @@ # unrecoverable data corruption) #synchronous_commit = on # synchronization level; # off, local, remote_write, remote_apply, or on -#wal_sync_method = fdatasync # fsync, fdatasync, fsync_writethrough, +#wal_sync_method = fdatasync # fsync, fdatasync, # open_sync, open_datasync #full_page_writes = on # recover from partial page writes #wal_log_hints = off # also do full page writes of non-critical updates diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c index 5b431d2f99..35bc604cf1 100644 --- a/src/bin/pg_test_fsync/pg_test_fsync.c +++ b/src/bin/pg_test_fsync/pg_test_fsync.c @@ -87,9 +87,6 @@ static DWORD WINAPI process_alarm(LPVOID param); #endif static void signal_cleanup(SIGNAL_ARGS); -#ifdef HAVE_FSYNC_WRITETHROUGH -static int pg_fsync_writethrough(int fd); -#endif static void print_elapse(struct timeval start_t, struct timeval stop_t, int ops); #define die(msg) pg_fatal("%s: %m", _(msg)) @@ -292,7 +289,7 @@ test_sync(int writes_per_op) printf(_("\nCompare file sync methods using one %dkB write:\n"), XLOG_BLCKSZ_K); else printf(_("\nCompare file sync methods using two %dkB writes:\n"), XLOG_BLCKSZ_K); - printf(_("(fdatasync is the default)\n")); + printf(_("(fdatasync is the default, but see docs for macOS)\n")); /* * Test open_datasync if available @@ -371,12 +368,13 @@ test_sync(int writes_per_op) close(tmpfile); /* - * If fsync_writethrough is available, test as well + * Test the macOS fcntl that use instead of fsync/fdatasync levels if + * fsync=full. */ - printf(LABEL_FORMAT, "fsync_writethrough"); + printf(LABEL_FORMAT, "fcntl(F_FULLFSYNC)"); fflush(stdout); -#ifdef HAVE_FSYNC_WRITETHROUGH +#ifdef F_FULLFSYNC if ((tmpfile = open(filename, O_RDWR | PG_BINARY, 0)) == -1) die("could not open output file"); START_TIMER; @@ -388,8 +386,8 @@ test_sync(int writes_per_op) XLOG_BLCKSZ, writes * XLOG_BLCKSZ) != XLOG_BLCKSZ) die("write failed"); - if (pg_fsync_writethrough(tmpfile) != 0) - die("fsync failed"); + if (fcntl(tmpfile, F_FULLFSYNC) != 0) + die("fcntl failed"); } STOP_TIMER; close(tmpfile); @@ -504,8 +502,7 @@ test_file_descriptor_sync(void) /* * Test whether fsync can sync data written on a different descriptor for * the same file. This checks the efficiency of multi-process fsyncs - * against the same file. Possibly this should be done with writethrough - * on platforms which support it. + * against the same file. */ printf(_("\nTest if fsync on non-write file descriptor is honored:\n")); printf(_("(If the times are similar, fsync() can sync data written on a different\n" @@ -600,20 +597,6 @@ signal_cleanup(SIGNAL_ARGS) exit(1); } -#ifdef HAVE_FSYNC_WRITETHROUGH - -static int -pg_fsync_writethrough(int fd) -{ -#if defined(F_FULLFSYNC) - return (fcntl(fd, F_FULLFSYNC, 0) == -1) ? -1 : 0; -#else - errno = ENOSYS; - return -1; -#endif -} -#endif - /* * print out the writes per second for tests */ diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 48ca852381..0386c2c7a3 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -22,8 +22,7 @@ #define SYNC_METHOD_FSYNC 0 #define SYNC_METHOD_FDATASYNC 1 #define SYNC_METHOD_OPEN 2 /* for O_SYNC */ -#define SYNC_METHOD_FSYNC_WRITETHROUGH 3 -#define SYNC_METHOD_OPEN_DSYNC 4 /* for O_DSYNC */ +#define SYNC_METHOD_OPEN_DSYNC 3 /* for O_DSYNC */ extern PGDLLIMPORT int sync_method; extern PGDLLIMPORT XLogRecPtr ProcLastRecPtr; diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 14bd574fc2..26d2ad38b7 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -23,6 +23,7 @@ #ifndef MISCADMIN_H #define MISCADMIN_H +#include #include #include "datatype/timestamp.h" /* for TimestampTz */ @@ -256,7 +257,19 @@ extern PGDLLIMPORT int IntervalStyle; #define MAXTZLEN 10 /* max TZ name len, not counting tr. null */ -extern PGDLLIMPORT bool enableFsync; +#define ENABLE_FSYNC_OFF 0 +#define ENABLE_FSYNC_ON 1 +#ifdef F_FULLFSYNC +#define ENABLE_FSYNC_FULL 2 +#endif + +#ifdef ENABLE_FSYNC_FULL +#define DEFAULT_ENABLE_FSYNC ENABLE_FSYNC_FULL +#else +#define DEFAULT_ENABLE_FSYNC ENABLE_FSYNC_ON +#endif + +extern PGDLLIMPORT int enableFsync; extern PGDLLIMPORT bool allowSystemTableMods; extern PGDLLIMPORT int work_mem; extern PGDLLIMPORT double hash_mem_multiplier; diff --git a/src/include/port/darwin.h b/src/include/port/darwin.h index 15fb69d6db..41e2a427d3 100644 --- a/src/include/port/darwin.h +++ b/src/include/port/darwin.h @@ -1,8 +1,3 @@ /* src/include/port/darwin.h */ #define __darwin__ 1 - -#if HAVE_DECL_F_FULLFSYNC /* not present before macOS 10.3 */ -#define HAVE_FSYNC_WRITETHROUGH - -#endif diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index 6791a406fc..ed49795f97 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -183,8 +183,6 @@ extern void RemovePgTempFilesInDir(const char *tmpdirname, bool missing_ok, extern bool looks_like_temp_rel_name(const char *name); extern int pg_fsync(int fd); -extern int pg_fsync_no_writethrough(int fd); -extern int pg_fsync_writethrough(int fd); extern int pg_fdatasync(int fd); extern void pg_flush_data(int fd, off_t offset, off_t nbytes); extern int pg_truncate(const char *path, off_t length); diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm index 1cbc857e35..ee963e8351 100644 --- a/src/tools/msvc/Solution.pm +++ b/src/tools/msvc/Solution.pm @@ -232,7 +232,6 @@ sub GenerateFiles HAVE_CRTDEFS_H => undef, HAVE_CRYPTO_LOCK => undef, HAVE_DECL_FDATASYNC => 0, - HAVE_DECL_F_FULLFSYNC => 0, HAVE_DECL_LLVMCREATEGDBREGISTRATIONLISTENER => 0, HAVE_DECL_LLVMCREATEPERFJITEVENTLISTENER => 0, HAVE_DECL_LLVMGETHOSTCPUNAME => 0, -- 2.39.2