On Wed, Feb 12, 2020 at 9:54 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> In commit 3eb77eba we made it possible for any subsystem that wants a
> file to be flushed as part of the next checkpoint to ask the
> checkpointer to do that, as previously only md.c could do.
Hello,
While working on recovery performance, I found my way back to this
idea and rebased the patch.
Problem statement:
Every time we have to write out a page of pg_commit_ts, pg_multixact
or pg_xact due to cache pressure, we immediately call fsync(). This
runs serially in the recovery process, and it's quite bad for
pg_commit_ts, because we need to dump out a page for every ~800
transactions (track_commit_timestamps is not enabled by default). If
we ask the checkpointer to do it, it collapses the 2048 fsync calls
for each SLRU segment into one, and the kernel can write out the data
with larger I/Os, maybe even ahead of time, and update the inode only
once.
Experiment:
Run crash recovery for 1 million pgbench transactions:
postgres -D pgdata \
-c synchronous_commit=off \
-c enable_commit_timestamps=on \
-c max_wal_size=10GB \
-c checkpoint_timeout=60min
# in another shell
pgbench -i -s10 postgres
psql postgres -c checkpoint
pgbench -t1000000 -Mprepared postgres
killall -9 postgres
# save the crashed pgdata dir for repeated experiments
mv pgdata pgdata-save
# now run experiments like this and see how long recovery takes
rm -fr pgdata
cp -r pgdata-save pgdata
postgres -D pgdata
What I see on a system that has around 2.5ms latency for fsync:
master: 16.83 seconds
patched: 4.00 seconds
It's harder to see it without commit timestamps enabled since we only
need to flush a pg_xact page every 32k transactions (and multixacts
are more complicated to test), but you can still see the effect. With
8x more transactions to make it clearer what's going on, I could
measure a speedup of around 6% from this patch, which I suppose scales
up fairly obviously with storage latency (every million transaction =
at least 30 fsyncs stalls, so you can multiply that by your fsync
latency and work out how much time your recovery process will be
asleep at the wheel instead of applying your records).
>From a syscall overhead point of view, it's a bit unfortunate that we
open and close SLRU segments every time we write, but it's probably
not really enough to complain about... except for the (small) risk of
an inode dropping out of kernel caches in the time between closing it
and the checkpointer opening it. Hmm.
From 0fe8767316d5a973f43553080c3973759d83038d Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 4 Aug 2020 17:57:18 +1200
Subject: [PATCH v2] Ask the checkpointer to flush SLRU files.
Previously, we called fsync() after writing out pg_xact, multixact and
commit_ts pages, leading to an I/O stall in user backends and recovery.
Ask the checkpointer to collapse requests for the same file into a
single system call as part of the next checkpoint, as we do for relation
files.
Discussion: https://postgr.es/m/CA+hUKGLJ=84yt+nvhkeedauutvhmfq9i-n7k_o50jmq6rpj...@mail.gmail.com
---
src/backend/access/transam/clog.c | 13 +++-
src/backend/access/transam/commit_ts.c | 12 ++-
src/backend/access/transam/multixact.c | 24 +++++-
src/backend/access/transam/slru.c | 101 +++++++++++++++++++------
src/backend/access/transam/subtrans.c | 4 +-
src/backend/commands/async.c | 5 +-
src/backend/storage/lmgr/predicate.c | 4 +-
src/backend/storage/sync/sync.c | 24 +++++-
src/include/access/clog.h | 3 +
src/include/access/commit_ts.h | 3 +
src/include/access/multixact.h | 4 +
src/include/access/slru.h | 12 ++-
src/include/storage/sync.h | 7 +-
13 files changed, 172 insertions(+), 44 deletions(-)
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index f3da40ae01..0c5b7a525e 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -42,6 +42,7 @@
#include "pg_trace.h"
#include "pgstat.h"
#include "storage/proc.h"
+#include "storage/sync.h"
/*
* Defines for CLOG page sizes. A page is the same BLCKSZ as is used
@@ -692,7 +693,8 @@ CLOGShmemInit(void)
{
XactCtl->PagePrecedes = CLOGPagePrecedes;
SimpleLruInit(XactCtl, "Xact", CLOGShmemBuffers(), CLOG_LSNS_PER_PAGE,
- XactSLRULock, "pg_xact", LWTRANCHE_XACT_BUFFER);
+ XactSLRULock, "pg_xact", LWTRANCHE_XACT_BUFFER,
+ SYNC_HANDLER_CLOG);
}
/*
@@ -1034,3 +1036,12 @@ clog_redo(XLogReaderState *record)
else
elog(PANIC, "clog_redo: unknown op code %u", info);
}
+
+/*
+ * Entrypoint for sync.c to sync clog files.
+ */
+int
+clogsyncfiletag(const FileTag *ftag, char *path)
+{
+ return slrusyncfiletag(XactCtl, ftag, path);
+}
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 903280ae92..b4edbdb4e3 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -555,7 +555,8 @@ CommitTsShmemInit(void)
CommitTsCtl->PagePrecedes = CommitTsPagePrecedes;
SimpleLruInit(CommitTsCtl, "CommitTs", CommitTsShmemBuffers(), 0,
CommitTsSLRULock, "pg_commit_ts",
- LWTRANCHE_COMMITTS_BUFFER);
+ LWTRANCHE_COMMITTS_BUFFER,
+ SYNC_HANDLER_COMMIT_TS);
commitTsShared = ShmemInitStruct("CommitTs shared",
sizeof(CommitTimestampShared),
@@ -1083,3 +1084,12 @@ commit_ts_redo(XLogReaderState *record)
else
elog(PANIC, "commit_ts_redo: unknown op code %u", info);
}
+
+/*
+ * Entrypoint for sync.c to sync commit_ts files.
+ */
+int
+committssyncfiletag(const FileTag *ftag, char *path)
+{
+ return slrusyncfiletag(CommitTsCtl, ftag, path);
+}
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 475f5ed861..27ae2edbdc 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1831,11 +1831,13 @@ MultiXactShmemInit(void)
SimpleLruInit(MultiXactOffsetCtl,
"MultiXactOffset", NUM_MULTIXACTOFFSET_BUFFERS, 0,
MultiXactOffsetSLRULock, "pg_multixact/offsets",
- LWTRANCHE_MULTIXACTOFFSET_BUFFER);
+ LWTRANCHE_MULTIXACTOFFSET_BUFFER,
+ SYNC_HANDLER_MULTIXACT_OFFSET);
SimpleLruInit(MultiXactMemberCtl,
"MultiXactMember", NUM_MULTIXACTMEMBER_BUFFERS, 0,
MultiXactMemberSLRULock, "pg_multixact/members",
- LWTRANCHE_MULTIXACTMEMBER_BUFFER);
+ LWTRANCHE_MULTIXACTMEMBER_BUFFER,
+ SYNC_HANDLER_MULTIXACT_MEMBER);
/* Initialize our shared state struct */
MultiXactState = ShmemInitStruct("Shared MultiXact State",
@@ -3386,3 +3388,21 @@ pg_get_multixact_members(PG_FUNCTION_ARGS)
SRF_RETURN_DONE(funccxt);
}
+
+/*
+ * Entrypoint for sync.c to sync offsets files.
+ */
+int
+multixactoffsetssyncfiletag(const FileTag *ftag, char *path)
+{
+ return slrusyncfiletag(MultiXactOffsetCtl, ftag, path);
+}
+
+/*
+ * Entrypoint for sync.c to sync members files.
+ */
+int
+multixactmemberssyncfiletag(const FileTag *ftag, char *path)
+{
+ return slrusyncfiletag(MultiXactMemberCtl, ftag, path);
+}
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam/slru.c
index 9e145f1c36..db1243772f 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -80,6 +80,18 @@ typedef struct SlruFlushData
typedef struct SlruFlushData *SlruFlush;
+/*
+ * Populate a file tag describing a segment file. We only use the segment
+ * number, since we can derive everything else we need by having separate
+ * sync handler functions for clog, multixact etc.
+ */
+#define INIT_SLRUFILETAG(a,xx_handler,xx_segno) \
+( \
+ memset(&(a), 0, sizeof(FileTag)), \
+ (a).handler = (xx_handler), \
+ (a).segno = (xx_segno) \
+)
+
/*
* Macro to mark a buffer slot "most recently used". Note multiple evaluation
* of arguments!
@@ -173,7 +185,8 @@ SimpleLruShmemSize(int nslots, int nlsns)
*/
void
SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
- LWLock *ctllock, const char *subdir, int tranche_id)
+ LWLock *ctllock, const char *subdir, int tranche_id,
+ SyncRequestHandler sync_handler)
{
SlruShared shared;
bool found;
@@ -251,7 +264,7 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
* assume caller set PagePrecedes.
*/
ctl->shared = shared;
- ctl->do_fsync = true; /* default behavior */
+ ctl->sync_handler = sync_handler;
StrNCpy(ctl->Dir, subdir, sizeof(ctl->Dir));
}
@@ -870,23 +883,31 @@ SlruPhysicalWritePage(SlruCtl ctl, int pageno, int slotno, SlruFlush fdata)
}
pgstat_report_wait_end();
- /*
- * If not part of Flush, need to fsync now. We assume this happens
- * infrequently enough that it's not a performance issue.
- */
- if (!fdata)
+ /* Queue up a sync request for the checkpointer. */
+ if (ctl->sync_handler != SYNC_HANDLER_NONE)
{
- pgstat_report_wait_start(WAIT_EVENT_SLRU_SYNC);
- if (ctl->do_fsync && pg_fsync(fd) != 0)
+ FileTag tag;
+
+ INIT_SLRUFILETAG(tag, ctl->sync_handler, segno);
+ if (!RegisterSyncRequest(&tag, SYNC_REQUEST, false))
{
+ /* No space to enqueue sync request. Do it synchronously. */
+ pgstat_report_wait_start(WAIT_EVENT_SLRU_SYNC);
+ if (pg_fsync(fd) != 0)
+ {
+ pgstat_report_wait_end();
+ slru_errcause = SLRU_FSYNC_FAILED;
+ slru_errno = errno;
+ CloseTransientFile(fd);
+ return false;
+ }
pgstat_report_wait_end();
- slru_errcause = SLRU_FSYNC_FAILED;
- slru_errno = errno;
- CloseTransientFile(fd);
- return false;
}
- pgstat_report_wait_end();
+ }
+ /* Close file, unless part of flush request. */
+ if (!fdata)
+ {
if (CloseTransientFile(fd) != 0)
{
slru_errcause = SLRU_CLOSE_FAILED;
@@ -1162,21 +1183,11 @@ SimpleLruFlush(SlruCtl ctl, bool allow_redirtied)
LWLockRelease(shared->ControlLock);
/*
- * Now fsync and close any files that were open
+ * Now close any files that were open
*/
ok = true;
for (i = 0; i < fdata.num_files; i++)
{
- pgstat_report_wait_start(WAIT_EVENT_SLRU_FLUSH_SYNC);
- if (ctl->do_fsync && pg_fsync(fdata.fd[i]) != 0)
- {
- slru_errcause = SLRU_FSYNC_FAILED;
- slru_errno = errno;
- pageno = fdata.segno[i] * SLRU_PAGES_PER_SEGMENT;
- ok = false;
- }
- pgstat_report_wait_end();
-
if (CloseTransientFile(fdata.fd[i]) != 0)
{
slru_errcause = SLRU_CLOSE_FAILED;
@@ -1295,6 +1306,7 @@ SlruDeleteSegment(SlruCtl ctl, int segno)
int slotno;
char path[MAXPGPATH];
bool did_write;
+ FileTag tag;
/* Clean out any possibly existing references to the segment. */
LWLockAcquire(shared->ControlLock, LW_EXCLUSIVE);
@@ -1338,6 +1350,17 @@ restart:
snprintf(path, MAXPGPATH, "%s/%04X", ctl->Dir, segno);
ereport(DEBUG2,
(errmsg("removing file \"%s\"", path)));
+
+ /*
+ * Tell the checkpointer to forget any sync requests, before we unlink the
+ * file.
+ */
+ if (ctl->sync_handler != SYNC_HANDLER_NONE)
+ {
+ INIT_SLRUFILETAG(tag, ctl->sync_handler, segno);
+ RegisterSyncRequest(&tag, SYNC_FORGET_REQUEST, true);
+ }
+
unlink(path);
LWLockRelease(shared->ControlLock);
@@ -1436,3 +1459,31 @@ SlruScanDirectory(SlruCtl ctl, SlruScanCallback callback, void *data)
return retval;
}
+
+/*
+ * Individual SLRUs (clog, ...) have to provide a sync.c handler function so
+ * that they can provide the correct "SlruCtl" (otherwise we don't know how to
+ * build the path), but they just forward to this common implementation that
+ * performs the fsync.
+ */
+int
+slrusyncfiletag(SlruCtl ctl, const FileTag *ftag, char *path)
+{
+ int fd;
+ int save_errno;
+ int result;
+
+ SlruFileName(ctl, path, ftag->segno);
+
+ fd = OpenTransientFile(path, O_RDWR | PG_BINARY);
+ if (fd < 0)
+ return -1;
+
+ result = pg_fsync(fd);
+ save_errno = errno;
+
+ CloseTransientFile(fd);
+
+ errno = save_errno;
+ return result;
+}
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index f33ae407a6..68568f18dc 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -193,9 +193,7 @@ SUBTRANSShmemInit(void)
SubTransCtl->PagePrecedes = SubTransPagePrecedes;
SimpleLruInit(SubTransCtl, "Subtrans", NUM_SUBTRANS_BUFFERS, 0,
SubtransSLRULock, "pg_subtrans",
- LWTRANCHE_SUBTRANS_BUFFER);
- /* Override default assumption that writes should be fsync'd */
- SubTransCtl->do_fsync = false;
+ LWTRANCHE_SUBTRANS_BUFFER, SYNC_HANDLER_NONE);
}
/*
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 71b7577afc..d0a40b0556 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -554,9 +554,8 @@ AsyncShmemInit(void)
*/
NotifyCtl->PagePrecedes = asyncQueuePagePrecedes;
SimpleLruInit(NotifyCtl, "Notify", NUM_NOTIFY_BUFFERS, 0,
- NotifySLRULock, "pg_notify", LWTRANCHE_NOTIFY_BUFFER);
- /* Override default assumption that writes should be fsync'd */
- NotifyCtl->do_fsync = false;
+ NotifySLRULock, "pg_notify", LWTRANCHE_NOTIFY_BUFFER,
+ SYNC_HANDLER_NONE);
if (!found)
{
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index d24919f76b..579201b10d 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -821,9 +821,7 @@ SerialInit(void)
SerialSlruCtl->PagePrecedes = SerialPagePrecedesLogically;
SimpleLruInit(SerialSlruCtl, "Serial",
NUM_SERIAL_BUFFERS, 0, SerialSLRULock, "pg_serial",
- LWTRANCHE_SERIAL_BUFFER);
- /* Override default assumption that writes should be fsync'd */
- SerialSlruCtl->do_fsync = false;
+ LWTRANCHE_SERIAL_BUFFER, SYNC_HANDLER_NONE);
/*
* Create or attach to the SerialControl structure.
diff --git a/src/backend/storage/sync/sync.c b/src/backend/storage/sync/sync.c
index 3ded2cdd71..64107fd732 100644
--- a/src/backend/storage/sync/sync.c
+++ b/src/backend/storage/sync/sync.c
@@ -18,6 +18,9 @@
#include <fcntl.h>
#include <sys/file.h>
+#include "access/commit_ts.h"
+#include "access/clog.h"
+#include "access/multixact.h"
#include "access/xlog.h"
#include "access/xlogutils.h"
#include "commands/tablespace.h"
@@ -90,13 +93,32 @@ typedef struct SyncOps
const FileTag *candidate);
} SyncOps;
+/*
+ * These indexes must correspond to the values of the SyncRequestHandler enum.
+ */
static const SyncOps syncsw[] = {
/* magnetic disk */
{
.sync_syncfiletag = mdsyncfiletag,
.sync_unlinkfiletag = mdunlinkfiletag,
.sync_filetagmatches = mdfiletagmatches
- }
+ },
+ /* pg_xact */
+ {
+ .sync_syncfiletag = clogsyncfiletag
+ },
+ /* pg_commit_ts */
+ {
+ .sync_syncfiletag = committssyncfiletag
+ },
+ /* pg_multixact/offsets */
+ {
+ .sync_syncfiletag = multixactoffsetssyncfiletag
+ },
+ /* pg_multixact/members */
+ {
+ .sync_syncfiletag = multixactmemberssyncfiletag
+ },
};
/*
diff --git a/src/include/access/clog.h b/src/include/access/clog.h
index 2db8acb189..d97b9042dc 100644
--- a/src/include/access/clog.h
+++ b/src/include/access/clog.h
@@ -12,6 +12,7 @@
#define CLOG_H
#include "access/xlogreader.h"
+#include "storage/sync.h"
#include "lib/stringinfo.h"
/*
@@ -50,6 +51,8 @@ extern void CheckPointCLOG(void);
extern void ExtendCLOG(TransactionId newestXact);
extern void TruncateCLOG(TransactionId oldestXact, Oid oldestxid_datoid);
+extern int clogsyncfiletag(const FileTag *ftag, char *path);
+
/* XLOG stuff */
#define CLOG_ZEROPAGE 0x00
#define CLOG_TRUNCATE 0x10
diff --git a/src/include/access/commit_ts.h b/src/include/access/commit_ts.h
index 2740c02a84..27900ce430 100644
--- a/src/include/access/commit_ts.h
+++ b/src/include/access/commit_ts.h
@@ -14,6 +14,7 @@
#include "access/xlog.h"
#include "datatype/timestamp.h"
#include "replication/origin.h"
+#include "storage/sync.h"
#include "utils/guc.h"
@@ -45,6 +46,8 @@ extern void SetCommitTsLimit(TransactionId oldestXact,
TransactionId newestXact);
extern void AdvanceOldestCommitTsXid(TransactionId oldestXact);
+extern int committssyncfiletag(const FileTag *ftag, char *path);
+
/* XLOG stuff */
#define COMMIT_TS_ZEROPAGE 0x00
#define COMMIT_TS_TRUNCATE 0x10
diff --git a/src/include/access/multixact.h b/src/include/access/multixact.h
index 6d729008c6..71d6e78063 100644
--- a/src/include/access/multixact.h
+++ b/src/include/access/multixact.h
@@ -13,6 +13,7 @@
#include "access/xlogreader.h"
#include "lib/stringinfo.h"
+#include "storage/sync.h"
/*
@@ -116,6 +117,9 @@ extern bool MultiXactIdPrecedes(MultiXactId multi1, MultiXactId multi2);
extern bool MultiXactIdPrecedesOrEquals(MultiXactId multi1,
MultiXactId multi2);
+extern int multixactoffsetssyncfiletag(const FileTag *ftag, char *path);
+extern int multixactmemberssyncfiletag(const FileTag *ftag, char *path);
+
extern void AtEOXact_MultiXact(void);
extern void AtPrepare_MultiXact(void);
extern void PostPrepare_MultiXact(TransactionId xid);
diff --git a/src/include/access/slru.h b/src/include/access/slru.h
index 61fbc80ef0..2720284157 100644
--- a/src/include/access/slru.h
+++ b/src/include/access/slru.h
@@ -15,6 +15,7 @@
#include "access/xlogdefs.h"
#include "storage/lwlock.h"
+#include "storage/sync.h"
/*
@@ -111,10 +112,10 @@ typedef struct SlruCtlData
SlruShared shared;
/*
- * This flag tells whether to fsync writes (true for pg_xact and multixact
- * stuff, false for pg_subtrans and pg_notify).
+ * Which sync handler function to use when handing sync requests over to
+ * the checkpointer. SYNC_HANDLER_NONE to disable fsync (eg pg_notify).
*/
- bool do_fsync;
+ SyncRequestHandler sync_handler;
/*
* Decide which of two page numbers is "older" for truncation purposes. We
@@ -135,7 +136,8 @@ typedef SlruCtlData *SlruCtl;
extern Size SimpleLruShmemSize(int nslots, int nlsns);
extern void SimpleLruInit(SlruCtl ctl, const char *name, int nslots, int nlsns,
- LWLock *ctllock, const char *subdir, int tranche_id);
+ LWLock *ctllock, const char *subdir, int tranche_id,
+ SyncRequestHandler sync_handler);
extern int SimpleLruZeroPage(SlruCtl ctl, int pageno);
extern int SimpleLruReadPage(SlruCtl ctl, int pageno, bool write_ok,
TransactionId xid);
@@ -151,6 +153,8 @@ typedef bool (*SlruScanCallback) (SlruCtl ctl, char *filename, int segpage,
extern bool SlruScanDirectory(SlruCtl ctl, SlruScanCallback callback, void *data);
extern void SlruDeleteSegment(SlruCtl ctl, int segno);
+extern int slrusyncfiletag(SlruCtl ctl, const FileTag *ftag, char *path);
+
/* SlruScanDirectory public callbacks */
extern bool SlruScanDirCbReportPresence(SlruCtl ctl, char *filename,
int segpage, void *data);
diff --git a/src/include/storage/sync.h b/src/include/storage/sync.h
index e16ab8e711..f32e412e75 100644
--- a/src/include/storage/sync.h
+++ b/src/include/storage/sync.h
@@ -34,7 +34,12 @@ typedef enum SyncRequestType
*/
typedef enum SyncRequestHandler
{
- SYNC_HANDLER_MD = 0 /* md smgr */
+ SYNC_HANDLER_MD = 0,
+ SYNC_HANDLER_CLOG,
+ SYNC_HANDLER_COMMIT_TS,
+ SYNC_HANDLER_MULTIXACT_OFFSET,
+ SYNC_HANDLER_MULTIXACT_MEMBER,
+ SYNC_HANDLER_NONE
} SyncRequestHandler;
/*
--
2.20.1
