public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/2] Refactor the fsync machinery to support future SMGR implementations.
17+ messages / 6 participants
[nested] [flat]
* [PATCH 2/2] Refactor the fsync machinery to support future SMGR implementations.
@ 2018-12-31 02:25 Thomas Munro <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Munro @ 2018-12-31 02:25 UTC (permalink / raw)
In anticipation of proposed block storage managers alongside md.c that
map bufmgr.c blocks to files optimised for different usage patterns:
1. Move the system for requesting fsyncs out of md.c into a new
translation unit smgrsync.c.
2. Have smgrsync.c perform the actual fsync() calls via the existing
polymorphic smgrimmedsync() interface, extended to allow an individual
segment number to be specified.
3. Teach the checkpointer how to forget individual segments that are
unlinked from the 'front' after having been dropped from shared
buffers.
4. Move the request tracking from a bitmapset into a sorted vector,
because the proposed block storage managers are not anchored at zero
and use potentially very large and sparse integers.
Author: Thomas Munro
Reviewed-by:
Discussion: https://postgr.es/m/CAEepm=2gTANm=e3ARnJT=n0h8hf88wqmaZxk0JYkxw+b21fNrw@mail.gmail.com
---
contrib/bloom/blinsert.c | 2 +-
src/backend/access/heap/heapam.c | 4 +-
src/backend/access/nbtree/nbtree.c | 2 +-
src/backend/access/nbtree/nbtsort.c | 2 +-
src/backend/access/spgist/spginsert.c | 2 +-
src/backend/access/transam/xlog.c | 2 +
src/backend/bootstrap/bootstrap.c | 1 +
src/backend/catalog/heap.c | 2 +-
src/backend/commands/dbcommands.c | 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/tablespace.c | 2 +-
src/backend/postmaster/bgwriter.c | 1 +
src/backend/postmaster/checkpointer.c | 22 +-
src/backend/storage/buffer/bufmgr.c | 2 +
src/backend/storage/ipc/ipci.c | 1 +
src/backend/storage/smgr/Makefile | 2 +-
src/backend/storage/smgr/md.c | 801 ++-----------------------------
src/backend/storage/smgr/smgr.c | 104 ++---
src/backend/storage/smgr/smgrsync.c | 855 ++++++++++++++++++++++++++++++++++
src/backend/tcop/utility.c | 2 +-
src/backend/utils/misc/guc.c | 1 +
src/include/postmaster/bgwriter.h | 24 +-
src/include/postmaster/checkpointer.h | 39 ++
src/include/storage/smgr.h | 29 +-
src/include/storage/smgrsync.h | 36 ++
25 files changed, 1055 insertions(+), 888 deletions(-)
create mode 100644 src/backend/storage/smgr/smgrsync.c
create mode 100644 src/include/postmaster/checkpointer.h
create mode 100644 src/include/storage/smgrsync.h
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index e43fbe0005..6fa07db4f8 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -188,7 +188,7 @@ blbuildempty(Relation index)
* write did not go through shared_buffers and therefore a concurrent
* checkpoint may have moved the redo pointer past our xlog record.
*/
- smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
+ smgrimmedsync(index->rd_smgr, INIT_FORKNUM, InvalidSegmentNumber);
}
/*
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index dc3499349b..4cf2661387 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8980,7 +8980,7 @@ heap_sync(Relation rel)
/* main heap */
FlushRelationBuffers(rel);
/* FlushRelationBuffers will have opened rd_smgr */
- smgrimmedsync(rel->rd_smgr, MAIN_FORKNUM);
+ smgrimmedsync(rel->rd_smgr, MAIN_FORKNUM, InvalidSegmentNumber);
/* FSM is not critical, don't bother syncing it */
@@ -8991,7 +8991,7 @@ heap_sync(Relation rel)
toastrel = table_open(rel->rd_rel->reltoastrelid, AccessShareLock);
FlushRelationBuffers(toastrel);
- smgrimmedsync(toastrel->rd_smgr, MAIN_FORKNUM);
+ smgrimmedsync(toastrel->rd_smgr, MAIN_FORKNUM, InvalidSegmentNumber);
table_close(toastrel, AccessShareLock);
}
}
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 98917de2ef..b29112c133 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -178,7 +178,7 @@ btbuildempty(Relation index)
* write did not go through shared_buffers and therefore a concurrent
* checkpoint may have moved the redo pointer past our xlog record.
*/
- smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
+ smgrimmedsync(index->rd_smgr, INIT_FORKNUM, InvalidSegmentNumber);
}
/*
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index dc398e1186..a64eaa06e4 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -1208,7 +1208,7 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2)
if (RelationNeedsWAL(wstate->index))
{
RelationOpenSmgr(wstate->index);
- smgrimmedsync(wstate->index->rd_smgr, MAIN_FORKNUM);
+ smgrimmedsync(wstate->index->rd_smgr, MAIN_FORKNUM, InvalidBlockNumber);
}
}
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index f428a15138..0eb5ced43d 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -205,7 +205,7 @@ spgbuildempty(Relation index)
* writes did not go through shared buffers and therefore a concurrent
* checkpoint may have moved the redo pointer past our xlog record.
*/
- smgrimmedsync(index->rd_smgr, INIT_FORKNUM);
+ smgrimmedsync(index->rd_smgr, INIT_FORKNUM, InvalidSegmentNumber);
}
/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ecd12fc53a..87d1172373 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -44,6 +44,7 @@
#include "pgstat.h"
#include "port/atomics.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "postmaster/walwriter.h"
#include "postmaster/startup.h"
#include "replication/basebackup.h"
@@ -64,6 +65,7 @@
#include "storage/procarray.h"
#include "storage/reinit.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "storage/spin.h"
#include "utils/builtins.h"
#include "utils/guc.h"
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index 4d7ed8ad1a..083314c18a 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -33,6 +33,7 @@
#include "pg_getopt.h"
#include "pgstat.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "postmaster/startup.h"
#include "postmaster/walwriter.h"
#include "replication/walreceiver.h"
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 06d18a1cfb..9c213efbc3 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1421,7 +1421,7 @@ heap_create_init_fork(Relation rel)
RelationOpenSmgr(rel);
smgrcreate(rel->rd_smgr, INIT_FORKNUM, false);
log_smgrcreate(&rel->rd_smgr->smgr_rnode.node, INIT_FORKNUM);
- smgrimmedsync(rel->rd_smgr, INIT_FORKNUM);
+ smgrimmedsync(rel->rd_smgr, INIT_FORKNUM, InvalidSegmentNumber);
}
/*
diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index d207cd899f..2f2993ab4d 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -47,7 +47,7 @@
#include "mb/pg_wchar.h"
#include "miscadmin.h"
#include "pgstat.h"
-#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "replication/slot.h"
#include "storage/copydir.h"
#include "storage/fd.h"
@@ -55,6 +55,7 @@
#include "storage/ipc.h"
#include "storage/procarray.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 715c6a221c..125b16c339 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11788,7 +11788,7 @@ copy_relation_data(SMgrRelation src, SMgrRelation dst,
* here, they might still not be on disk when the crash occurs.
*/
if (relpersistence == RELPERSISTENCE_PERMANENT || copying_initfork)
- smgrimmedsync(dst, forkNum);
+ smgrimmedsync(dst, forkNum, InvalidSegmentNumber);
}
/*
diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c
index 4afd178e97..ac239cfa8f 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -70,7 +70,7 @@
#include "commands/tablespace.h"
#include "common/file_perm.h"
#include "miscadmin.h"
-#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "storage/fd.h"
#include "storage/lmgr.h"
#include "storage/standby.h"
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index e6b6c549de..fd5803f195 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -44,6 +44,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "storage/bufmgr.h"
#include "storage/buf_internals.h"
#include "storage/condition_variable.h"
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index fe96c41359..a43dc03be3 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -47,6 +47,8 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
+#include "postmaster/postmaster.h"
#include "replication/syncrep.h"
#include "storage/bufmgr.h"
#include "storage/condition_variable.h"
@@ -56,6 +58,7 @@
#include "storage/proc.h"
#include "storage/shmem.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "storage/spin.h"
#include "utils/guc.h"
#include "utils/memutils.h"
@@ -108,10 +111,10 @@
*/
typedef struct
{
- RelFileNode rnode;
+ int type;
+ RelFileNode rnode;
ForkNumber forknum;
- BlockNumber segno; /* see md.c for special values */
- /* might add a real request-type field later; not needed yet */
+ SegmentNumber segno;
} CheckpointerRequest;
typedef struct
@@ -1077,9 +1080,7 @@ RequestCheckpoint(int flags)
* RelFileNodeBackend.
*
* segno specifies which segment (not block!) of the relation needs to be
- * fsync'd. (Since the valid range is much less than BlockNumber, we can
- * use high values for special flags; that's all internal to md.c, which
- * see for details.)
+ * fsync'd.
*
* To avoid holding the lock for longer than necessary, we normally write
* to the requests[] queue without checking for duplicates. The checkpointer
@@ -1092,13 +1093,14 @@ RequestCheckpoint(int flags)
* let the backend know by returning false.
*/
bool
-ForwardFsyncRequest(RelFileNode rnode, ForkNumber forknum, BlockNumber segno)
+ForwardFsyncRequest(int type, RelFileNode rnode, ForkNumber forknum,
+ SegmentNumber segno)
{
CheckpointerRequest *request;
bool too_full;
if (!IsUnderPostmaster)
- return false; /* probably shouldn't even get here */
+ elog(ERROR, "ForwardFsyncRequest must not be called in single user mode");
if (AmCheckpointerProcess())
elog(ERROR, "ForwardFsyncRequest must not be called in checkpointer");
@@ -1130,6 +1132,7 @@ ForwardFsyncRequest(RelFileNode rnode, ForkNumber forknum, BlockNumber segno)
/* OK, insert request */
request = &CheckpointerShmem->requests[CheckpointerShmem->num_requests++];
+ request->type = type;
request->rnode = rnode;
request->forknum = forknum;
request->segno = segno;
@@ -1314,7 +1317,8 @@ AbsorbFsyncRequests(void)
LWLockRelease(CheckpointerCommLock);
for (request = requests; n > 0; request++, n--)
- RememberFsyncRequest(request->rnode, request->forknum, request->segno);
+ RememberFsyncRequest(request->type, request->rnode, request->forknum,
+ request->segno);
END_CRIT_SECTION();
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 273e2f385f..97bdfcb7b3 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -42,11 +42,13 @@
#include "pg_trace.h"
#include "pgstat.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "storage/buf_internals.h"
#include "storage/bufmgr.h"
#include "storage/ipc.h"
#include "storage/proc.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "storage/standby.h"
#include "utils/rel.h"
#include "utils/resowner_private.h"
diff --git a/src/backend/storage/ipc/ipci.c b/src/backend/storage/ipc/ipci.c
index 5965d3620f..07b6c2f5f3 100644
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -27,6 +27,7 @@
#include "postmaster/autovacuum.h"
#include "postmaster/bgworker_internals.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "postmaster/postmaster.h"
#include "replication/logicallauncher.h"
#include "replication/slot.h"
diff --git a/src/backend/storage/smgr/Makefile b/src/backend/storage/smgr/Makefile
index 2b95cb0df1..c9c4be325e 100644
--- a/src/backend/storage/smgr/Makefile
+++ b/src/backend/storage/smgr/Makefile
@@ -12,6 +12,6 @@ subdir = src/backend/storage/smgr
top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
-OBJS = md.o smgr.o smgrtype.o
+OBJS = md.o smgr.o smgrsync.o smgrtype.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 2aba2dfe91..99470c0ebf 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -30,37 +30,24 @@
#include "access/xlog.h"
#include "pgstat.h"
#include "portability/instr_time.h"
-#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "storage/fd.h"
#include "storage/bufmgr.h"
#include "storage/relfilenode.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "utils/hsearch.h"
#include "utils/memutils.h"
#include "pg_trace.h"
-/* intervals for calling AbsorbFsyncRequests in mdsync and mdpostckpt */
-#define FSYNCS_PER_ABSORB 10
-#define UNLINKS_PER_ABSORB 10
-
-/*
- * Special values for the segno arg to RememberFsyncRequest.
- *
- * Note that CompactCheckpointerRequestQueue assumes that it's OK to remove an
- * fsync request from the queue if an identical, subsequent request is found.
- * See comments there before making changes here.
- */
-#define FORGET_RELATION_FSYNC (InvalidBlockNumber)
-#define FORGET_DATABASE_FSYNC (InvalidBlockNumber-1)
-#define UNLINK_RELATION_REQUEST (InvalidBlockNumber-2)
/*
* On Windows, we have to interpret EACCES as possibly meaning the same as
* ENOENT, because if a file is unlinked-but-not-yet-gone on that platform,
* that's what you get. Ugh. This code is designed so that we don't
* actually believe these cases are okay without further evidence (namely,
- * a pending fsync request getting canceled ... see mdsync).
+ * a pending fsync request getting canceled ... see smgrsync).
*/
#ifndef WIN32
#define FILE_POSSIBLY_DELETED(err) ((err) == ENOENT)
@@ -134,30 +121,9 @@ static MemoryContext MdCxt; /* context for all MdfdVec objects */
* (Regular backends do not track pending operations locally, but forward
* them to the checkpointer.)
*/
-typedef uint16 CycleCtr; /* can be any convenient integer size */
+typedef uint32 CycleCtr; /* can be any convenient integer size */
-typedef struct
-{
- RelFileNode rnode; /* hash table key (must be first!) */
- CycleCtr cycle_ctr; /* mdsync_cycle_ctr of oldest request */
- /* requests[f] has bit n set if we need to fsync segment n of fork f */
- Bitmapset *requests[MAX_FORKNUM + 1];
- /* canceled[f] is true if we canceled fsyncs for fork "recently" */
- bool canceled[MAX_FORKNUM + 1];
-} PendingOperationEntry;
-
-typedef struct
-{
- RelFileNode rnode; /* the dead relation to delete */
- CycleCtr cycle_ctr; /* mdckpt_cycle_ctr when request was made */
-} PendingUnlinkEntry;
-static HTAB *pendingOpsTable = NULL;
-static List *pendingUnlinks = NIL;
-static MemoryContext pendingOpsCxt; /* context for the above */
-
-static CycleCtr mdsync_cycle_ctr = 0;
-static CycleCtr mdckpt_cycle_ctr = 0;
/*** behavior for mdopen & _mdfd_getseg ***/
@@ -184,8 +150,7 @@ static void mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum,
bool isRedo);
static MdfdVec *mdopen(SMgrRelation reln, ForkNumber forknum, int behavior);
static void register_dirty_segment(SMgrRelation reln, ForkNumber forknum,
- MdfdVec *seg);
-static void register_unlink(RelFileNodeBackend rnode);
+ MdfdVec *seg);
static void _fdvec_resize(SMgrRelation reln,
ForkNumber forknum,
int nseg);
@@ -208,64 +173,6 @@ mdinit(void)
MdCxt = AllocSetContextCreate(TopMemoryContext,
"MdSmgr",
ALLOCSET_DEFAULT_SIZES);
-
- /*
- * Create pending-operations hashtable if we need it. Currently, we need
- * it if we are standalone (not under a postmaster) or if we are a startup
- * or checkpointer auxiliary process.
- */
- if (!IsUnderPostmaster || AmStartupProcess() || AmCheckpointerProcess())
- {
- HASHCTL hash_ctl;
-
- /*
- * XXX: The checkpointer needs to add entries to the pending ops table
- * when absorbing fsync requests. That is done within a critical
- * section, which isn't usually allowed, but we make an exception. It
- * means that there's a theoretical possibility that you run out of
- * memory while absorbing fsync requests, which leads to a PANIC.
- * Fortunately the hash table is small so that's unlikely to happen in
- * practice.
- */
- pendingOpsCxt = AllocSetContextCreate(MdCxt,
- "Pending ops context",
- ALLOCSET_DEFAULT_SIZES);
- MemoryContextAllowInCriticalSection(pendingOpsCxt, true);
-
- MemSet(&hash_ctl, 0, sizeof(hash_ctl));
- hash_ctl.keysize = sizeof(RelFileNode);
- hash_ctl.entrysize = sizeof(PendingOperationEntry);
- hash_ctl.hcxt = pendingOpsCxt;
- pendingOpsTable = hash_create("Pending Ops Table",
- 100L,
- &hash_ctl,
- HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
- pendingUnlinks = NIL;
- }
-}
-
-/*
- * In archive recovery, we rely on checkpointer to do fsyncs, but we will have
- * already created the pendingOpsTable during initialization of the startup
- * process. Calling this function drops the local pendingOpsTable so that
- * subsequent requests will be forwarded to checkpointer.
- */
-void
-SetForwardFsyncRequests(void)
-{
- /* Perform any pending fsyncs we may have queued up, then drop table */
- if (pendingOpsTable)
- {
- mdsync();
- hash_destroy(pendingOpsTable);
- }
- pendingOpsTable = NULL;
-
- /*
- * We should not have any pending unlink requests, since mdunlink doesn't
- * queue unlink requests when isRedo.
- */
- Assert(pendingUnlinks == NIL);
}
/*
@@ -382,7 +289,7 @@ mdunlink(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
{
/*
* We have to clean out any pending fsync requests for the doomed
- * relation, else the next mdsync() will fail. There can't be any such
+ * relation, else the next smgrsync() will fail. There can't be any such
* requests for a temp relation, though. We can send just one request
* even when deleting multiple forks, since the fsync queuing code accepts
* the "InvalidForkNumber = all forks" convention.
@@ -442,7 +349,7 @@ mdunlinkfork(RelFileNodeBackend rnode, ForkNumber forkNum, bool isRedo)
errmsg("could not truncate file \"%s\": %m", path)));
/* Register request to unlink first segment later */
- register_unlink(rnode);
+ UnlinkAfterCheckpoint(rnode);
}
/*
@@ -976,423 +883,55 @@ mdtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
*
* Note that only writes already issued are synced; this routine knows
* nothing of dirty buffers that may exist inside the buffer manager.
+ *
+ * See smgrimmedsync comment for contract.
*/
-void
-mdimmedsync(SMgrRelation reln, ForkNumber forknum)
+bool
+mdimmedsync(SMgrRelation reln, ForkNumber forknum, SegmentNumber segno)
{
- int segno;
+ MdfdVec *segments;
+ size_t num_segments;
+ size_t i;
- /*
- * NOTE: mdnblocks makes sure we have opened all active segments, so that
- * fsync loop will get them all!
- */
- mdnblocks(reln, forknum);
-
- segno = reln->md_num_open_segs[forknum];
+ if (segno != InvalidSegmentNumber)
+ {
+ /*
+ * Get the specified segment, or report failure if it doesn't seem to
+ * exist.
+ */
+ segments = _mdfd_openseg(reln, forknum, segno * RELSEG_SIZE,
+ EXTENSION_RETURN_NULL);
+ if (segments == NULL)
+ return false;
+ num_segments = 1;
+ }
+ else
+ {
+ /*
+ * NOTE: mdnblocks makes sure we have opened all active segments, so that
+ * fsync loop will get them all!
+ */
+ mdnblocks(reln, forknum);
+ num_segments = reln->md_num_open_segs[forknum];
+ segments = &reln->md_seg_fds[forknum][0];
+ }
- while (segno > 0)
+ for (i = 0; i < num_segments; ++i)
{
- MdfdVec *v = &reln->md_seg_fds[forknum][segno - 1];
+ MdfdVec *v = &segments[i];
if (FileSync(v->mdfd_vfd, WAIT_EVENT_DATA_FILE_IMMEDIATE_SYNC) < 0)
ereport(data_sync_elevel(ERROR),
(errcode_for_file_access(),
errmsg("could not fsync file \"%s\": %m",
FilePathName(v->mdfd_vfd))));
- segno--;
- }
-}
-
-/*
- * mdsync() -- Sync previous writes to stable storage.
- */
-void
-mdsync(void)
-{
- static bool mdsync_in_progress = false;
-
- HASH_SEQ_STATUS hstat;
- PendingOperationEntry *entry;
- int absorb_counter;
-
- /* Statistics on sync times */
- int processed = 0;
- instr_time sync_start,
- sync_end,
- sync_diff;
- uint64 elapsed;
- uint64 longest = 0;
- uint64 total_elapsed = 0;
-
- /*
- * This is only called during checkpoints, and checkpoints should only
- * occur in processes that have created a pendingOpsTable.
- */
- if (!pendingOpsTable)
- elog(ERROR, "cannot sync without a pendingOpsTable");
-
- /*
- * If we are in the checkpointer, the sync had better include all fsync
- * requests that were queued by backends up to this point. The tightest
- * race condition that could occur is that a buffer that must be written
- * and fsync'd for the checkpoint could have been dumped by a backend just
- * before it was visited by BufferSync(). We know the backend will have
- * queued an fsync request before clearing the buffer's dirtybit, so we
- * are safe as long as we do an Absorb after completing BufferSync().
- */
- AbsorbFsyncRequests();
-
- /*
- * To avoid excess fsync'ing (in the worst case, maybe a never-terminating
- * checkpoint), we want to ignore fsync requests that are entered into the
- * hashtable after this point --- they should be processed next time,
- * instead. We use mdsync_cycle_ctr to tell old entries apart from new
- * ones: new ones will have cycle_ctr equal to the incremented value of
- * mdsync_cycle_ctr.
- *
- * In normal circumstances, all entries present in the table at this point
- * will have cycle_ctr exactly equal to the current (about to be old)
- * value of mdsync_cycle_ctr. However, if we fail partway through the
- * fsync'ing loop, then older values of cycle_ctr might remain when we
- * come back here to try again. Repeated checkpoint failures would
- * eventually wrap the counter around to the point where an old entry
- * might appear new, causing us to skip it, possibly allowing a checkpoint
- * to succeed that should not have. To forestall wraparound, any time the
- * previous mdsync() failed to complete, run through the table and
- * forcibly set cycle_ctr = mdsync_cycle_ctr.
- *
- * Think not to merge this loop with the main loop, as the problem is
- * exactly that that loop may fail before having visited all the entries.
- * From a performance point of view it doesn't matter anyway, as this path
- * will never be taken in a system that's functioning normally.
- */
- if (mdsync_in_progress)
- {
- /* prior try failed, so update any stale cycle_ctr values */
- hash_seq_init(&hstat, pendingOpsTable);
- while ((entry = (PendingOperationEntry *) hash_seq_search(&hstat)) != NULL)
- {
- entry->cycle_ctr = mdsync_cycle_ctr;
- }
}
- /* Advance counter so that new hashtable entries are distinguishable */
- mdsync_cycle_ctr++;
-
- /* Set flag to detect failure if we don't reach the end of the loop */
- mdsync_in_progress = true;
-
- /* Now scan the hashtable for fsync requests to process */
- absorb_counter = FSYNCS_PER_ABSORB;
- hash_seq_init(&hstat, pendingOpsTable);
- while ((entry = (PendingOperationEntry *) hash_seq_search(&hstat)) != NULL)
- {
- ForkNumber forknum;
-
- /*
- * If the entry is new then don't process it this time; it might
- * contain multiple fsync-request bits, but they are all new. Note
- * "continue" bypasses the hash-remove call at the bottom of the loop.
- */
- if (entry->cycle_ctr == mdsync_cycle_ctr)
- continue;
-
- /* Else assert we haven't missed it */
- Assert((CycleCtr) (entry->cycle_ctr + 1) == mdsync_cycle_ctr);
-
- /*
- * Scan over the forks and segments represented by the entry.
- *
- * The bitmap manipulations are slightly tricky, because we can call
- * AbsorbFsyncRequests() inside the loop and that could result in
- * bms_add_member() modifying and even re-palloc'ing the bitmapsets.
- * So we detach it, but if we fail we'll merge it with any new
- * requests that have arrived in the meantime.
- */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- {
- Bitmapset *requests = entry->requests[forknum];
- int segno;
-
- entry->requests[forknum] = NULL;
- entry->canceled[forknum] = false;
-
- segno = -1;
- while ((segno = bms_next_member(requests, segno)) >= 0)
- {
- int failures;
-
- /*
- * If fsync is off then we don't have to bother opening the
- * file at all. (We delay checking until this point so that
- * changing fsync on the fly behaves sensibly.)
- */
- if (!enableFsync)
- continue;
-
- /*
- * If in checkpointer, we want to absorb pending requests
- * every so often to prevent overflow of the fsync request
- * queue. It is unspecified whether newly-added entries will
- * be visited by hash_seq_search, but we don't care since we
- * don't need to process them anyway.
- */
- if (--absorb_counter <= 0)
- {
- AbsorbFsyncRequests();
- absorb_counter = FSYNCS_PER_ABSORB;
- }
-
- /*
- * The fsync table could contain requests to fsync segments
- * that have been deleted (unlinked) by the time we get to
- * them. Rather than just hoping an ENOENT (or EACCES on
- * Windows) error can be ignored, what we do on error is
- * absorb pending requests and then retry. Since mdunlink()
- * queues a "cancel" message before actually unlinking, the
- * fsync request is guaranteed to be marked canceled after the
- * absorb if it really was this case. DROP DATABASE likewise
- * has to tell us to forget fsync requests before it starts
- * deletions.
- */
- for (failures = 0;; failures++) /* loop exits at "break" */
- {
- SMgrRelation reln;
- MdfdVec *seg;
- char *path;
- int save_errno;
-
- /*
- * Find or create an smgr hash entry for this relation.
- * This may seem a bit unclean -- md calling smgr? But
- * it's really the best solution. It ensures that the
- * open file reference isn't permanently leaked if we get
- * an error here. (You may say "but an unreferenced
- * SMgrRelation is still a leak!" Not really, because the
- * only case in which a checkpoint is done by a process
- * that isn't about to shut down is in the checkpointer,
- * and it will periodically do smgrcloseall(). This fact
- * justifies our not closing the reln in the success path
- * either, which is a good thing since in non-checkpointer
- * cases we couldn't safely do that.)
- */
- reln = smgropen(entry->rnode, InvalidBackendId);
-
- /* Attempt to open and fsync the target segment */
- seg = _mdfd_getseg(reln, forknum,
- (BlockNumber) segno * (BlockNumber) RELSEG_SIZE,
- false,
- EXTENSION_RETURN_NULL
- | EXTENSION_DONT_CHECK_SIZE);
-
- INSTR_TIME_SET_CURRENT(sync_start);
-
- if (seg != NULL &&
- FileSync(seg->mdfd_vfd, WAIT_EVENT_DATA_FILE_SYNC) >= 0)
- {
- /* Success; update statistics about sync timing */
- INSTR_TIME_SET_CURRENT(sync_end);
- sync_diff = sync_end;
- INSTR_TIME_SUBTRACT(sync_diff, sync_start);
- elapsed = INSTR_TIME_GET_MICROSEC(sync_diff);
- if (elapsed > longest)
- longest = elapsed;
- total_elapsed += elapsed;
- processed++;
- requests = bms_del_member(requests, segno);
- if (log_checkpoints)
- elog(DEBUG1, "checkpoint sync: number=%d file=%s time=%.3f msec",
- processed,
- FilePathName(seg->mdfd_vfd),
- (double) elapsed / 1000);
-
- break; /* out of retry loop */
- }
-
- /* Compute file name for use in message */
- save_errno = errno;
- path = _mdfd_segpath(reln, forknum, (BlockNumber) segno);
- errno = save_errno;
-
- /*
- * It is possible that the relation has been dropped or
- * truncated since the fsync request was entered.
- * Therefore, allow ENOENT, but only if we didn't fail
- * already on this file. This applies both for
- * _mdfd_getseg() and for FileSync, since fd.c might have
- * closed the file behind our back.
- *
- * XXX is there any point in allowing more than one retry?
- * Don't see one at the moment, but easy to change the
- * test here if so.
- */
- if (!FILE_POSSIBLY_DELETED(errno) ||
- failures > 0)
- {
- Bitmapset *new_requests;
-
- /*
- * We need to merge these unsatisfied requests with
- * any others that have arrived since we started.
- */
- new_requests = entry->requests[forknum];
- entry->requests[forknum] =
- bms_join(new_requests, requests);
-
- errno = save_errno;
- ereport(data_sync_elevel(ERROR),
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\": %m",
- path)));
- }
- else
- ereport(DEBUG1,
- (errcode_for_file_access(),
- errmsg("could not fsync file \"%s\" but retrying: %m",
- path)));
- pfree(path);
-
- /*
- * Absorb incoming requests and check to see if a cancel
- * arrived for this relation fork.
- */
- AbsorbFsyncRequests();
- absorb_counter = FSYNCS_PER_ABSORB; /* might as well... */
-
- if (entry->canceled[forknum])
- break;
- } /* end retry loop */
- }
- bms_free(requests);
- }
-
- /*
- * We've finished everything that was requested before we started to
- * scan the entry. If no new requests have been inserted meanwhile,
- * remove the entry. Otherwise, update its cycle counter, as all the
- * requests now in it must have arrived during this cycle.
- */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- {
- if (entry->requests[forknum] != NULL)
- break;
- }
- if (forknum <= MAX_FORKNUM)
- entry->cycle_ctr = mdsync_cycle_ctr;
- else
- {
- /* Okay to remove it */
- if (hash_search(pendingOpsTable, &entry->rnode,
- HASH_REMOVE, NULL) == NULL)
- elog(ERROR, "pendingOpsTable corrupted");
- }
- } /* end loop over hashtable entries */
-
- /* Return sync performance metrics for report at checkpoint end */
- CheckpointStats.ckpt_sync_rels = processed;
- CheckpointStats.ckpt_longest_sync = longest;
- CheckpointStats.ckpt_agg_sync_time = total_elapsed;
-
- /* Flag successful completion of mdsync */
- mdsync_in_progress = false;
-}
-
-/*
- * mdpreckpt() -- Do pre-checkpoint work
- *
- * To distinguish unlink requests that arrived before this checkpoint
- * started from those that arrived during the checkpoint, we use a cycle
- * counter similar to the one we use for fsync requests. That cycle
- * counter is incremented here.
- *
- * This must be called *before* the checkpoint REDO point is determined.
- * That ensures that we won't delete files too soon.
- *
- * Note that we can't do anything here that depends on the assumption
- * that the checkpoint will be completed.
- */
-void
-mdpreckpt(void)
-{
- /*
- * Any unlink requests arriving after this point will be assigned the next
- * cycle counter, and won't be unlinked until next checkpoint.
- */
- mdckpt_cycle_ctr++;
-}
-
-/*
- * mdpostckpt() -- Do post-checkpoint work
- *
- * Remove any lingering files that can now be safely removed.
- */
-void
-mdpostckpt(void)
-{
- int absorb_counter;
-
- absorb_counter = UNLINKS_PER_ABSORB;
- while (pendingUnlinks != NIL)
- {
- PendingUnlinkEntry *entry = (PendingUnlinkEntry *) linitial(pendingUnlinks);
- char *path;
-
- /*
- * New entries are appended to the end, so if the entry is new we've
- * reached the end of old entries.
- *
- * Note: if just the right number of consecutive checkpoints fail, we
- * could be fooled here by cycle_ctr wraparound. However, the only
- * consequence is that we'd delay unlinking for one more checkpoint,
- * which is perfectly tolerable.
- */
- if (entry->cycle_ctr == mdckpt_cycle_ctr)
- break;
-
- /* Unlink the file */
- path = relpathperm(entry->rnode, MAIN_FORKNUM);
- if (unlink(path) < 0)
- {
- /*
- * There's a race condition, when the database is dropped at the
- * same time that we process the pending unlink requests. If the
- * DROP DATABASE deletes the file before we do, we will get ENOENT
- * here. rmtree() also has to ignore ENOENT errors, to deal with
- * the possibility that we delete the file first.
- */
- if (errno != ENOENT)
- ereport(WARNING,
- (errcode_for_file_access(),
- errmsg("could not remove file \"%s\": %m", path)));
- }
- pfree(path);
-
- /* And remove the list entry */
- pendingUnlinks = list_delete_first(pendingUnlinks);
- pfree(entry);
-
- /*
- * As in mdsync, we don't want to stop absorbing fsync requests for a
- * long time when there are many deletions to be done. We can safely
- * call AbsorbFsyncRequests() at this point in the loop (note it might
- * try to delete list entries).
- */
- if (--absorb_counter <= 0)
- {
- AbsorbFsyncRequests();
- absorb_counter = UNLINKS_PER_ABSORB;
- }
- }
+ return true;
}
/*
* register_dirty_segment() -- Mark a relation segment as needing fsync
- *
- * If there is a local pending-ops table, just make an entry in it for
- * mdsync to process later. Otherwise, try to pass off the fsync request
- * to the checkpointer process. If that fails, just do the fsync
- * locally before returning (we hope this will not happen often enough
- * to be a performance problem).
*/
static void
register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
@@ -1400,16 +939,8 @@ register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
/* Temp relations should never be fsync'd */
Assert(!SmgrIsTemp(reln));
- if (pendingOpsTable)
+ if (!FsyncAtCheckpoint(reln->smgr_rnode.node, forknum, seg->mdfd_segno))
{
- /* push it into local pending-ops table */
- RememberFsyncRequest(reln->smgr_rnode.node, forknum, seg->mdfd_segno);
- }
- else
- {
- if (ForwardFsyncRequest(reln->smgr_rnode.node, forknum, seg->mdfd_segno))
- return; /* passed it off successfully */
-
ereport(DEBUG1,
(errmsg("could not forward fsync request because request queue is full")));
@@ -1421,258 +952,6 @@ register_dirty_segment(SMgrRelation reln, ForkNumber forknum, MdfdVec *seg)
}
}
-/*
- * register_unlink() -- Schedule a file to be deleted after next checkpoint
- *
- * We don't bother passing in the fork number, because this is only used
- * with main forks.
- *
- * As with register_dirty_segment, this could involve either a local or
- * a remote pending-ops table.
- */
-static void
-register_unlink(RelFileNodeBackend rnode)
-{
- /* Should never be used with temp relations */
- Assert(!RelFileNodeBackendIsTemp(rnode));
-
- if (pendingOpsTable)
- {
- /* push it into local pending-ops table */
- RememberFsyncRequest(rnode.node, MAIN_FORKNUM,
- UNLINK_RELATION_REQUEST);
- }
- else
- {
- /*
- * Notify the checkpointer about it. If we fail to queue the request
- * message, we have to sleep and try again, because we can't simply
- * delete the file now. Ugly, but hopefully won't happen often.
- *
- * XXX should we just leave the file orphaned instead?
- */
- Assert(IsUnderPostmaster);
- while (!ForwardFsyncRequest(rnode.node, MAIN_FORKNUM,
- UNLINK_RELATION_REQUEST))
- pg_usleep(10000L); /* 10 msec seems a good number */
- }
-}
-
-/*
- * RememberFsyncRequest() -- callback from checkpointer side of fsync request
- *
- * We stuff fsync requests into the local hash table for execution
- * during the checkpointer's next checkpoint. UNLINK requests go into a
- * separate linked list, however, because they get processed separately.
- *
- * The range of possible segment numbers is way less than the range of
- * BlockNumber, so we can reserve high values of segno for special purposes.
- * We define three:
- * - FORGET_RELATION_FSYNC means to cancel pending fsyncs for a relation,
- * either for one fork, or all forks if forknum is InvalidForkNumber
- * - FORGET_DATABASE_FSYNC means to cancel pending fsyncs for a whole database
- * - UNLINK_RELATION_REQUEST is a request to delete the file after the next
- * checkpoint.
- * Note also that we're assuming real segment numbers don't exceed INT_MAX.
- *
- * (Handling FORGET_DATABASE_FSYNC requests is a tad slow because the hash
- * table has to be searched linearly, but dropping a database is a pretty
- * heavyweight operation anyhow, so we'll live with it.)
- */
-void
-RememberFsyncRequest(RelFileNode rnode, ForkNumber forknum, BlockNumber segno)
-{
- Assert(pendingOpsTable);
-
- if (segno == FORGET_RELATION_FSYNC)
- {
- /* Remove any pending requests for the relation (one or all forks) */
- PendingOperationEntry *entry;
-
- entry = (PendingOperationEntry *) hash_search(pendingOpsTable,
- &rnode,
- HASH_FIND,
- NULL);
- if (entry)
- {
- /*
- * We can't just delete the entry since mdsync could have an
- * active hashtable scan. Instead we delete the bitmapsets; this
- * is safe because of the way mdsync is coded. We also set the
- * "canceled" flags so that mdsync can tell that a cancel arrived
- * for the fork(s).
- */
- if (forknum == InvalidForkNumber)
- {
- /* remove requests for all forks */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- {
- bms_free(entry->requests[forknum]);
- entry->requests[forknum] = NULL;
- entry->canceled[forknum] = true;
- }
- }
- else
- {
- /* remove requests for single fork */
- bms_free(entry->requests[forknum]);
- entry->requests[forknum] = NULL;
- entry->canceled[forknum] = true;
- }
- }
- }
- else if (segno == FORGET_DATABASE_FSYNC)
- {
- /* Remove any pending requests for the entire database */
- HASH_SEQ_STATUS hstat;
- PendingOperationEntry *entry;
- ListCell *cell,
- *prev,
- *next;
-
- /* Remove fsync requests */
- hash_seq_init(&hstat, pendingOpsTable);
- while ((entry = (PendingOperationEntry *) hash_seq_search(&hstat)) != NULL)
- {
- if (entry->rnode.dbNode == rnode.dbNode)
- {
- /* remove requests for all forks */
- for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
- {
- bms_free(entry->requests[forknum]);
- entry->requests[forknum] = NULL;
- entry->canceled[forknum] = true;
- }
- }
- }
-
- /* Remove unlink requests */
- prev = NULL;
- for (cell = list_head(pendingUnlinks); cell; cell = next)
- {
- PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
-
- next = lnext(cell);
- if (entry->rnode.dbNode == rnode.dbNode)
- {
- pendingUnlinks = list_delete_cell(pendingUnlinks, cell, prev);
- pfree(entry);
- }
- else
- prev = cell;
- }
- }
- else if (segno == UNLINK_RELATION_REQUEST)
- {
- /* Unlink request: put it in the linked list */
- MemoryContext oldcxt = MemoryContextSwitchTo(pendingOpsCxt);
- PendingUnlinkEntry *entry;
-
- /* PendingUnlinkEntry doesn't store forknum, since it's always MAIN */
- Assert(forknum == MAIN_FORKNUM);
-
- entry = palloc(sizeof(PendingUnlinkEntry));
- entry->rnode = rnode;
- entry->cycle_ctr = mdckpt_cycle_ctr;
-
- pendingUnlinks = lappend(pendingUnlinks, entry);
-
- MemoryContextSwitchTo(oldcxt);
- }
- else
- {
- /* Normal case: enter a request to fsync this segment */
- MemoryContext oldcxt = MemoryContextSwitchTo(pendingOpsCxt);
- PendingOperationEntry *entry;
- bool found;
-
- entry = (PendingOperationEntry *) hash_search(pendingOpsTable,
- &rnode,
- HASH_ENTER,
- &found);
- /* if new entry, initialize it */
- if (!found)
- {
- entry->cycle_ctr = mdsync_cycle_ctr;
- MemSet(entry->requests, 0, sizeof(entry->requests));
- MemSet(entry->canceled, 0, sizeof(entry->canceled));
- }
-
- /*
- * NB: it's intentional that we don't change cycle_ctr if the entry
- * already exists. The cycle_ctr must represent the oldest fsync
- * request that could be in the entry.
- */
-
- entry->requests[forknum] = bms_add_member(entry->requests[forknum],
- (int) segno);
-
- MemoryContextSwitchTo(oldcxt);
- }
-}
-
-/*
- * ForgetRelationFsyncRequests -- forget any fsyncs for a relation fork
- *
- * forknum == InvalidForkNumber means all forks, although this code doesn't
- * actually know that, since it's just forwarding the request elsewhere.
- */
-void
-ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum)
-{
- if (pendingOpsTable)
- {
- /* standalone backend or startup process: fsync state is local */
- RememberFsyncRequest(rnode, forknum, FORGET_RELATION_FSYNC);
- }
- else if (IsUnderPostmaster)
- {
- /*
- * Notify the checkpointer about it. If we fail to queue the cancel
- * message, we have to sleep and try again ... ugly, but hopefully
- * won't happen often.
- *
- * XXX should we CHECK_FOR_INTERRUPTS in this loop? Escaping with an
- * error would leave the no-longer-used file still present on disk,
- * which would be bad, so I'm inclined to assume that the checkpointer
- * will always empty the queue soon.
- */
- while (!ForwardFsyncRequest(rnode, forknum, FORGET_RELATION_FSYNC))
- pg_usleep(10000L); /* 10 msec seems a good number */
-
- /*
- * Note we don't wait for the checkpointer to actually absorb the
- * cancel message; see mdsync() for the implications.
- */
- }
-}
-
-/*
- * ForgetDatabaseFsyncRequests -- forget any fsyncs and unlinks for a DB
- */
-void
-ForgetDatabaseFsyncRequests(Oid dbid)
-{
- RelFileNode rnode;
-
- rnode.dbNode = dbid;
- rnode.spcNode = 0;
- rnode.relNode = 0;
-
- if (pendingOpsTable)
- {
- /* standalone backend or startup process: fsync state is local */
- RememberFsyncRequest(rnode, InvalidForkNumber, FORGET_DATABASE_FSYNC);
- }
- else if (IsUnderPostmaster)
- {
- /* see notes in ForgetRelationFsyncRequests */
- while (!ForwardFsyncRequest(rnode, InvalidForkNumber,
- FORGET_DATABASE_FSYNC))
- pg_usleep(10000L); /* 10 msec seems a good number */
- }
-}
-
/*
* DropRelationFiles -- drop files of all given relations
*/
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index 0c0bba4ab3..c6d3da1c1a 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -21,6 +21,7 @@
#include "storage/bufmgr.h"
#include "storage/ipc.h"
#include "storage/smgr.h"
+#include "storage/smgrsync.h"
#include "utils/hsearch.h"
#include "utils/inval.h"
@@ -58,10 +59,8 @@ typedef struct f_smgr
BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
void (*smgr_truncate) (SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
- void (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum);
- void (*smgr_pre_ckpt) (void); /* may be NULL */
- void (*smgr_sync) (void); /* may be NULL */
- void (*smgr_post_ckpt) (void); /* may be NULL */
+ bool (*smgr_immedsync) (SMgrRelation reln, ForkNumber forknum,
+ SegmentNumber segno);
} f_smgr;
@@ -81,10 +80,7 @@ static const f_smgr smgrsw[] = {
.smgr_writeback = mdwriteback,
.smgr_nblocks = mdnblocks,
.smgr_truncate = mdtruncate,
- .smgr_immedsync = mdimmedsync,
- .smgr_pre_ckpt = mdpreckpt,
- .smgr_sync = mdsync,
- .smgr_post_ckpt = mdpostckpt
+ .smgr_immedsync = mdimmedsync
}
};
@@ -104,6 +100,14 @@ static void smgrshutdown(int code, Datum arg);
static void add_to_unowned_list(SMgrRelation reln);
static void remove_from_unowned_list(SMgrRelation reln);
+/*
+ * For now there is only one implementation.
+ */
+static inline int
+which_for_relfilenode(RelFileNode rnode)
+{
+ return 0; /* we only have md.c at present */
+}
/*
* smgrinit(), smgrshutdown() -- Initialize or shut down storage
@@ -118,6 +122,8 @@ smgrinit(void)
{
int i;
+ smgrsync_init();
+
for (i = 0; i < NSmgr; i++)
{
if (smgrsw[i].smgr_init)
@@ -185,7 +191,7 @@ smgropen(RelFileNode rnode, BackendId backend)
reln->smgr_targblock = InvalidBlockNumber;
reln->smgr_fsm_nblocks = InvalidBlockNumber;
reln->smgr_vm_nblocks = InvalidBlockNumber;
- reln->smgr_which = 0; /* we only have md.c at present */
+ reln->smgr_which = which_for_relfilenode(rnode);
/* mark it not open */
for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
@@ -726,17 +732,20 @@ smgrtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
* smgrimmedsync() -- Force the specified relation to stable storage.
*
* Synchronously force all previous writes to the specified relation
- * down to disk.
- *
- * This is useful for building completely new relations (eg, new
- * indexes). Instead of incrementally WAL-logging the index build
- * steps, we can just write completed index pages to disk with smgrwrite
- * or smgrextend, and then fsync the completed index file before
- * committing the transaction. (This is sufficient for purposes of
- * crash recovery, since it effectively duplicates forcing a checkpoint
- * for the completed index. But it is *not* sufficient if one wishes
- * to use the WAL log for PITR or replication purposes: in that case
- * we have to make WAL entries as well.)
+ * down to disk. If segnum is >= 0, only applies to data in
+ * one segment file.
+ *
+ * Used for checkpointing dirty files.
+ *
+ * This can also be used for building completely new relations (eg, new
+ * indexes). Instead of incrementally WAL-logging the index build steps,
+ * we can just write completed index pages to disk with smgrwrite or
+ * smgrextend, and then fsync the completed index file before committing
+ * the transaction. (This is sufficient for purposes of crash recovery,
+ * since it effectively duplicates forcing a checkpoint for the completed
+ * index. But it is *not* sufficient if one wishes to use the WAL log
+ * for PITR or replication purposes: in that case we have to make WAL
+ * entries as well.)
*
* The preceding writes should specify skipFsync = true to avoid
* duplicative fsyncs.
@@ -744,57 +753,14 @@ smgrtruncate(SMgrRelation reln, ForkNumber forknum, BlockNumber nblocks)
* Note that you need to do FlushRelationBuffers() first if there is
* any possibility that there are dirty buffers for the relation;
* otherwise the sync is not very meaningful.
+ *
+ * Fail to fsync raises an error, but non-existence of a requested
+ * segment is reported with a false return value.
*/
-void
-smgrimmedsync(SMgrRelation reln, ForkNumber forknum)
-{
- smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum);
-}
-
-
-/*
- * smgrpreckpt() -- Prepare for checkpoint.
- */
-void
-smgrpreckpt(void)
-{
- int i;
-
- for (i = 0; i < NSmgr; i++)
- {
- if (smgrsw[i].smgr_pre_ckpt)
- smgrsw[i].smgr_pre_ckpt();
- }
-}
-
-/*
- * smgrsync() -- Sync files to disk during checkpoint.
- */
-void
-smgrsync(void)
-{
- int i;
-
- for (i = 0; i < NSmgr; i++)
- {
- if (smgrsw[i].smgr_sync)
- smgrsw[i].smgr_sync();
- }
-}
-
-/*
- * smgrpostckpt() -- Post-checkpoint cleanup.
- */
-void
-smgrpostckpt(void)
+bool
+smgrimmedsync(SMgrRelation reln, ForkNumber forknum, SegmentNumber segno)
{
- int i;
-
- for (i = 0; i < NSmgr; i++)
- {
- if (smgrsw[i].smgr_post_ckpt)
- smgrsw[i].smgr_post_ckpt();
- }
+ return smgrsw[reln->smgr_which].smgr_immedsync(reln, forknum, segno);
}
/*
diff --git a/src/backend/storage/smgr/smgrsync.c b/src/backend/storage/smgr/smgrsync.c
new file mode 100644
index 0000000000..d343e59931
--- /dev/null
+++ b/src/backend/storage/smgr/smgrsync.c
@@ -0,0 +1,855 @@
+/*-------------------------------------------------------------------------
+ *
+ * smgrsync.c
+ * management of file synchronization.
+ *
+ * This modules tracks which files need to be fsynced or unlinked at the
+ * next checkpoint, and performs those actions. Normally the work is done
+ * when called by the checkpointer, but it is also done in standalone mode
+ * and startup.
+ *
+ * Originally this logic was inside md.c, but it is now made more general,
+ * for reuse by other SMGR implementations that work with files.
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/storage/smgr/smgrsync.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include <unistd.h>
+
+#include "access/xlog.h"
+#include "miscadmin.h"
+#include "nodes/pg_list.h"
+#include "pgstat.h"
+#include "portability/instr_time.h"
+#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
+#include "storage/relfilenode.h"
+#include "storage/smgrsync.h"
+#include "utils/hsearch.h"
+#include "utils/memutils.h"
+
+static MemoryContext pendingOpsCxt; /* context for the pending ops state */
+
+#define SV_PREFIX segnum_vector
+#define SV_DECLARE
+#define SV_DEFINE
+#define SV_ELEMENT_TYPE BlockNumber
+#define SV_SCOPE static inline
+#define SV_GLOBAL_MEMORY_CONTEXT pendingOpsCxt
+#include "lib/simplevector.h"
+
+#define SA_PREFIX segnum_array
+#define SA_COMPARE(a,b) (*a < *b ? -1 : *a == *b ? 0 : 1)
+#define SA_DECLARE
+#define SA_DEFINE
+#define SA_ELEMENT_TYPE SV_ELEMENT_TYPE
+#define SA_SCOPE static inline
+#include "lib/sort_utils.h"
+
+/*
+ * In some contexts (currently, standalone backends and the checkpointer)
+ * we keep track of pending fsync operations: we need to remember all relation
+ * segments that have been written since the last checkpoint, so that we can
+ * fsync them down to disk before completing the next checkpoint. A hash
+ * table remembers the pending operations. We use a hash table mostly as
+ * a convenient way of merging duplicate requests.
+ *
+ * We use a similar mechanism to remember no-longer-needed files that can
+ * be deleted after the next checkpoint, but we use a linked list instead of
+ * a hash table, because we don't expect there to be any duplicate requests.
+ *
+ * These mechanisms are only used for non-temp relations; we never fsync
+ * temp rels, nor do we need to postpone their deletion (see comments in
+ * mdunlink).
+ *
+ * (Regular backends do not track pending operations locally, but forward
+ * them to the checkpointer.)
+ */
+
+typedef uint32 CycleCtr; /* can be any convenient integer size */
+
+/*
+ * Values for the "type" member of CheckpointerRequest.
+ *
+ * Note that CompactCheckpointerRequestQueue assumes that it's OK to remove an
+ * fsync request from the queue if an identical, subsequent request is found.
+ * See comments there before making changes here.
+ */
+#define FSYNC_SEGMENT_REQUEST 1
+#define FORGET_SEGMENT_FSYNC 2
+#define FORGET_RELATION_FSYNC 3
+#define FORGET_DATABASE_FSYNC 4
+#define UNLINK_RELATION_REQUEST 5
+#define UNLINK_SEGMENT_REQUEST 6
+
+/* intervals for calling AbsorbFsyncRequests in smgrsync and smgrpostckpt */
+#define FSYNCS_PER_ABSORB 10
+#define UNLINKS_PER_ABSORB 10
+
+/*
+ * An entry in the hash table of files that need to be flushed for the next
+ * checkpoint.
+ */
+typedef struct PendingFsyncEntry
+{
+ RelFileNode rnode;
+ segnum_vector requests[MAX_FORKNUM + 1];
+ segnum_vector requests_in_progress[MAX_FORKNUM + 1];
+ CycleCtr cycle_ctr;
+} PendingFsyncEntry;
+
+typedef struct PendingUnlinkEntry
+{
+ RelFileNode rnode; /* the dead relation to delete */
+ CycleCtr cycle_ctr; /* ckpt_cycle_ctr when request was made */
+} PendingUnlinkEntry;
+
+static bool sync_in_progress = false;
+static CycleCtr sync_cycle_ctr = 0;
+static CycleCtr ckpt_cycle_ctr = 0;
+
+static HTAB *pendingFsyncTable = NULL;
+static List *pendingUnlinks = NIL;
+
+/*
+ * Initialize the pending operations state, if necessary.
+ */
+void
+smgrsync_init(void)
+{
+ /*
+ * Create pending-operations hashtable if we need it. Currently, we need
+ * it if we are standalone (not under a postmaster) or if we are a startup
+ * or checkpointer auxiliary process.
+ */
+ if (!IsUnderPostmaster || AmStartupProcess() || AmCheckpointerProcess())
+ {
+ HASHCTL hash_ctl;
+
+ /*
+ * XXX: The checkpointer needs to add entries to the pending ops table
+ * when absorbing fsync requests. That is done within a critical
+ * section, which isn't usually allowed, but we make an exception. It
+ * means that there's a theoretical possibility that you run out of
+ * memory while absorbing fsync requests, which leads to a PANIC.
+ * Fortunately the hash table is small so that's unlikely to happen in
+ * practice.
+ */
+ pendingOpsCxt = AllocSetContextCreate(TopMemoryContext,
+ "Pending ops context",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContextAllowInCriticalSection(pendingOpsCxt, true);
+
+ MemSet(&hash_ctl, 0, sizeof(hash_ctl));
+ hash_ctl.keysize = sizeof(RelFileNode);
+ hash_ctl.entrysize = sizeof(PendingFsyncEntry);
+ hash_ctl.hcxt = pendingOpsCxt;
+ pendingFsyncTable = hash_create("Pending Ops Table",
+ 100L,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS | HASH_CONTEXT);
+ pendingUnlinks = NIL;
+ }
+}
+
+/*
+ * Do pre-checkpoint work.
+ *
+ * To distinguish unlink requests that arrived before this checkpoint
+ * started from those that arrived during the checkpoint, we use a cycle
+ * counter similar to the one we use for fsync requests. That cycle
+ * counter is incremented here.
+ *
+ * This must be called *before* the checkpoint REDO point is determined.
+ * That ensures that we won't delete files too soon.
+ *
+ * Note that we can't do anything here that depends on the assumption
+ * that the checkpoint will be completed.
+ */
+void
+smgrpreckpt(void)
+{
+ /*
+ * Any unlink requests arriving after this point will be assigned the next
+ * cycle counter, and won't be unlinked until next checkpoint.
+ */
+ ckpt_cycle_ctr++;
+}
+
+/*
+ * Sync previous writes to stable storage.
+ */
+void
+smgrsync(void)
+{
+ HASH_SEQ_STATUS hstat;
+ PendingFsyncEntry *entry;
+ int absorb_counter;
+
+ /* Statistics on sync times */
+ instr_time sync_start,
+ sync_end,
+ sync_diff;
+ uint64 elapsed;
+ int processed = CheckpointStats.ckpt_sync_rels;
+ uint64 longest = CheckpointStats.ckpt_longest_sync;
+ uint64 total_elapsed = CheckpointStats.ckpt_agg_sync_time;
+
+ /*
+ * This is only called during checkpoints, and checkpoints should only
+ * occur in processes that have created a pendingFsyncTable.
+ */
+ if (!pendingFsyncTable)
+ elog(ERROR, "cannot sync without a pendingFsyncTable");
+
+ /*
+ * If we are in the checkpointer, the sync had better include all fsync
+ * requests that were queued by backends up to this point. The tightest
+ * race condition that could occur is that a buffer that must be written
+ * and fsync'd for the checkpoint could have been dumped by a backend just
+ * before it was visited by BufferSync(). We know the backend will have
+ * queued an fsync request before clearing the buffer's dirtybit, so we
+ * are safe as long as we do an Absorb after completing BufferSync().
+ */
+ AbsorbFsyncRequests();
+
+ /*
+ * To avoid excess fsync'ing (in the worst case, maybe a never-terminating
+ * checkpoint), we want to ignore fsync requests that are entered into the
+ * hashtable after this point --- they should be processed next time,
+ * instead. We use sync_cycle_ctr to tell old entries apart from new
+ * ones: new ones will have cycle_ctr equal to the incremented value of
+ * sync_cycle_ctr.
+ *
+ * In normal circumstances, all entries present in the table at this point
+ * will have cycle_ctr exactly equal to the current (about to be old)
+ * value of sync_cycle_ctr. However, if we fail partway through the
+ * fsync'ing loop, then older values of cycle_ctr might remain when we
+ * come back here to try again. Repeated checkpoint failures would
+ * eventually wrap the counter around to the point where an old entry
+ * might appear new, causing us to skip it, possibly allowing a checkpoint
+ * to succeed that should not have. To forestall wraparound, any time the
+ * previous smgrsync() failed to complete, run through the table and
+ * forcibly set cycle_ctr = sync_cycle_ctr.
+ *
+ * Think not to merge this loop with the main loop, as the problem is
+ * exactly that that loop may fail before having visited all the entries.
+ * From a performance point of view it doesn't matter anyway, as this path
+ * will never be taken in a system that's functioning normally.
+ */
+ if (sync_in_progress)
+ {
+ /* prior try failed, so update any stale cycle_ctr values */
+ hash_seq_init(&hstat, pendingFsyncTable);
+ while ((entry = (PendingFsyncEntry *) hash_seq_search(&hstat)) != NULL)
+ {
+ ForkNumber forknum;
+
+ entry->cycle_ctr = sync_cycle_ctr;
+
+ /*
+ * If any requests remain unprocessed, they need to be merged with
+ * the segment numbers that have arrived since.
+ */
+ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
+ {
+ segnum_vector *requests = &entry->requests[forknum];
+ segnum_vector *requests_in_progress =
+ &entry->requests_in_progress[forknum];
+
+ if (!segnum_vector_empty(requests_in_progress))
+ {
+ /* Append the unfinished requests that were not yet handled. */
+ segnum_vector_append_n(requests,
+ segnum_vector_data(requests_in_progress),
+ segnum_vector_size(requests_in_progress));
+ segnum_vector_reset(requests_in_progress);
+
+ /* Sort and make unique. */
+ segnum_array_sort(segnum_vector_begin(requests),
+ segnum_vector_end(requests));
+ segnum_vector_resize(requests,
+ segnum_array_unique(segnum_vector_begin(requests),
+ segnum_vector_end(requests)) -
+ segnum_vector_begin(requests));
+ }
+ }
+ }
+ }
+
+ /* Advance counter so that new hashtable entries are distinguishable */
+ sync_cycle_ctr++;
+
+ /* Set flag to detect failure if we don't reach the end of the loop */
+ sync_in_progress = true;
+
+ /* Now scan the hashtable for fsync requests to process */
+ absorb_counter = FSYNCS_PER_ABSORB;
+ hash_seq_init(&hstat, pendingFsyncTable);
+ while ((entry = (PendingFsyncEntry *) hash_seq_search(&hstat)))
+ {
+ ForkNumber forknum;
+ SMgrRelation reln;
+
+ /*
+ * If the entry is new then don't process it this time; it might
+ * contain multiple fsync requests, but they are all new. Note
+ * "continue" bypasses the hash-remove call at the bottom of the loop.
+ */
+ if (entry->cycle_ctr == sync_cycle_ctr)
+ continue;
+
+ /* Else assert we haven't missed it */
+ Assert((CycleCtr) (entry->cycle_ctr + 1) == sync_cycle_ctr);
+
+ /*
+ * Scan over the forks and segments represented by the entry.
+ *
+ * The vector manipulations are slightly tricky, because we can call
+ * AbsorbFsyncRequests() inside the loop and that could result in new
+ * segment numbers being added. So we swap the contents of "requests"
+ * with "requests_in_progress", and if we fail we'll merge it with any
+ * new requests that have arrived in the meantime.
+ */
+ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
+ {
+ segnum_vector *requests_in_progress =
+ &entry->requests_in_progress[forknum];
+
+ /*
+ * Transfer the current set of segment numbers into the "in
+ * progress" vector (which must be empty initially).
+ */
+ Assert(segnum_vector_empty(requests_in_progress));
+ segnum_vector_swap(&entry->requests[forknum], requests_in_progress);
+
+ /*
+ * If fsync is off then we don't have to bother opening the
+ * files at all. (We delay checking until this point so that
+ * changing fsync on the fly behaves sensibly.)
+ */
+ if (!enableFsync)
+ segnum_vector_clear(requests_in_progress);
+
+ /* Loop until all requests have been handled. */
+ while (!segnum_vector_empty(requests_in_progress))
+ {
+ SegmentNumber segno = *segnum_vector_back(requests_in_progress);
+
+ INSTR_TIME_SET_CURRENT(sync_start);
+
+ reln = smgropen(entry->rnode, InvalidBackendId);
+ if (!smgrimmedsync(reln, forknum, segno))
+ {
+ /*
+ * The underlying file couldn't be found. Check if a
+ * later message in the queue reports that it has been
+ * unlinked; if so it will be removed from the vector,
+ * indicating that we can safely skip it.
+ */
+ AbsorbFsyncRequests();
+ if (!segnum_array_binary_search(segnum_vector_begin(requests_in_progress),
+ segnum_vector_end(requests_in_progress),
+ &segno))
+ continue;
+
+ /* Otherwise it's an unexpectedly missing file. */
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open backing file to fsync: %u/%u/%u",
+ entry->rnode.dbNode,
+ entry->rnode.relNode,
+ segno)));
+ }
+
+ /* Success; update statistics about sync timing */
+ INSTR_TIME_SET_CURRENT(sync_end);
+ sync_diff = sync_end;
+ INSTR_TIME_SUBTRACT(sync_diff, sync_start);
+ elapsed = INSTR_TIME_GET_MICROSEC(sync_diff);
+ if (elapsed > longest)
+ longest = elapsed;
+ total_elapsed += elapsed;
+ processed++;
+
+ /* Remove this segment number. */
+ Assert(segno == *segnum_vector_back(requests_in_progress));
+ segnum_vector_pop_back(requests_in_progress);
+
+ if (log_checkpoints)
+ ereport(DEBUG1,
+ (errmsg("checkpoint sync: number=%d db=%u rel=%u seg=%u time=%.3f msec",
+ processed,
+ entry->rnode.dbNode,
+ entry->rnode.relNode,
+ segno,
+ (double) elapsed / 1000),
+ errhidestmt(true),
+ errhidecontext(true)));
+
+ /*
+ * If in checkpointer, we want to absorb pending requests
+ * every so often to prevent overflow of the fsync request
+ * queue. It is unspecified whether newly-added entries will
+ * be visited by hash_seq_search, but we don't care since we
+ * don't need to process them anyway.
+ */
+ if (--absorb_counter <= 0)
+ {
+ AbsorbFsyncRequests();
+ absorb_counter = FSYNCS_PER_ABSORB;
+ }
+ }
+ }
+
+ /*
+ * We've finished everything that was requested before we started to
+ * scan the entry. If no new requests have been inserted meanwhile,
+ * remove the entry. Otherwise, update its cycle counter, as all the
+ * requests now in it must have arrived during this cycle.
+ */
+ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
+ {
+ Assert(segnum_vector_empty(&entry->requests_in_progress[forknum]));
+ if (!segnum_vector_empty(&entry->requests[forknum]))
+ break;
+ segnum_vector_reset(&entry->requests[forknum]);
+ }
+ if (forknum <= MAX_FORKNUM)
+ entry->cycle_ctr = sync_cycle_ctr;
+ else
+ {
+ /* Okay to remove it */
+ if (hash_search(pendingFsyncTable, &entry->rnode,
+ HASH_REMOVE, NULL) == NULL)
+ elog(ERROR, "pendingOpsTable corrupted");
+ }
+ } /* end loop over hashtable entries */
+
+ /* Maintain sync performance metrics for report at checkpoint end */
+ CheckpointStats.ckpt_sync_rels = processed;
+ CheckpointStats.ckpt_longest_sync = longest;
+ CheckpointStats.ckpt_agg_sync_time = total_elapsed;
+
+ /* Flag successful completion of smgrsync */
+ sync_in_progress = false;
+}
+
+/*
+ * Do post-checkpoint work.
+ *
+ * Remove any lingering files that can now be safely removed.
+ */
+void
+smgrpostckpt(void)
+{
+ int absorb_counter;
+
+ absorb_counter = UNLINKS_PER_ABSORB;
+ while (pendingUnlinks != NIL)
+ {
+ PendingUnlinkEntry *entry = (PendingUnlinkEntry *) linitial(pendingUnlinks);
+ char *path;
+
+ /*
+ * New entries are appended to the end, so if the entry is new we've
+ * reached the end of old entries.
+ *
+ * Note: if just the right number of consecutive checkpoints fail, we
+ * could be fooled here by cycle_ctr wraparound. However, the only
+ * consequence is that we'd delay unlinking for one more checkpoint,
+ * which is perfectly tolerable.
+ */
+ if (entry->cycle_ctr == ckpt_cycle_ctr)
+ break;
+
+ /* Unlink the file */
+ path = relpathperm(entry->rnode, MAIN_FORKNUM);
+ if (unlink(path) < 0)
+ {
+ /*
+ * There's a race condition, when the database is dropped at the
+ * same time that we process the pending unlink requests. If the
+ * DROP DATABASE deletes the file before we do, we will get ENOENT
+ * here. rmtree() also has to ignore ENOENT errors, to deal with
+ * the possibility that we delete the file first.
+ */
+ if (errno != ENOENT)
+ ereport(WARNING,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+ }
+ pfree(path);
+
+ /* And remove the list entry */
+ pendingUnlinks = list_delete_first(pendingUnlinks);
+ pfree(entry);
+
+ /*
+ * As in smgrsync, we don't want to stop absorbing fsync requests for a
+ * long time when there are many deletions to be done. We can safely
+ * call AbsorbFsyncRequests() at this point in the loop (note it might
+ * try to delete list entries).
+ */
+ if (--absorb_counter <= 0)
+ {
+ AbsorbFsyncRequests();
+ absorb_counter = UNLINKS_PER_ABSORB;
+ }
+ }
+}
+
+
+/*
+ * Mark a file as needing fsync.
+ *
+ * If there is a local pending-ops table, just make an entry in it for
+ * smgrsync to process later. Otherwise, try to pass off the fsync request to
+ * the checkpointer process.
+ *
+ * Returns true on success, but false if the queue was full and we couldn't
+ * pass the request to the the checkpointer, meaning that the caller must
+ * perform the fsync.
+ */
+bool
+FsyncAtCheckpoint(RelFileNode rnode, ForkNumber forknum, SegmentNumber segno)
+{
+ if (pendingFsyncTable)
+ {
+ RememberFsyncRequest(FSYNC_SEGMENT_REQUEST, rnode, forknum, segno);
+ return true;
+ }
+ else
+ return ForwardFsyncRequest(FSYNC_SEGMENT_REQUEST, rnode, forknum,
+ segno);
+}
+
+/*
+ * Schedule a file to be deleted after next checkpoint.
+ *
+ * As with FsyncAtCheckpoint, this could involve either a local or a remote
+ * pending-ops table.
+ */
+void
+UnlinkAfterCheckpoint(RelFileNodeBackend rnode)
+{
+ /* Should never be used with temp relations */
+ Assert(!RelFileNodeBackendIsTemp(rnode));
+
+ if (pendingFsyncTable)
+ {
+ /* push it into local pending-ops table */
+ RememberFsyncRequest(UNLINK_RELATION_REQUEST,
+ rnode.node,
+ MAIN_FORKNUM,
+ InvalidSegmentNumber);
+ }
+ else
+ {
+ /* Notify the checkpointer about it. */
+ Assert(IsUnderPostmaster);
+
+ ForwardFsyncRequest(UNLINK_RELATION_REQUEST,
+ rnode.node,
+ MAIN_FORKNUM,
+ InvalidSegmentNumber);
+ }
+}
+
+/*
+ * In archive recovery, we rely on checkpointer to do fsyncs, but we will have
+ * already created the pendingFsyncTable during initialization of the startup
+ * process. Calling this function drops the local pendingFsyncTable so that
+ * subsequent requests will be forwarded to checkpointer.
+ */
+void
+SetForwardFsyncRequests(void)
+{
+ /* Perform any pending fsyncs we may have queued up, then drop table */
+ if (pendingFsyncTable)
+ {
+ smgrsync();
+ hash_destroy(pendingFsyncTable);
+ }
+ pendingFsyncTable = NULL;
+
+ /*
+ * We should not have any pending unlink requests, since mdunlink doesn't
+ * queue unlink requests when isRedo.
+ */
+ Assert(pendingUnlinks == NIL);
+}
+
+/*
+ * Find and remove a segment number by binary search.
+ */
+static inline void
+delete_segno(segnum_vector *vec, SegmentNumber segno)
+{
+ SegmentNumber *position =
+ segnum_array_lower_bound(segnum_vector_begin(vec),
+ segnum_vector_end(vec),
+ &segno);
+
+ if (position != segnum_vector_end(vec) &&
+ *position == segno)
+ segnum_vector_erase(vec, position);
+}
+
+/*
+ * Add a segment number by binary search. Hopefully these tend to be added a
+ * the high end, which is cheap.
+ */
+static inline void
+insert_segno(segnum_vector *vec, SegmentNumber segno)
+{
+ segnum_vector_insert(vec,
+ segnum_array_lower_bound(segnum_vector_begin(vec),
+ segnum_vector_end(vec),
+ &segno),
+ &segno);
+}
+
+/*
+ * RememberFsyncRequest() -- callback from checkpointer side of fsync request
+ *
+ * We stuff fsync requests into the local hash table for execution
+ * during the checkpointer's next checkpoint. UNLINK requests go into a
+ * separate linked list, however, because they get processed separately.
+ *
+ * Valid valid values for 'type':
+ * - FSYNC_SEGMENT_REQUEST means to schedule an fsync
+ * - FORGET_SEGMENT_FSYNC means to cancel pending fsyncs for one segment
+ * - FORGET_RELATION_FSYNC means to cancel pending fsyncs for a relation,
+ * either for one fork, or all forks if forknum is InvalidForkNumber
+ * - FORGET_DATABASE_FSYNC means to cancel pending fsyncs for a whole database
+ * - UNLINK_RELATION_REQUEST is a request to delete the file after the next
+ * checkpoint.
+ * Note also that we're assuming real segment numbers don't exceed INT_MAX.
+ *
+ * (Handling FORGET_DATABASE_FSYNC requests is a tad slow because the hash
+ * table has to be searched linearly, but dropping a database is a pretty
+ * heavyweight operation anyhow, so we'll live with it.)
+ */
+void
+RememberFsyncRequest(int type, RelFileNode rnode, ForkNumber forknum,
+ SegmentNumber segno)
+{
+ Assert(pendingFsyncTable);
+
+ if (type == FORGET_SEGMENT_FSYNC || type == FORGET_RELATION_FSYNC)
+ {
+ PendingFsyncEntry *entry;
+
+ entry = hash_search(pendingFsyncTable, &rnode, HASH_FIND, NULL);
+ if (entry)
+ {
+ if (type == FORGET_SEGMENT_FSYNC)
+ {
+ delete_segno(&entry->requests[forknum], segno);
+ delete_segno(&entry->requests_in_progress[forknum], segno);
+ }
+ else if (forknum == InvalidForkNumber)
+ {
+ /* Remove requests for all forks. */
+ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
+ {
+ segnum_vector_reset(&entry->requests[forknum]);
+ segnum_vector_reset(&entry->requests_in_progress[forknum]);
+ }
+ }
+ else
+ {
+ /* Forget about all segments for one fork. */
+ segnum_vector_reset(&entry->requests[forknum]);
+ segnum_vector_reset(&entry->requests_in_progress[forknum]);
+ }
+ }
+ }
+ else if (type == FORGET_DATABASE_FSYNC)
+ {
+ HASH_SEQ_STATUS hstat;
+ PendingFsyncEntry *entry;
+
+ /* Remove fsync requests */
+ hash_seq_init(&hstat, pendingFsyncTable);
+ while ((entry = (PendingFsyncEntry *) hash_seq_search(&hstat)) != NULL)
+ {
+ if (rnode.dbNode == entry->rnode.dbNode)
+ {
+ /* Remove requests for all forks. */
+ for (forknum = 0; forknum <= MAX_FORKNUM; forknum++)
+ {
+ segnum_vector_reset(&entry->requests[forknum]);
+ segnum_vector_reset(&entry->requests_in_progress[forknum]);
+ }
+ }
+ }
+
+ /* Remove unlink requests */
+ if (segno == FORGET_DATABASE_FSYNC)
+ {
+ ListCell *cell,
+ *next,
+ *prev;
+
+ prev = NULL;
+ for (cell = list_head(pendingUnlinks); cell; cell = next)
+ {
+ PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
+
+ next = lnext(cell);
+ if (rnode.dbNode == entry->rnode.dbNode)
+ {
+ pendingUnlinks = list_delete_cell(pendingUnlinks, cell,
+ prev);
+ pfree(entry);
+ }
+ else
+ prev = cell;
+ }
+ }
+ }
+ else if (type == UNLINK_RELATION_REQUEST)
+ {
+ /* Unlink request: put it in the linked list */
+ MemoryContext oldcxt = MemoryContextSwitchTo(pendingOpsCxt);
+ PendingUnlinkEntry *entry;
+
+ /* PendingUnlinkEntry doesn't store forknum, since it's always MAIN */
+ Assert(forknum == MAIN_FORKNUM);
+
+ entry = palloc(sizeof(PendingUnlinkEntry));
+ entry->rnode = rnode;
+ entry->cycle_ctr = ckpt_cycle_ctr;
+
+ pendingUnlinks = lappend(pendingUnlinks, entry);
+
+ MemoryContextSwitchTo(oldcxt);
+ }
+ else if (type == FSYNC_SEGMENT_REQUEST)
+ {
+ /* Normal case: enter a request to fsync this segment */
+ PendingFsyncEntry *entry;
+ bool found;
+
+ entry = (PendingFsyncEntry *) hash_search(pendingFsyncTable,
+ &rnode,
+ HASH_ENTER,
+ &found);
+ /* if new entry, initialize it */
+ if (!found)
+ {
+ ForkNumber f;
+
+ entry->cycle_ctr = ckpt_cycle_ctr;
+ for (f = 0; f <= MAX_FORKNUM; f++)
+ {
+ segnum_vector_init(&entry->requests[f]);
+ segnum_vector_init(&entry->requests_in_progress[f]);
+ }
+ }
+
+ /*
+ * NB: it's intentional that we don't change cycle_ctr if the entry
+ * already exists. The cycle_ctr must represent the oldest fsync
+ * request that could be in the entry.
+ */
+
+ insert_segno(&entry->requests[forknum], segno);
+ }
+}
+
+/*
+ * ForgetSegmentFsyncRequests -- forget any fsyncs for one segment of a
+ * relation fork
+ *
+ * forknum == InvalidForkNumber means all forks, although this code doesn't
+ * actually know that, since it's just forwarding the request elsewhere.
+ */
+void
+ForgetSegmentFsyncRequests(RelFileNode rnode, ForkNumber forknum,
+ SegmentNumber segno)
+{
+ if (pendingFsyncTable)
+ {
+ /* standalone backend or startup process: fsync state is local */
+ RememberFsyncRequest(FORGET_SEGMENT_FSYNC, rnode, forknum, segno);
+ }
+ else if (IsUnderPostmaster)
+ {
+ /* Notify the checkpointer about it. */
+ while (!ForwardFsyncRequest(FORGET_SEGMENT_FSYNC, rnode, forknum,
+ segno))
+ pg_usleep(10000L); /* 10 msec seems a good number */
+
+ /*
+ * Note we don't wait for the checkpointer to actually absorb the
+ * cancel message; see smgrsync() for the implications.
+ */
+ }
+}
+
+/*
+ * ForgetRelationFsyncRequests -- forget any fsyncs for a relation fork
+ *
+ * forknum == InvalidForkNumber means all forks, although this code doesn't
+ * actually know that, since it's just forwarding the request elsewhere.
+ */
+void
+ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum)
+{
+ if (pendingFsyncTable)
+ {
+ /* standalone backend or startup process: fsync state is local */
+ RememberFsyncRequest(FORGET_RELATION_FSYNC, rnode, forknum,
+ InvalidSegmentNumber);
+ }
+ else if (IsUnderPostmaster)
+ {
+ /* Notify the checkpointer about it. */
+ while (!ForwardFsyncRequest(FORGET_RELATION_FSYNC, rnode, forknum,
+ InvalidSegmentNumber))
+ pg_usleep(10000L); /* 10 msec seems a good number */
+
+ /*
+ * Note we don't wait for the checkpointer to actually absorb the
+ * cancel message; see smgrsync() for the implications.
+ */
+ }
+}
+
+/*
+ * ForgetDatabaseFsyncRequests -- forget any fsyncs and unlinks for a DB
+ */
+void
+ForgetDatabaseFsyncRequests(Oid dbid)
+{
+ RelFileNode rnode;
+
+ rnode.dbNode = dbid;
+ rnode.spcNode = 0;
+ rnode.relNode = 0;
+
+ if (pendingFsyncTable)
+ {
+ /* standalone backend or startup process: fsync state is local */
+ RememberFsyncRequest(FORGET_DATABASE_FSYNC, rnode, 0,
+ InvalidSegmentNumber);
+ }
+ else if (IsUnderPostmaster)
+ {
+ /* see notes in ForgetRelationFsyncRequests */
+ while (!ForwardFsyncRequest(FORGET_DATABASE_FSYNC, rnode, 0,
+ InvalidSegmentNumber))
+ pg_usleep(10000L); /* 10 msec seems a good number */
+ }
+}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6ec795f1b4..41760b03de 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -59,7 +59,7 @@
#include "commands/view.h"
#include "miscadmin.h"
#include "parser/parse_utilcmd.h"
-#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "rewrite/rewriteDefine.h"
#include "rewrite/rewriteRemove.h"
#include "storage/fd.h"
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 41d477165c..8869e730dc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -61,6 +61,7 @@
#include "postmaster/autovacuum.h"
#include "postmaster/bgworker_internals.h"
#include "postmaster/bgwriter.h"
+#include "postmaster/checkpointer.h"
#include "postmaster/postmaster.h"
#include "postmaster/syslogger.h"
#include "postmaster/walwriter.h"
diff --git a/src/include/postmaster/bgwriter.h b/src/include/postmaster/bgwriter.h
index 53b8f5fe3c..585ce52667 100644
--- a/src/include/postmaster/bgwriter.h
+++ b/src/include/postmaster/bgwriter.h
@@ -1,10 +1,7 @@
/*-------------------------------------------------------------------------
*
* bgwriter.h
- * Exports from postmaster/bgwriter.c and postmaster/checkpointer.c.
- *
- * The bgwriter process used to handle checkpointing duties too. Now
- * there is a separate process, but we did not bother to split this header.
+ * Exports from postmaster/bgwriter.c.
*
* Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
*
@@ -15,29 +12,10 @@
#ifndef _BGWRITER_H
#define _BGWRITER_H
-#include "storage/block.h"
-#include "storage/relfilenode.h"
-
-
/* GUC options */
extern int BgWriterDelay;
-extern int CheckPointTimeout;
-extern int CheckPointWarning;
-extern double CheckPointCompletionTarget;
extern void BackgroundWriterMain(void) pg_attribute_noreturn();
-extern void CheckpointerMain(void) pg_attribute_noreturn();
-
-extern void RequestCheckpoint(int flags);
-extern void CheckpointWriteDelay(int flags, double progress);
-
-extern bool ForwardFsyncRequest(RelFileNode rnode, ForkNumber forknum,
- BlockNumber segno);
-extern void AbsorbFsyncRequests(void);
-
-extern Size CheckpointerShmemSize(void);
-extern void CheckpointerShmemInit(void);
-extern bool FirstCallSinceLastCheckpoint(void);
#endif /* _BGWRITER_H */
diff --git a/src/include/postmaster/checkpointer.h b/src/include/postmaster/checkpointer.h
new file mode 100644
index 0000000000..28b13c2d9c
--- /dev/null
+++ b/src/include/postmaster/checkpointer.h
@@ -0,0 +1,39 @@
+/*-------------------------------------------------------------------------
+ *
+ * checkpointer.h
+ * Exports from postmaster/checkpointer.c.
+ *
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
+ *
+ * src/include/postmaster/checkpointer.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef CHECKPOINTER_H
+#define CHECKPOINTER_H
+
+#include "common/relpath.h"
+#include "storage/block.h"
+#include "storage/relfilenode.h"
+
+/* GUC options */
+extern int CheckPointTimeout;
+extern int CheckPointWarning;
+extern double CheckPointCompletionTarget;
+
+extern void CheckpointerMain(void) pg_attribute_noreturn();
+extern bool ForwardFsyncRequest(int type, RelFileNode rnode,
+ ForkNumber forknum, BlockNumber segno);
+extern void RequestCheckpoint(int flags);
+extern void CheckpointWriteDelay(int flags, double progress);
+
+extern void AbsorbFsyncRequests(void);
+extern void AbsorbAllFsyncRequests(void);
+
+extern Size CheckpointerShmemSize(void);
+extern void CheckpointerShmemInit(void);
+
+extern bool FirstCallSinceLastCheckpoint(void);
+extern void CountBackendWrite(void);
+
+#endif
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 820d08ed4e..5a52229dcd 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -18,6 +18,15 @@
#include "storage/block.h"
#include "storage/relfilenode.h"
+/*
+ * The type used to identify segment numbers. Generally, segments are an
+ * internal detail of individual storage manager implementations, but since
+ * they appear in various places to allow them to be passed between processes,
+ * it seemed worthwhile to have a typename.
+ */
+typedef uint32 SegmentNumber;
+
+#define InvalidSegmentNumber ((SegmentNumber) 0xFFFFFFFF)
/*
* smgr.c maintains a table of SMgrRelation objects, which are essentially
@@ -105,10 +114,9 @@ extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
extern void smgrtruncate(SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
-extern void smgrimmedsync(SMgrRelation reln, ForkNumber forknum);
-extern void smgrpreckpt(void);
-extern void smgrsync(void);
-extern void smgrpostckpt(void);
+extern bool smgrimmedsync(SMgrRelation reln, ForkNumber forknum,
+ SegmentNumber segno);
+
extern void AtEOXact_SMgr(void);
@@ -133,16 +141,9 @@ extern void mdwriteback(SMgrRelation reln, ForkNumber forknum,
extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
extern void mdtruncate(SMgrRelation reln, ForkNumber forknum,
BlockNumber nblocks);
-extern void mdimmedsync(SMgrRelation reln, ForkNumber forknum);
-extern void mdpreckpt(void);
-extern void mdsync(void);
-extern void mdpostckpt(void);
-
-extern void SetForwardFsyncRequests(void);
-extern void RememberFsyncRequest(RelFileNode rnode, ForkNumber forknum,
- BlockNumber segno);
-extern void ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum);
-extern void ForgetDatabaseFsyncRequests(Oid dbid);
+extern bool mdimmedsync(SMgrRelation reln, ForkNumber forknum,
+ SegmentNumber segno);
+
extern void DropRelationFiles(RelFileNode *delrels, int ndelrels, bool isRedo);
#endif /* SMGR_H */
diff --git a/src/include/storage/smgrsync.h b/src/include/storage/smgrsync.h
new file mode 100644
index 0000000000..01a174e729
--- /dev/null
+++ b/src/include/storage/smgrsync.h
@@ -0,0 +1,36 @@
+/*-------------------------------------------------------------------------
+ *
+ * smgrsync.h
+ * management of file synchronization
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/storage/smgrpending.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef SMGRSYNC_H
+#define SMGRSYNC_H
+
+#include "storage/smgr.h"
+
+extern void smgrsync_init(void);
+extern void smgrpreckpt(void);
+extern void smgrsync(void);
+extern void smgrpostckpt(void);
+
+extern void UnlinkAfterCheckpoint(RelFileNodeBackend rnode);
+extern bool FsyncAtCheckpoint(RelFileNode rnode, ForkNumber forknum,
+ SegmentNumber segno);
+extern void RememberFsyncRequest(int type, RelFileNode rnode,
+ ForkNumber forknum, SegmentNumber segno);
+extern void SetForwardFsyncRequests(void);
+extern void ForgetSegmentFsyncRequests(RelFileNode rnode, ForkNumber forknum,
+ SegmentNumber segno);
+extern void ForgetRelationFsyncRequests(RelFileNode rnode, ForkNumber forknum);
+extern void ForgetDatabaseFsyncRequests(Oid dbid);
+
+
+#endif
--
2.16.5
--6TrnltStXW4iwmi0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: attachment;
filename="0002-Refactor-the-fsync-machinery-to-support-future-SM-v8.patch"
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v2] pg_wal on COW fs
@ 2019-02-27 22:41 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Alvaro Herrera @ 2019-02-27 22:41 UTC (permalink / raw)
---
doc/src/sgml/config.sgml | 20 ++++
src/backend/access/transam/xlog.c | 101 ++++++++++++------
src/backend/utils/misc/guc.c | 13 +++
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/access/xlog.h | 1 +
5 files changed, 102 insertions(+), 34 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8bd57f376b2..60a873273aa 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2959,6 +2959,26 @@ include_dir 'conf.d'
</listitem>
</varlistentry>
+ <varlistentry id="guc-wal-cow-filesystem" xreflabel="wal_cow_filesystem">
+ <term><varname>wal_cow_filesystem</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>wal_cow_filesystem</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ This parameter should only be set to <literal>on</literal> when the WAL
+ resides on a <firstterm>Copy-On-Write</firstterm> (<acronym>COW</acronym>)
+ filesystem.
+ Enabling this option adjusts some behavior to take advantage of the
+ filesystem characteristics (for example, recycling WAL files and
+ zero-filling new WAL files are disabled).
+ This setting is only appropriate for filesystems which
+ allocate new disk blocks on every write.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
<sect2 id="runtime-config-wal-archiving">
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ecd12fc53ae..1acce1c70d6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -94,6 +94,7 @@ bool wal_log_hints = false;
bool wal_compression = false;
char *wal_consistency_checking_string = NULL;
bool *wal_consistency_checking = NULL;
+bool wal_cow_filesystem = false;
bool log_checkpoints = false;
int sync_method = DEFAULT_SYNC_METHOD;
int wal_level = WAL_LEVEL_MINIMAL;
@@ -3216,6 +3217,7 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
XLogSegNo max_segno;
int fd;
int nbytes;
+ bool fail;
XLogFilePath(path, ThisTimeLineID, logsegno, wal_segment_size);
@@ -3255,41 +3257,65 @@ XLogFileInit(XLogSegNo logsegno, bool *use_existent, bool use_lock)
(errcode_for_file_access(),
errmsg("could not create file \"%s\": %m", tmppath)));
- /*
- * Zero-fill the file. We have to do this the hard way to ensure that all
- * the file space has really been allocated --- on platforms that allow
- * "holes" in files, just seeking to the end doesn't allocate intermediate
- * space. This way, we know that we have all the space and (after the
- * fsync below) that all the indirect blocks are down on disk. Therefore,
- * fdatasync(2) or O_DSYNC will be sufficient to sync future writes to the
- * log file.
- */
memset(zbuffer.data, 0, XLOG_BLCKSZ);
- for (nbytes = 0; nbytes < wal_segment_size; nbytes += XLOG_BLCKSZ)
+
+ /*
+ * Ensure the filesystem has physically allocated disk space for the data.
+ */
+ if (!wal_cow_filesystem)
{
+ /*
+ * In non-CoW filesystems, zero-fill the file. We have to do this the
+ * hard way to ensure that all the file space has really been
+ * allocated --- on platforms that allow "holes" in files, just seeking
+ * to the end doesn't allocate intermediate space. This way, we know
+ * that we have all the space and (after the fsync below) that all the
+ * indirect blocks are down on disk. Therefore, fdatasync(2) or
+ * O_DSYNC will be sufficient to sync future writes to the log file.
+ */
+ fail = false; /* keep compiler quiet */
+ for (nbytes = 0; nbytes < wal_segment_size; nbytes += XLOG_BLCKSZ)
+ {
+ errno = 0;
+ pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_WRITE);
+ fail = (int) write(fd, zbuffer.data, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ;
+ pgstat_report_wait_end();
+ if (fail)
+ break;
+ }
+ }
+ else
+ {
+ /*
+ * In CoW filesystems, seeking to the end and writing a solitary byte
+ * is enough.
+ */
errno = 0;
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_WRITE);
- if ((int) write(fd, zbuffer.data, XLOG_BLCKSZ) != (int) XLOG_BLCKSZ)
- {
- int save_errno = errno;
- /*
- * If we fail to make the file, delete it to release disk space
- */
- unlink(tmppath);
+ fail = lseek(fd, (off_t) (wal_segment_size - 1), SEEK_SET) < (off_t) 0 ||
+ (int) write(fd, zbuffer.data, 1) != (int) 1;
- close(fd);
-
- /* if write didn't set errno, assume problem is no disk space */
- errno = save_errno ? save_errno : ENOSPC;
-
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not write to file \"%s\": %m", tmppath)));
- }
pgstat_report_wait_end();
}
+ if (fail)
+ {
+ int save_errno = errno;
+
+ /* If we fail to make the file, delete it to release disk space */
+ unlink(tmppath);
+
+ close(fd);
+
+ /* if write didn't set errno, assume problem is no disk space */
+ errno = save_errno ? save_errno : ENOSPC;
+
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not write to file \"%s\": %m", tmppath)));
+ }
+
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
if (pg_fsync(fd) != 0)
{
@@ -4053,14 +4079,17 @@ RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
XLogSegNo endlogSegNo;
XLogSegNo recycleSegNo;
- /*
- * Initialize info about where to try to recycle to.
- */
- XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
- if (RedoRecPtr == InvalidXLogRecPtr)
- recycleSegNo = endlogSegNo + 10;
+ /* Initialize info about where to try to recycle to, if needed. */
+ if (!wal_cow_filesystem)
+ {
+ XLByteToSeg(endptr, endlogSegNo, wal_segment_size);
+ if (RedoRecPtr == InvalidXLogRecPtr)
+ recycleSegNo = endlogSegNo + 10;
+ else
+ recycleSegNo = XLOGfileslop(RedoRecPtr);
+ }
else
- recycleSegNo = XLOGfileslop(RedoRecPtr);
+ recycleSegNo = (XLogSegNo) 0; /* keep compiler quiet */
snprintf(path, MAXPGPATH, XLOGDIR "/%s", segname);
@@ -4068,8 +4097,12 @@ RemoveXlogFile(const char *segname, XLogRecPtr RedoRecPtr, XLogRecPtr endptr)
* Before deleting the file, see if it can be recycled as a future log
* segment. Only recycle normal files, pg_standby for example can create
* symbolic links pointing to a separate archive directory.
+ *
+ * Skip recycling on COW filesystems, though. It's better to create
+ * new files each time.
*/
- if (endlogSegNo <= recycleSegNo &&
+ if (!wal_cow_filesystem &&
+ endlogSegNo <= recycleSegNo &&
lstat(path, &statbuf) == 0 && S_ISREG(statbuf.st_mode) &&
InstallXLogFileSegment(&endlogSegNo, path,
true, recycleSegNo, true))
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 156d147c85b..cbcf475885e 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1177,6 +1177,19 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"wal_cow_filesystem", PGC_SUSET, WAL_SETTINGS,
+ gettext_noop("WAL is stored on Copy-On-Write file system."),
+ gettext_noop("This option adjusts behavior to take advantage of "
+ "filesystem characteristics specific to CoW filesystems, "
+ "improving performance. "
+ "It should be enabled on ZFS and other similar filesystems.")
+ },
+ &wal_cow_filesystem,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"log_checkpoints", PGC_SIGHUP, LOGGING_WHAT,
gettext_noop("Logs each checkpoint."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 194f3120964..c9254c04bab 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -206,6 +206,7 @@
#wal_compression = off # enable compression of full-page writes
#wal_log_hints = off # also do full page writes of non-critical updates
# (change requires restart)
+#wal_cow_filesystem = off # is pg_wal on a Copy-on-Write filesystem?
#wal_buffers = -1 # min 32kB, -1 sets based on shared_buffers
# (change requires restart)
#wal_writer_delay = 200ms # 1-10000 milliseconds
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index f90a6a91391..fa9d69affe4 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -116,6 +116,7 @@ extern bool EnableHotStandby;
extern bool fullPageWrites;
extern bool wal_log_hints;
extern bool wal_compression;
+extern bool wal_cow_filesystem;
extern bool *wal_consistency_checking;
extern char *wal_consistency_checking_string;
extern bool log_checkpoints;
--
2.17.1
--BOKacYhQ+x31HxR3--
^ permalink raw reply [nested|flat] 17+ messages in thread
* [PATCH v4 01/12] Change section heading to better reflect saving a result in variable(s)
@ 2023-09-24 20:49 Karl O. Pinc <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Karl O. Pinc @ 2023-09-24 20:49 UTC (permalink / raw)
The current section title of "Executing a Command with a Single-Row
Result" does not reflect what the section is really about. Other
sections make clear how to _execute_ commands, single-row result or not.
What this section is about is how to _save_ a single row of results into
variable(s).
It would be nice to talk about saving results into variables in the
section heading but I couldn't come up with anything pithy. "Saving a
Single-Row of a Command's Result" seems good enough, especially since
there's few other places to save results other than in variables.
---
doc/src/sgml/plpgsql.sgml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index f55e901c7e..8747e84245 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -1126,7 +1126,7 @@ PERFORM create_mv('cs_session_page_requests_mv', my_query);
</sect2>
<sect2 id="plpgsql-statements-sql-onerow">
- <title>Executing a Command with a Single-Row Result</title>
+ <title>Saving a Single-Row of a Command's Result</title>
<indexterm zone="plpgsql-statements-sql-onerow">
<primary>SELECT INTO</primary>
--
2.30.2
--MP_/OOXZvOwbpccKfGOtE9/SwX6
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename=v4-0002-Change-section-heading-to-better-describe-referen.patch
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
@ 2025-03-06 22:25 Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-06 22:25 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Wed, Mar 5, 2025 at 8:08 PM Michael Paquier <[email protected]> wrote:
> Honestly, I don't see a reason not to introduce that, like in the
> attached.
This new code races against the session timeout. I see this on timer expiration:
[14:19:55.224](0.000s) # issuing query 34 via background psql:
SELECT state FROM pg_stat_activity WHERE pid = ;
[14:19:55.228](0.004s) # pump_until: process terminated
unexpectedly when searching for "(?^:(^|\n)background_psql:
QUERY_SEPARATOR 34:\r?\n)" with stream: ""
process ended prematurely at
/home/jacob/src/postgres/src/test/perl/PostgreSQL/Test/Utils.pm line
439.
Which makes it seem like some sort of crash, IMO. I don't find that as
easily debuggable as the previous log message, which was
[14:21:33.104](0.001s) # issuing query 32 via background psql:
SELECT pid FROM pg_stat_activity
# WHERE state = 'starting' and wait_event = 'init-pre-auth';
IPC::Run: timeout on timer #1 at
/home/jacob/perl5/lib/perl5/IPC/Run.pm line 3007.
> + WHERE state = 'starting' and wait_event = 'init-pre-auth';});
Did you have thoughts on expanding the check to backend_type [1]?
> + # Give up. The output of the last attempt is logged by query(),
> + # so no need to do anything here.
> + return 0;
One of my primary complaints about the poll_query_until()
implementation is that "giving up" in this case means continuing to
run pieces of the test that have no business running after a timeout,
and increasing the log noise after a failure. I'm not sure how loudly
to complain in this particular case, since I know we use it
elsewhere...
Thanks!
--Jacob
[1] https://postgr.es/m/CAOYmi%2BnxNCQcTQE-tQ7Lwpe4cYc1u-yxwEe5kt2AVN%2BDXXVVbQ%40mail.gmail.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-06 23:15 ` Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2025-03-06 23:15 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 06, 2025 at 02:25:07PM -0800, Jacob Champion wrote:
> On Wed, Mar 5, 2025 at 8:08 PM Michael Paquier <[email protected]> wrote:
>> + WHERE state = 'starting' and wait_event = 'init-pre-auth';});
>
> Did you have thoughts on expanding the check to backend_type [1]?
>
>> + # Give up. The output of the last attempt is logged by query(),
>> + # so no need to do anything here.
>> + return 0;
>
> One of my primary complaints about the poll_query_until()
> implementation is that "giving up" in this case means continuing to
> run pieces of the test that have no business running after a timeout,
> and increasing the log noise after a failure. I'm not sure how loudly
> to complain in this particular case, since I know we use it
> elsewhere...
Indeed. The existing poll_query_until() is a bit more reliable in
terms of error handling, even with a very low PG_TEST_TIMEOUT_DEFAULT.
A second thing that was bugging me on a second lookup this morning is
how we should handle error cases. A background psql process depends
on what the caller defines for ON_ERROR_STOP. In the case of this
test, we're OK to fail immediately because we expect the queries to
always work. I'm not sure if this is fine by default, especially if
callers of this routine expect to have the same properties as
poll_query_until() in Cluster.pm. They would not, because a
BackgroundPsql is an entire different object, except if given options
when staring psql to act like that.
I have applied the simplest patch for now, to silence the failures in
the CI, and included your suggestion to add a check on the
backend_type for the extra safety it offers.
I'd like the addition of the poll_query_until() in the long-term, but
I'm really not sure if the semantics would be right this way under a
background psql. In the auth 007 test, they would be OK, but it could
be surprising if we have other callers that begin relying on it.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
@ 2025-03-06 23:39 ` Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-06 23:39 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 6, 2025 at 3:15 PM Michael Paquier <[email protected]> wrote:
> I have applied the simplest patch for now, to silence the failures in
> the CI, and included your suggestion to add a check on the
> backend_type for the extra safety it offers.
Thanks! Initial CI run looks green, so that's a good start.
I've reattached the wait event patches, to get the cfbot back to where it was.
> I'd like the addition of the poll_query_until() in the long-term, but
> I'm really not sure if the semantics would be right this way under a
> background psql. In the auth 007 test, they would be OK, but it could
> be surprising if we have other callers that begin relying on it.
Yeah, that API is definitely subtle.
--Jacob
Attachments:
[application/octet-stream] v11-0001-Report-external-auth-calls-as-wait-events.patch (13.0K, ../../CAOYmi+kHY8HZ5RdXGT1bVZzkAq7ta=--YSGehLr35HRjF4Fyug@mail.gmail.com/2-v11-0001-Report-external-auth-calls-as-wait-events.patch)
download | inline diff:
From 0dc8e12758a1e7bdde7e5e4e60570b8b4f75d240 Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 3 May 2024 15:58:23 -0700
Subject: [PATCH v11 1/2] Report external auth calls as wait events
Introduce a new "Auth" wait class for various external authentication
systems, to make it obvious what's going wrong if one of those systems
hangs. Each new wait event is unique in order to more easily pinpoint
problematic locations in the code.
Discussion: https://postgr.es/m/CAOYmi%2B%3D60deN20WDyCoHCiecgivJxr%3D98s7s7-C8SkXwrCfHXg%40mail.gmail.com
---
doc/src/sgml/monitoring.sgml | 8 +++
src/backend/libpq/auth.c | 56 +++++++++++++++++--
src/backend/utils/activity/wait_event.c | 11 ++++
.../utils/activity/wait_event_names.txt | 27 +++++++++
src/include/utils/wait_event.h | 1 +
src/test/regress/expected/sysviews.out | 3 +-
6 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 16646f560e8..877163fb403 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1045,6 +1045,14 @@ postgres 27093 0.0 0.0 30096 2752 ? Ss 11:34 0:00 postgres: ser
see <xref linkend="wait-event-activity-table"/>.
</entry>
</row>
+ <row>
+ <entry><literal>Auth</literal></entry>
+ <entry>The server process is waiting for an external system to
+ authenticate and/or authorize the client connection.
+ <literal>wait_event</literal> will identify the specific wait point;
+ see <xref linkend="wait-event-auth-table"/>.
+ </entry>
+ </row>
<row>
<entry><literal>BufferPin</literal></entry>
<entry>The server process is waiting for exclusive access to
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 81e2f8184e3..bd8a2a098b3 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -39,6 +39,7 @@
#include "replication/walsender.h"
#include "storage/ipc.h"
#include "utils/memutils.h"
+#include "utils/wait_event.h"
/*----------------------------------------------------------------
* Global authentication functions
@@ -990,6 +991,7 @@ pg_GSS_recvauth(Port *port)
elog(DEBUG4, "processing received GSS token of length %u",
(unsigned int) gbuf.length);
+ pgstat_report_wait_start(WAIT_EVENT_GSSAPI_ACCEPT_SEC_CONTEXT);
maj_stat = gss_accept_sec_context(&min_stat,
&port->gss->ctx,
port->gss->cred,
@@ -1001,6 +1003,7 @@ pg_GSS_recvauth(Port *port)
&gflags,
NULL,
pg_gss_accept_delegation ? &delegated_creds : NULL);
+ pgstat_report_wait_end();
/* gbuf no longer used */
pfree(buf.data);
@@ -1212,6 +1215,7 @@ pg_SSPI_recvauth(Port *port)
/*
* Acquire a handle to the server credentials.
*/
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACQUIRE_CREDENTIALS_HANDLE);
r = AcquireCredentialsHandle(NULL,
"negotiate",
SECPKG_CRED_INBOUND,
@@ -1221,6 +1225,8 @@ pg_SSPI_recvauth(Port *port)
NULL,
&sspicred,
&expiry);
+ pgstat_report_wait_end();
+
if (r != SEC_E_OK)
pg_SSPI_error(ERROR, _("could not acquire SSPI credentials"), r);
@@ -1286,6 +1292,7 @@ pg_SSPI_recvauth(Port *port)
elog(DEBUG4, "processing received SSPI token of length %u",
(unsigned int) buf.len);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_ACCEPT_SECURITY_CONTEXT);
r = AcceptSecurityContext(&sspicred,
sspictx,
&inbuf,
@@ -1295,6 +1302,7 @@ pg_SSPI_recvauth(Port *port)
&outbuf,
&contextattr,
NULL);
+ pgstat_report_wait_end();
/* input buffer no longer used */
pfree(buf.data);
@@ -1392,11 +1400,13 @@ pg_SSPI_recvauth(Port *port)
CloseHandle(token);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_LOOKUP_ACCOUNT_SID);
if (!LookupAccountSid(NULL, tokenuser->User.Sid, accountname, &accountnamesize,
domainname, &domainnamesize, &accountnameuse))
ereport(ERROR,
(errmsg_internal("could not look up account SID: error code %lu",
GetLastError())));
+ pgstat_report_wait_end();
free(tokenuser);
@@ -1493,8 +1503,11 @@ pg_SSPI_make_upn(char *accountname,
*/
samname = psprintf("%s\\%s", domainname, accountname);
+
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_TRANSLATE_NAME);
res = TranslateName(samname, NameSamCompatible, NameUserPrincipal,
NULL, &upnamesize);
+ pgstat_report_wait_end();
if ((!res && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
|| upnamesize == 0)
@@ -1509,8 +1522,10 @@ pg_SSPI_make_upn(char *accountname,
/* upnamesize includes the terminating NUL. */
upname = palloc(upnamesize);
+ pgstat_report_wait_start(WAIT_EVENT_SSPI_TRANSLATE_NAME);
res = TranslateName(samname, NameSamCompatible, NameUserPrincipal,
upname, &upnamesize);
+ pgstat_report_wait_end();
pfree(samname);
if (res)
@@ -2109,7 +2124,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_AUTHENTICATE);
retval = pam_authenticate(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2122,7 +2139,9 @@ CheckPAMAuth(Port *port, const char *user, const char *password)
return pam_no_password ? STATUS_EOF : STATUS_ERROR;
}
+ pgstat_report_wait_start(WAIT_EVENT_PAM_ACCT_MGMT);
retval = pam_acct_mgmt(pamh, 0);
+ pgstat_report_wait_end();
if (retval != PAM_SUCCESS)
{
@@ -2262,7 +2281,11 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
}
/* Look up a list of LDAP server hosts and port numbers */
- if (ldap_domain2hostlist(domain, &hostlist))
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_HOST_LOOKUP);
+ r = ldap_domain2hostlist(domain, &hostlist);
+ pgstat_report_wait_end();
+
+ if (r)
{
ereport(LOG,
(errmsg("LDAP authentication could not find DNS SRV records for \"%s\"",
@@ -2356,11 +2379,15 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
if (port->hba->ldaptls)
{
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_START_TLS);
#ifndef WIN32
- if ((r = ldap_start_tls_s(*ldap, NULL, NULL)) != LDAP_SUCCESS)
+ r = ldap_start_tls_s(*ldap, NULL, NULL);
#else
- if ((r = ldap_start_tls_s(*ldap, NULL, NULL, NULL, NULL)) != LDAP_SUCCESS)
+ r = ldap_start_tls_s(*ldap, NULL, NULL, NULL, NULL);
#endif
+ pgstat_report_wait_end();
+
+ if (r != LDAP_SUCCESS)
{
ereport(LOG,
(errmsg("could not start LDAP TLS session: %s",
@@ -2520,9 +2547,12 @@ CheckLDAPAuth(Port *port)
* Bind with a pre-defined username/password (if available) for
* searching. If none is specified, this turns into an anonymous bind.
*/
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND_FOR_SEARCH);
r = ldap_simple_bind_s(ldap,
port->hba->ldapbinddn ? port->hba->ldapbinddn : "",
port->hba->ldapbindpasswd ? ldap_password_hook(port->hba->ldapbindpasswd) : "");
+ pgstat_report_wait_end();
+
if (r != LDAP_SUCCESS)
{
ereport(LOG,
@@ -2545,6 +2575,8 @@ CheckLDAPAuth(Port *port)
filter = psprintf("(uid=%s)", port->user_name);
search_message = NULL;
+
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_SEARCH);
r = ldap_search_s(ldap,
port->hba->ldapbasedn,
port->hba->ldapscope,
@@ -2552,6 +2584,7 @@ CheckLDAPAuth(Port *port)
attributes,
0,
&search_message);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -2620,7 +2653,9 @@ CheckLDAPAuth(Port *port)
port->user_name,
port->hba->ldapsuffix ? port->hba->ldapsuffix : "");
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_BIND);
r = ldap_simple_bind_s(ldap, fulluser, passwd);
+ pgstat_report_wait_end();
if (r != LDAP_SUCCESS)
{
@@ -3069,8 +3104,12 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
return STATUS_ERROR;
}
- if (sendto(sock, radius_buffer, packetlength, 0,
- serveraddrs[0].ai_addr, serveraddrs[0].ai_addrlen) < 0)
+ pgstat_report_wait_start(WAIT_EVENT_RADIUS_SENDTO);
+ r = sendto(sock, radius_buffer, packetlength, 0,
+ serveraddrs[0].ai_addr, serveraddrs[0].ai_addrlen);
+ pgstat_report_wait_end();
+
+ if (r < 0)
{
ereport(LOG,
(errmsg("could not send RADIUS packet: %m")));
@@ -3118,7 +3157,10 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
FD_ZERO(&fdset);
FD_SET(sock, &fdset);
+ pgstat_report_wait_start(WAIT_EVENT_RADIUS_WAIT);
r = select(sock + 1, &fdset, NULL, NULL, &timeout);
+ pgstat_report_wait_end();
+
if (r < 0)
{
if (errno == EINTR)
@@ -3151,8 +3193,12 @@ PerformRadiusTransaction(const char *server, const char *secret, const char *por
*/
addrsize = sizeof(remoteaddr);
+
+ pgstat_report_wait_start(WAIT_EVENT_RADIUS_RECVFROM);
packetlength = recvfrom(sock, receive_buffer, RADIUS_BUFFER_SIZE, 0,
(struct sockaddr *) &remoteaddr, &addrsize);
+ pgstat_report_wait_end();
+
if (packetlength < 0)
{
ereport(LOG,
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index d9b8f34a355..6cc3e1e7c7a 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -34,6 +34,7 @@ static const char *pgstat_get_wait_client(WaitEventClient w);
static const char *pgstat_get_wait_ipc(WaitEventIPC w);
static const char *pgstat_get_wait_timeout(WaitEventTimeout w);
static const char *pgstat_get_wait_io(WaitEventIO w);
+static const char *pgstat_get_wait_auth(WaitEventAuth w);
static uint32 local_my_wait_event_info;
@@ -413,6 +414,9 @@ pgstat_get_wait_event_type(uint32 wait_event_info)
case PG_WAIT_INJECTIONPOINT:
event_type = "InjectionPoint";
break;
+ case PG_WAIT_AUTH:
+ event_type = "Auth";
+ break;
default:
event_type = "???";
break;
@@ -495,6 +499,13 @@ pgstat_get_wait_event(uint32 wait_event_info)
event_name = pgstat_get_wait_io(w);
break;
}
+ case PG_WAIT_AUTH:
+ {
+ WaitEventAuth w = (WaitEventAuth) wait_event_info;
+
+ event_name = pgstat_get_wait_auth(w);
+ break;
+ }
default:
event_name = "unknown wait event";
break;
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index e199f071628..a2852225614 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -162,6 +162,33 @@ XACT_GROUP_UPDATE "Waiting for the group leader to update transaction status at
ABI_compatibility:
+#
+# Wait Events - Auth
+#
+# Use this category when a process is waiting for a third party to
+# authenticate/authorize the user.
+#
+
+Section: ClassName - WaitEventAuth
+
+GSSAPI_ACCEPT_SEC_CONTEXT "Waiting for a response from a Kerberos server via GSSAPI."
+LDAP_BIND "Waiting for an LDAP bind operation to authenticate the user."
+LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory."
+LDAP_HOST_LOOKUP "Waiting for DNS lookup of LDAP servers."
+LDAP_SEARCH "Waiting for an LDAP search operation to complete."
+LDAP_START_TLS "Waiting for an LDAP StartTLS exchange."
+PAM_ACCT_MGMT "Waiting for the PAM service to validate the user account."
+PAM_AUTHENTICATE "Waiting for the PAM service to authenticate the user."
+RADIUS_RECVFROM "Waiting for a <function>recvfrom</function> call during a RADIUS transaction."
+RADIUS_SENDTO "Waiting for a <function>sendto</function> call during a RADIUS transaction."
+RADIUS_WAIT "Waiting for a RADIUS server to respond."
+SSPI_ACCEPT_SECURITY_CONTEXT "Waiting for a Windows security provider to accept the client's SSPI token."
+SSPI_ACQUIRE_CREDENTIALS_HANDLE "Waiting for a Windows security provider to acquire server credentials for SSPI."
+SSPI_LOOKUP_ACCOUNT_SID "Waiting for Windows to find the user's security identifier."
+SSPI_TRANSLATE_NAME "Waiting for Windows to translate a Kerberos UPN."
+
+ABI_compatibility:
+
#
# Wait Events - Timeout
#
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index b8cb3e5a430..3d995a9e5be 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -25,6 +25,7 @@
#define PG_WAIT_TIMEOUT 0x09000000U
#define PG_WAIT_IO 0x0A000000U
#define PG_WAIT_INJECTIONPOINT 0x0B000000U
+#define PG_WAIT_AUTH 0x0C000000U
/* enums for wait events */
#include "utils/wait_event_types.h"
diff --git a/src/test/regress/expected/sysviews.out b/src/test/regress/expected/sysviews.out
index 83228cfca29..b94930658b0 100644
--- a/src/test/regress/expected/sysviews.out
+++ b/src/test/regress/expected/sysviews.out
@@ -181,6 +181,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
type | ok
-----------+----
Activity | t
+ Auth | t
BufferPin | t
Client | t
Extension | t
@@ -189,7 +190,7 @@ select type, count(*) > 0 as ok FROM pg_wait_events
LWLock | t
Lock | t
Timeout | t
-(9 rows)
+(10 rows)
-- Test that the pg_timezone_names and pg_timezone_abbrevs views are
-- more-or-less working. We can't test their contents in any great detail
--
2.34.1
[application/octet-stream] v11-0002-squash-Report-external-auth-calls-as-wait-events.patch (5.3K, ../../CAOYmi+kHY8HZ5RdXGT1bVZzkAq7ta=--YSGehLr35HRjF4Fyug@mail.gmail.com/3-v11-0002-squash-Report-external-auth-calls-as-wait-events.patch)
download | inline diff:
From 15dd9dfcded95024f6e148d4a2a216ebb3e7ed1b Mon Sep 17 00:00:00 2001
From: Jacob Champion <[email protected]>
Date: Fri, 8 Nov 2024 14:19:26 -0800
Subject: [PATCH v11 2/2] squash! Report external auth calls as wait events
Add a wait event around all calls to ldap_unbind(). (For the record, I
do not want to implement this in this way.)
---
src/backend/libpq/auth.c | 18 ++++++++++++++++++
.../utils/activity/wait_event_names.txt | 9 +++++++++
2 files changed, 27 insertions(+)
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index bd8a2a098b3..34cc145dc5e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2373,7 +2373,9 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not set LDAP protocol version: %s",
ldap_err2string(r)),
errdetail_for_ldap(*ldap)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_SET_OPTION);
ldap_unbind(*ldap);
+ pgstat_report_wait_end();
return STATUS_ERROR;
}
@@ -2393,7 +2395,9 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
(errmsg("could not start LDAP TLS session: %s",
ldap_err2string(r)),
errdetail_for_ldap(*ldap)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_START_TLS);
ldap_unbind(*ldap);
+ pgstat_report_wait_end();
return STATUS_ERROR;
}
}
@@ -2537,7 +2541,9 @@ CheckLDAPAuth(Port *port)
{
ereport(LOG,
(errmsg("invalid character in user name for LDAP authentication")));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_NAME_CHECK);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
return STATUS_ERROR;
}
@@ -2561,7 +2567,9 @@ CheckLDAPAuth(Port *port)
server_name,
ldap_err2string(r)),
errdetail_for_ldap(ldap)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_BIND_FOR_SEARCH);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
return STATUS_ERROR;
}
@@ -2594,7 +2602,9 @@ CheckLDAPAuth(Port *port)
errdetail_for_ldap(ldap)));
if (search_message != NULL)
ldap_msgfree(search_message);
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_SEARCH);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
pfree(filter);
return STATUS_ERROR;
@@ -2616,7 +2626,9 @@ CheckLDAPAuth(Port *port)
count,
filter, server_name, count)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_COUNT_ENTRIES);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
pfree(filter);
ldap_msgfree(search_message);
@@ -2635,7 +2647,9 @@ CheckLDAPAuth(Port *port)
filter, server_name,
ldap_err2string(error)),
errdetail_for_ldap(ldap)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_GET_DN);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
pfree(filter);
ldap_msgfree(search_message);
@@ -2663,7 +2677,9 @@ CheckLDAPAuth(Port *port)
(errmsg("LDAP login failed for user \"%s\" on server \"%s\": %s",
fulluser, server_name, ldap_err2string(r)),
errdetail_for_ldap(ldap)));
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_AFTER_BIND);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
pfree(fulluser);
return STATUS_ERROR;
@@ -2672,7 +2688,9 @@ CheckLDAPAuth(Port *port)
/* Save the original bind DN as the authenticated identity. */
set_authn_id(port, fulluser);
+ pgstat_report_wait_start(WAIT_EVENT_LDAP_UNBIND_SUCCESS);
ldap_unbind(ldap);
+ pgstat_report_wait_end();
pfree(passwd);
pfree(fulluser);
diff --git a/src/backend/utils/activity/wait_event_names.txt b/src/backend/utils/activity/wait_event_names.txt
index a2852225614..f082756c294 100644
--- a/src/backend/utils/activity/wait_event_names.txt
+++ b/src/backend/utils/activity/wait_event_names.txt
@@ -177,6 +177,15 @@ LDAP_BIND_FOR_SEARCH "Waiting for an LDAP bind operation to search the directory
LDAP_HOST_LOOKUP "Waiting for DNS lookup of LDAP servers."
LDAP_SEARCH "Waiting for an LDAP search operation to complete."
LDAP_START_TLS "Waiting for an LDAP StartTLS exchange."
+LDAP_UNBIND_AFTER_BIND "Waiting for an LDAP connection to be unbound after a simple bind failed."
+LDAP_UNBIND_AFTER_BIND_FOR_SEARCH "Waiting for an LDAP connection to be unbound after a bind for search failed."
+LDAP_UNBIND_AFTER_COUNT_ENTRIES "Waiting for an LDAP connection to be unbound after an entry count failed."
+LDAP_UNBIND_AFTER_GET_DN "Waiting for an LDAP connection to be unbound after ldap_get_dn failed."
+LDAP_UNBIND_AFTER_NAME_CHECK "Waiting for an LDAP connection to be unbound after a name check failed."
+LDAP_UNBIND_AFTER_SEARCH "Waiting for an LDAP connection to be unbound after a bind+search failed."
+LDAP_UNBIND_AFTER_SET_OPTION "Waiting for an LDAP connection to be unbound after ldap_set_option failed."
+LDAP_UNBIND_AFTER_START_TLS "Waiting for an LDAP connection to be unbound after ldap_start_tls_s failed."
+LDAP_UNBIND_SUCCESS "Waiting for a successful LDAP connection to be unbound."
PAM_ACCT_MGMT "Waiting for the PAM service to validate the user account."
PAM_AUTHENTICATE "Waiting for the PAM service to authenticate the user."
RADIUS_RECVFROM "Waiting for a <function>recvfrom</function> call during a RADIUS transaction."
--
2.34.1
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-07 16:38 ` Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2025-03-07 16:38 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-06 15:39:44 -0800, Jacob Champion wrote:
> I've reattached the wait event patches, to get the cfbot back to where it was.
FWIW, I continue to think that this is a misuse of wait events. We shouldn't
use them as a poor man's general purpose tracing framework.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2025-03-07 17:03 ` Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-07 17:03 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 8:38 AM Andres Freund <[email protected]> wrote:
> FWIW, I continue to think that this is a misuse of wait events. We shouldn't
> use them as a poor man's general purpose tracing framework.
Well, okay. That's frustrating.
If I return to the original design, but replace all of the high-level
wait events with calls to pgstat_report_activity(), does that work?
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-07 17:25 ` Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2025-03-07 17:25 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-07 09:03:18 -0800, Jacob Champion wrote:
> On Fri, Mar 7, 2025 at 8:38 AM Andres Freund <[email protected]> wrote:
> > FWIW, I continue to think that this is a misuse of wait events. We shouldn't
> > use them as a poor man's general purpose tracing framework.
>
> Well, okay. That's frustrating.
I should have clarified - there are a few that I think are ok, basically the
places where we wrap syscalls, e.g. around the sendto, select and recvfrom in
PerformRadiusTransaction().
OTOH that code is effectively completely broken. Doing a blocking select() is
just a no-go, the code isn't interruptible, breaking authentication
timeout. And using select() means that we theoretically could crash due to an
fd that's above FD_SETSIZE.
Most of the other places I'm not on board with, that's wrapping large amounts
of code in a wait event, which pretty much means we're not waiting.
I think some of the wrapped calls into library code might actually call back
into our code (to receive/send data), and our code then will use wait events
around lower level operations done as part of that.
Which pretty much explains my main issue with this - either the code can't
wait in those function calls, in which case it's wrong to use wait events, or
the code is flat out broken.
It's also IMO quite wrong to do something that can throw an error inside a
wait event, because that means that the wait event will still be reported
during error recovery. Probably not the only place doing so, but it's still
wrong.
> If I return to the original design, but replace all of the high-level
> wait events with calls to pgstat_report_activity(), does that work?
It'd be less wrong.
But I really doubt that it's a good idea to encode all kinds of function calls
happening during authentication into something SQL visible. Why stop with
these functions and not just do that for *all* functions in postgres? I mean
it'd not work and slow everything down, but how do you define that line?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2025-03-07 18:28 ` Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-07 18:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 9:25 AM Andres Freund <[email protected]> wrote:
> I should have clarified - there are a few that I think are ok, basically the
> places where we wrap syscalls, e.g. around the sendto, select and recvfrom in
> PerformRadiusTransaction().
Okay.
> OTOH that code is effectively completely broken. Doing a blocking select() is
> just a no-go, the code isn't interruptible, breaking authentication
> timeout. And using select() means that we theoretically could crash due to an
> fd that's above FD_SETSIZE.
I think we're in agreement here; I'm just trying to improve things
incrementally. If someone actually hits the broken case, I think it'd
be helpful for them to see it.
> I think some of the wrapped calls into library code might actually call back
> into our code (to receive/send data), and our code then will use wait events
> around lower level operations done as part of that.
That would be a problem, agreed, but I didn't think I'd wrapped any
callback APIs. (Admittedly I have little experience with the SSPI
stuff.) But looking at the wrapped calls in the patch... which are you
suspicious of?
> It's also IMO quite wrong to do something that can throw an error inside a
> wait event, because that means that the wait event will still be reported
> during error recovery.
Hm, okay. I can change that for the LookupAccountSid case.
> Probably not the only place doing so, but it's still
> wrong.
It's definitely not the only place. :D
> Why stop with
> these functions and not just do that for *all* functions in postgres? I mean
> it'd not work and slow everything down,
(That seems like a good reason not to do it for all functions in
Postgres, no? I hope the slope is not all that slippery in practice.)
> but how do you define that line?
Cost/benefit. In this case, authentication hanging in an unknown place
in PAM and LDAP has caused tangible support problems. I suspect I'd
have gotten complaints if I only focused on those two places, though,
so I expanded it to the other blocking calls I could see.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-12 22:16 ` Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-12 22:16 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Fri, Mar 7, 2025 at 10:28 AM Jacob Champion
<[email protected]> wrote:
> > I think some of the wrapped calls into library code might actually call back
> > into our code (to receive/send data), and our code then will use wait events
> > around lower level operations done as part of that.
>
> That would be a problem, agreed, but I didn't think I'd wrapped any
> callback APIs. (Admittedly I have little experience with the SSPI
> stuff.) But looking at the wrapped calls in the patch... which are you
> suspicious of?
I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
being done there; it doesn't seem safe to potentially ereport(ERROR)
and longjmp through a PAM call stack? But I'll switch those over to
something safe or else drop that part of the patch.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-13 16:23 ` Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-13 16:23 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Andres Freund <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Wed, Mar 12, 2025 at 3:16 PM Jacob Champion
<[email protected]> wrote:
> I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
> being done there; it doesn't seem safe to potentially ereport(ERROR)
> and longjmp through a PAM call stack? But I'll switch those over to
> something safe or else drop that part of the patch.
PAM aside... Michael, what's your level of enthusiasm for the rest of
this patch? I was confidently, embarrassingly wrong about how
CheckPAMAuth worked, and it makes me think I need to put this down and
take a completely new crack at it in 19.
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-13 16:56 ` Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2025-03-13 16:56 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
Hi,
On 2025-03-13 09:23:10 -0700, Jacob Champion wrote:
> On Wed, Mar 12, 2025 at 3:16 PM Jacob Champion
> <[email protected]> wrote:
> > I missed PAM_CONV, sorry. I'm worried about the sendAuthRequest()
> > being done there; it doesn't seem safe to potentially ereport(ERROR)
> > and longjmp through a PAM call stack?
That indeed doesn't seem safe.
I am wondering if PAM is so fundamentally incompatible with handling
interrupts / a non-blocking interface that we have little choice but to
eventually remove it...
> PAM aside... Michael, what's your level of enthusiasm for the rest of this
> patch? I was confidently, embarrassingly wrong about how CheckPAMAuth
> worked, and it makes me think I need to put this down and take a completely
> new crack at it in 19.
FWIW, I continue to think that it's better to invest in making more auth
methods non-blocking, rather than adding wait events for code that could maybe
sometimes wait on different things internally.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2025-03-13 17:29 ` Jacob Champion <[email protected]>
2025-03-13 17:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-13 17:29 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 13, 2025 at 9:56 AM Andres Freund <[email protected]> wrote:
> I am wondering if PAM is so fundamentally incompatible with handling
> interrupts / a non-blocking interface that we have little choice but to
> eventually remove it...
Given the choice between a usually-working PAM module with known
architectural flaws, and not having PAM at all, I think many users
would rather continue using what's working for them.
> FWIW, I continue to think that it's better to invest in making more auth
> methods non-blocking, rather than adding wait events for code that could maybe
> sometimes wait on different things internally.
I think we disagree on the either/or nature of that. If I can get
proof that a certain thing is causing bugs in the wild, then I have
ammunition to fix that thing. Right now there is no visibility, and my
interest in rewriting old authentication methods without bug reports
to motivate that work is pretty low. I'm not willing to sign up for
that at the moment.
(But I do really appreciate the review. I'm just feeling crispy about
the overall result...)
Thanks,
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-13 17:56 ` Andres Freund <[email protected]>
2025-03-17 17:22 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Andres Freund @ 2025-03-13 17:56 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On 2025-03-13 10:29:49 -0700, Jacob Champion wrote:
> On Thu, Mar 13, 2025 at 9:56 AM Andres Freund <[email protected]> wrote:
> > I am wondering if PAM is so fundamentally incompatible with handling
> > interrupts / a non-blocking interface that we have little choice but to
> > eventually remove it...
>
> Given the choice between a usually-working PAM module with known
> architectural flaws, and not having PAM at all, I think many users
> would rather continue using what's working for them.
authentication_timeout currently doesn't reliably work while in some auth
methods, nor does pg_terminate_backend() etc. That's IMO is rather bad from a
DOSability perspective.
The fact that some auth methods are broken like that has had a sizable
negative impact on postgres for a long time. Not just when those methods are
used, but also architecturally.
It's e.g. one of the main reasons we need the ugly escalating logic in
postmaster shutdowns to send SIGQUITs and then SIGKILL after a while, because
we don't have a reliable way of terminating backends normally. This used to
be way worse because historically postgres considered it sane (why, I have no
idea) to ereport() in timeout functions, which then occasionally lead to
backends stuck in malloc locks etc.
> > FWIW, I continue to think that it's better to invest in making more auth
> > methods non-blocking, rather than adding wait events for code that could maybe
> > sometimes wait on different things internally.
>
> I think we disagree on the either/or nature of that. If I can get
> proof that a certain thing is causing bugs in the wild, then I have
> ammunition to fix that thing.
FWIW, I've have repeatedly seen production issues due to authentication
timeout not working for some auth methods.
It's not hard to see why - e.g. a non-resonsive radius server just leaves the
backend hanging in select(). Even though it would get interrupted by signals,
we'll just retry without even checking interrupts / timeouts :(.
> Right now there is no visibility, and my interest in rewriting old
> authentication methods without bug reports to motivate that work is pretty
> low. I'm not willing to sign up for that at the moment.
Fair enough.
> (But I do really appreciate the review. I'm just feeling crispy about
> the overall result...)
Also fair enough :)
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 17:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
@ 2025-03-17 17:22 ` Jacob Champion <[email protected]>
2025-03-21 05:53 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
0 siblings, 1 reply; 17+ messages in thread
From: Jacob Champion @ 2025-03-17 17:22 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Thu, Mar 13, 2025 at 10:56 AM Andres Freund <[email protected]> wrote:
> > Given the choice between a usually-working PAM module with known
> > architectural flaws, and not having PAM at all, I think many users
> > would rather continue using what's working for them.
>
> authentication_timeout currently doesn't reliably work while in some auth
> methods, nor does pg_terminate_backend() etc. That's IMO is rather bad from a
> DOSability perspective.
>
> The fact that some auth methods are broken like that has had a sizable
> negative impact on postgres for a long time. Not just when those methods are
> used, but also architecturally.
Right -- I just don't think end users are going to factor that into
their choice of authentication method. If IT tells you "use this PAM
module", then... that's it.
If we remove PAM, maybe they change authentication methods... or maybe
they just don't ever upgrade Postgres again. My money's on the latter.
--
I looked into switching over to pgstat_report_activity(), but that
wasn't designed to be called in the middle of backend initialization.
It would take more work to make those calls safe/sane when `st_state
== STATE_STARTING`. I plan to mark this patchset as Withdrawn for now.
Thanks all!
--Jacob
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 17:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-17 17:22 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
@ 2025-03-21 05:53 ` Michael Paquier <[email protected]>
0 siblings, 0 replies; 17+ messages in thread
From: Michael Paquier @ 2025-03-21 05:53 UTC (permalink / raw)
To: Jacob Champion <[email protected]>; +Cc: Andres Freund <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Andrew Dunstan <[email protected]>; pgsql-hackers; Euler Taveira <[email protected]>; Daniel Gustafsson <[email protected]>
On Mon, Mar 17, 2025 at 10:22:47AM -0700, Jacob Champion wrote:
> I looked into switching over to pgstat_report_activity(), but that
> wasn't designed to be called in the middle of backend initialization.
> It would take more work to make those calls safe/sane when `st_state
> == STATE_STARTING`. I plan to mark this patchset as Withdrawn for now.
Okay, fine by me. I had the impression that it would have been
possible to salvage some of the wait event states, but at least the
starting state showing up in pg_stat_activity will be able to provide
some information, so it's better than none. Unfortunately, I don't
have any room until the feature freeze for that.
Outside the stat report activity calls, I've been wondering if we
should add some dynamic tracking of which hba/ident entry a backend
PID is working with. For example, if we knew the file and the entry
line number, we would know on which auth method this backend is
bumping into. That maybe of course limited if someone modifies and
reloads the HBA file while a backend is stuck. Now, these files are
mostly static, and we have system views that provide the contents of
the ident and HBA files as SQL, so with a JOIN..
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2025-03-21 05:53 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-12-31 02:25 [PATCH 2/2] Refactor the fsync machinery to support future SMGR implementations. Thomas Munro <[email protected]>
2019-02-27 22:41 [PATCH v2] pg_wal on COW fs Alvaro Herrera <[email protected]>
2023-09-24 20:49 [PATCH v4 01/12] Change section heading to better reflect saving a result in variable(s) Karl O. Pinc <[email protected]>
2025-03-06 22:25 Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-06 23:15 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
2025-03-06 23:39 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 16:38 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 17:03 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-07 17:25 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-07 18:28 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-12 22:16 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:23 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 16:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-13 17:29 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-13 17:56 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Andres Freund <[email protected]>
2025-03-17 17:22 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Jacob Champion <[email protected]>
2025-03-21 05:53 ` Re: [PATCH] pg_stat_activity: make slow/hanging authentication more visible Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox