On Wed, Jan 25, 2023 at 8:57 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
> Thanks. I have some comments on
> v3-0002-Add-io_direct-setting-developer-only.patch:
>
> 1. I think we don't need to overwrite the io_direct_string in
> check_io_direct so that show_io_direct can be avoided.
Thanks for looking at this, and sorry for the late response. Yeah, agreed.
> 2. check_io_direct can leak the flags memory - when io_direct is not
> supported or for an invalid list syntax or an invalid option is
> specified.
>
> I have addressed my review comments as a delta patch on top of v3-0002
> and added it here as v1-0001-Review-comments-io_direct-GUC.txt.
Thanks. Your way is nicer. I merged your patch and added you as a co-author.
> Some comments on the tests added:
>
> 1. Is there a way to know if Direct IO for WAL and data has been
> picked up programmatically? IOW, can we know if the OS page cache is
> bypassed? I know an external extension pgfincore which can help here,
> but nothing in the core exists AFAICS.
Right, that extension can tell you how many pages are in the kernel
page cache which is quite interesting for this. I also once hacked up
something primitive to see *which* pages are in kernel cache, so I
could join that against pg_buffercache to measure double buffering,
when I was studying the "smile" shape where pgbench TPS goes down and
then back up again as you increase shared_buffers if the working set
is nearly as big as physical memory (code available in a link from
[1]).
Yeah, I agree it might be nice for human investigators to put
something like that in contrib/pg_buffercache, but I'm not sure you
could rely on it enough for an automated test, though, 'cause it
probably won't work on some file systems and the tests would probably
fail for random transient reasons (for example: some systems won't
kick pages out of kernel cache if they were already there, just
because we decided to open the file with O_DIRECT). (I got curious
about why mincore() wasn't standardised along with mmap() and all that
jazz; it seems the BSD and later Sun people who invented all those
interfaces didn't think that one was quite good enough[2], but every
(?) Unixoid OS copied it anyway, with variations... Apparently the
Windows thing is called VirtualQuery()).
> 2. Can we combine these two append_conf to a single statement?
> +$node->append_conf('io_direct', 'data,wal,wal_init');
> +$node->append_conf('shared_buffers', '64kB'); # tiny to force I/O
OK, sure, done. And also oops, that was completely wrong and not
working the way I had it in that version...
> 3. A nitpick: Can we split these queries multi-line instead of in a single
> line?
> +$node->safe_psql('postgres', 'begin; create temporary table t2 as
> select 1 as i from generate_series(1, 10000); update t2 set i = i;
> insert into t2count select count(*) from t2; commit;');
OK.
> 4. I don't think we need to stop the node before the test ends, no?
> +$node->stop;
> +
> +done_testing();
Sure, but why not?
Otherwise, I rebased, and made a couple more changes:
I found a line of the manual about wal_sync_method that needed to be removed:
- The <literal>open_</literal>* options also use
<literal>O_DIRECT</literal> if available.
In fact that sentence didn't correctly document the behaviour in
released branches (wal_level=minimal is also required for that, so
probably very few people ever used it). I think we should adjust that
misleading sentence in back-branches, separately from this patch set.
I also updated the commit message to highlight the only expected
user-visible change for this, namely the loss of the above incorrectly
documented obscure special case, which is replaced by the less obscure
new setting io_direct=wal, if someone still wants that behaviour.
Also a few minor comment changes.
[1] https://twitter.com/MengTangmu/status/994770040745615361
[2] http://kos.enix.org/pub/gingell8.pdf
From c6e01d506762fb7c11a3fb31d56902fa53ea822b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 13 Dec 2022 16:25:59 +1300
Subject: [PATCH v4 1/3] Introduce PG_IO_ALIGN_SIZE and align all I/O buffers.
In order to be able to use O_DIRECT/FILE_FLAG_NO_BUFFERING on common
systems in a later commit, we need the address and length of user space
buffers to align with the sector size of the storage. O_DIRECT would
either fail to work or fail to work efficiently without that on various
platforms. Even without O_DIRECT, aligning on memory pages is known to
improve traditional buffered I/O performance.
The alignment size is set to 4096, which is enough for currently known
systems: it covers traditional 512 byte sectors and modern 4096 byte
sectors, as well as common 4096 byte memory pages. There is no standard
governing the requirements for O_DIRECT so it's possible we might have
to reconsider this approach or fail to work on some exotic system, but
for now this simplistic approach works and it can be changed at compile
time.
Three classes of I/O buffers for regular data pages are adjusted:
(1) Heap buffers are allocated with the new palloc_aligned() or
MemoryContextAllocAligned() functions introduced by commit 439f6175.
(2) Stack buffers now use a new struct PGIOAlignedBlock to respect
PG_IO_ALIGN_SIZE, if possible with this compiler. (3) The main buffer
pool is also aligned in shared memory.
If arbitrary alignment of stack objects is not possible with this
compiler, then completely disable the use of O_DIRECT by setting
PG_O_DIRECT to 0. (This avoids the need to consider systems that have
O_DIRECT but don't have a compiler with an extension that can align
stack objects the way we want; that could be done but we don't currently
know of any such system, so it's easier to pretend there is no O_DIRECT
support instead: that's an existing and tested class of system.)
Add assertions that all buffers passed into smgrread(), smgrwrite(),
smgrextend() are correctly aligned, if PG_O_DIRECT isn't 0.
Author: Thomas Munro <thomas.mu...@gmail.com>
Author: Andres Freund <and...@anarazel.de>
Reviewed-by: Justin Pryzby <pry...@telsasoft.com>
Discussion: https://postgr.es/m/ca+hukgk1x532hyqj_mzfwt0n1zt8trz980d79wbjwnt-yyl...@mail.gmail.com
---
contrib/bloom/blinsert.c | 2 +-
contrib/pg_prewarm/pg_prewarm.c | 2 +-
src/backend/access/gist/gistbuild.c | 9 +++---
src/backend/access/hash/hashpage.c | 2 +-
src/backend/access/heap/rewriteheap.c | 2 +-
src/backend/access/nbtree/nbtree.c | 2 +-
src/backend/access/nbtree/nbtsort.c | 8 ++++--
src/backend/access/spgist/spginsert.c | 2 +-
src/backend/access/transam/generic_xlog.c | 13 ++++++---
src/backend/access/transam/xlog.c | 9 +++---
src/backend/catalog/storage.c | 2 +-
src/backend/storage/buffer/buf_init.c | 10 +++++--
src/backend/storage/buffer/bufmgr.c | 2 +-
src/backend/storage/buffer/localbuf.c | 7 +++--
src/backend/storage/file/buffile.c | 6 ++++
src/backend/storage/page/bufpage.c | 5 +++-
src/backend/storage/smgr/md.c | 15 +++++++++-
src/backend/utils/sort/logtape.c | 2 +-
src/bin/pg_checksums/pg_checksums.c | 2 +-
src/bin/pg_rewind/local_source.c | 4 +--
src/bin/pg_upgrade/file.c | 4 +--
src/common/file_utils.c | 4 +--
src/include/c.h | 34 +++++++++++++++++------
src/include/pg_config_manual.h | 6 ++++
src/include/storage/fd.h | 5 ++--
src/tools/pgindent/typedefs.list | 1 +
26 files changed, 112 insertions(+), 48 deletions(-)
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index dcd8120895..b42b9e6c41 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -166,7 +166,7 @@ blbuildempty(Relation index)
Page metapage;
/* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
+ metapage = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
BloomFillMetapage(index, metapage);
/*
diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 54209924ae..e464d0d4d2 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -36,7 +36,7 @@ typedef enum
PREWARM_BUFFER
} PrewarmType;
-static PGAlignedBlock blockbuffer;
+static PGIOAlignedBlock blockbuffer;
/*
* pg_prewarm(regclass, mode text, fork text,
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index d2f8da5b02..5e0c1447f9 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -415,7 +415,7 @@ gist_indexsortbuild(GISTBuildState *state)
* Write an empty page as a placeholder for the root page. It will be
* replaced with the real root page at the end.
*/
- page = palloc0(BLCKSZ);
+ page = palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, MCXT_ALLOC_ZERO);
smgrextend(RelationGetSmgr(state->indexrel), MAIN_FORKNUM, GIST_ROOT_BLKNO,
page, true);
state->pages_allocated++;
@@ -509,7 +509,8 @@ gist_indexsortbuild_levelstate_add(GISTBuildState *state,
levelstate->current_page++;
if (levelstate->pages[levelstate->current_page] == NULL)
- levelstate->pages[levelstate->current_page] = palloc(BLCKSZ);
+ levelstate->pages[levelstate->current_page] =
+ palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
newPage = levelstate->pages[levelstate->current_page];
gistinitpage(newPage, old_page_flags);
@@ -579,7 +580,7 @@ gist_indexsortbuild_levelstate_flush(GISTBuildState *state,
/* Create page and copy data */
data = (char *) (dist->list);
- target = palloc0(BLCKSZ);
+ target = palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, MCXT_ALLOC_ZERO);
gistinitpage(target, isleaf ? F_LEAF : 0);
for (int i = 0; i < dist->block.num; i++)
{
@@ -630,7 +631,7 @@ gist_indexsortbuild_levelstate_flush(GISTBuildState *state,
if (parent == NULL)
{
parent = palloc0(sizeof(GistSortedBuildLevelState));
- parent->pages[0] = (Page) palloc(BLCKSZ);
+ parent->pages[0] = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
parent->parent = NULL;
gistinitpage(parent->pages[0], 0);
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 6d8af42260..af3a154266 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -992,7 +992,7 @@ static bool
_hash_alloc_buckets(Relation rel, BlockNumber firstblock, uint32 nblocks)
{
BlockNumber lastblock;
- PGAlignedBlock zerobuf;
+ PGIOAlignedBlock zerobuf;
Page page;
HashPageOpaque ovflopaque;
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index ae0282a70e..424958912c 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -255,7 +255,7 @@ begin_heap_rewrite(Relation old_heap, Relation new_heap, TransactionId oldest_xm
state->rs_old_rel = old_heap;
state->rs_new_rel = new_heap;
- state->rs_buffer = (Page) palloc(BLCKSZ);
+ state->rs_buffer = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
/* new_heap needn't be empty, just locked */
state->rs_blockno = RelationGetNumberOfBlocks(new_heap);
state->rs_buffer_valid = false;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 992f84834f..2df8849858 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -154,7 +154,7 @@ btbuildempty(Relation index)
Page metapage;
/* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
+ metapage = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
_bt_initmetapage(metapage, P_NONE, 0, _bt_allequalimage(index, false));
/*
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 1207a49689..6ad3f3c54d 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -619,7 +619,7 @@ _bt_blnewpage(uint32 level)
Page page;
BTPageOpaque opaque;
- page = (Page) palloc(BLCKSZ);
+ page = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
/* Zero the page and set up standard page header info */
_bt_pageinit(page, BLCKSZ);
@@ -660,7 +660,9 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
while (blkno > wstate->btws_pages_written)
{
if (!wstate->btws_zeropage)
- wstate->btws_zeropage = (Page) palloc0(BLCKSZ);
+ wstate->btws_zeropage = (Page) palloc_aligned(BLCKSZ,
+ PG_IO_ALIGN_SIZE,
+ MCXT_ALLOC_ZERO);
/* don't set checksum for all-zero page */
smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM,
wstate->btws_pages_written++,
@@ -1170,7 +1172,7 @@ _bt_uppershutdown(BTWriteState *wstate, BTPageState *state)
* set to point to "P_NONE"). This changes the index to the "valid" state
* by filling in a valid magic number in the metapage.
*/
- metapage = (Page) palloc(BLCKSZ);
+ metapage = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
_bt_initmetapage(metapage, rootblkno, rootlevel,
wstate->inskey->allequalimage);
_bt_blwritepage(wstate, metapage, BTREE_METAPAGE);
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index 718a88335d..72d2e1551c 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -158,7 +158,7 @@ spgbuildempty(Relation index)
Page page;
/* Construct metapage. */
- page = (Page) palloc(BLCKSZ);
+ page = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
SpGistInitMetapage(page);
/*
diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index 9f67d1c1cd..6c68191ca6 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -58,14 +58,17 @@ typedef struct
char delta[MAX_DELTA_SIZE]; /* delta between page images */
} PageData;
-/* State of generic xlog record construction */
+/*
+ * State of generic xlog record construction. Must be allocated at an I/O
+ * aligned address.
+ */
struct GenericXLogState
{
+ /* Page images (properly aligned, must be first) */
+ PGIOAlignedBlock images[MAX_GENERIC_XLOG_PAGES];
/* Info about each page, see above */
PageData pages[MAX_GENERIC_XLOG_PAGES];
bool isLogged;
- /* Page images (properly aligned) */
- PGAlignedBlock images[MAX_GENERIC_XLOG_PAGES];
};
static void writeFragment(PageData *pageData, OffsetNumber offset,
@@ -269,7 +272,9 @@ GenericXLogStart(Relation relation)
GenericXLogState *state;
int i;
- state = (GenericXLogState *) palloc(sizeof(GenericXLogState));
+ state = (GenericXLogState *) palloc_aligned(sizeof(GenericXLogState),
+ PG_IO_ALIGN_SIZE,
+ 0);
state->isLogged = RelationNeedsWAL(relation);
for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 46821ad605..3fea8c4082 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -4506,7 +4506,7 @@ XLOGShmemSize(void)
/* xlblocks array */
size = add_size(size, mul_size(sizeof(XLogRecPtr), XLOGbuffers));
/* extra alignment padding for XLOG I/O buffers */
- size = add_size(size, XLOG_BLCKSZ);
+ size = add_size(size, Max(XLOG_BLCKSZ, PG_IO_ALIGN_SIZE));
/* and the buffers themselves */
size = add_size(size, mul_size(XLOG_BLCKSZ, XLOGbuffers));
@@ -4603,10 +4603,11 @@ XLOGShmemInit(void)
/*
* Align the start of the page buffers to a full xlog block size boundary.
- * This simplifies some calculations in XLOG insertion. It is also
- * required for O_DIRECT.
+ * This simplifies some calculations in XLOG insertion. We also need I/O
+ * alignment for O_DIRECT, but that's also a power of two and usually
+ * smaller. Take the larger of the two alignment requirements.
*/
- allocptr = (char *) TYPEALIGN(XLOG_BLCKSZ, allocptr);
+ allocptr = (char *) TYPEALIGN(Max(XLOG_BLCKSZ, PG_IO_ALIGN_SIZE), allocptr);
XLogCtl->pages = allocptr;
memset(XLogCtl->pages, 0, (Size) XLOG_BLCKSZ * XLOGbuffers);
diff --git a/src/backend/catalog/storage.c b/src/backend/catalog/storage.c
index af1491aa1d..2add053489 100644
--- a/src/backend/catalog/storage.c
+++ b/src/backend/catalog/storage.c
@@ -451,7 +451,7 @@ void
RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
ForkNumber forkNum, char relpersistence)
{
- PGAlignedBlock buf;
+ PGIOAlignedBlock buf;
Page page;
bool use_wal;
bool copying_initfork;
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 20946c47cb..0057443f0c 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -78,9 +78,12 @@ InitBufferPool(void)
NBuffers * sizeof(BufferDescPadded),
&foundDescs);
+ /* Align buffer pool on IO page size boundary. */
BufferBlocks = (char *)
- ShmemInitStruct("Buffer Blocks",
- NBuffers * (Size) BLCKSZ, &foundBufs);
+ TYPEALIGN(PG_IO_ALIGN_SIZE,
+ ShmemInitStruct("Buffer Blocks",
+ NBuffers * (Size) BLCKSZ + PG_IO_ALIGN_SIZE,
+ &foundBufs));
/* Align condition variables to cacheline boundary. */
BufferIOCVArray = (ConditionVariableMinimallyPadded *)
@@ -163,7 +166,8 @@ BufferShmemSize(void)
/* to allow aligning buffer descriptors */
size = add_size(size, PG_CACHE_LINE_SIZE);
- /* size of data pages */
+ /* size of data pages, plus alignment padding */
+ size = add_size(size, PG_IO_ALIGN_SIZE);
size = add_size(size, mul_size(NBuffers, BLCKSZ));
/* size of stuff controlled by freelist.c */
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 908a8934bd..033f230b1d 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4261,7 +4261,7 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
bool use_wal;
BlockNumber nblocks;
BlockNumber blkno;
- PGAlignedBlock buf;
+ PGIOAlignedBlock buf;
BufferAccessStrategy bstrategy_src;
BufferAccessStrategy bstrategy_dst;
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index 3846d3eaca..aae02949ce 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -735,8 +735,11 @@ GetLocalBufferStorage(void)
/* And don't overflow MaxAllocSize, either */
num_bufs = Min(num_bufs, MaxAllocSize / BLCKSZ);
- cur_block = (char *) MemoryContextAlloc(LocalBufferContext,
- num_bufs * BLCKSZ);
+ /* Buffers should be I/O aligned. */
+ cur_block = (char *)
+ TYPEALIGN(PG_IO_ALIGN_SIZE,
+ MemoryContextAlloc(LocalBufferContext,
+ num_bufs * BLCKSZ + PG_IO_ALIGN_SIZE));
next_buf_in_block = 0;
num_bufs_in_block = num_bufs;
}
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 37ea8ac6b7..84ead85942 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -95,6 +95,12 @@ struct BufFile
off_t curOffset; /* offset part of current pos */
int pos; /* next read/write position in buffer */
int nbytes; /* total # of valid bytes in buffer */
+
+ /*
+ * XXX Should ideally us PGIOAlignedBlock, but might need a way to avoid
+ * wasting per-file alignment padding when some users create many
+ * files.
+ */
PGAlignedBlock buffer;
};
diff --git a/src/backend/storage/page/bufpage.c b/src/backend/storage/page/bufpage.c
index 92994f8f39..9a302ddc30 100644
--- a/src/backend/storage/page/bufpage.c
+++ b/src/backend/storage/page/bufpage.c
@@ -1522,7 +1522,10 @@ PageSetChecksumCopy(Page page, BlockNumber blkno)
* and second to avoid wasting space in processes that never call this.
*/
if (pageCopy == NULL)
- pageCopy = MemoryContextAlloc(TopMemoryContext, BLCKSZ);
+ pageCopy = MemoryContextAllocAligned(TopMemoryContext,
+ BLCKSZ,
+ PG_IO_ALIGN_SIZE,
+ 0);
memcpy(pageCopy, (char *) page, BLCKSZ);
((PageHeader) pageCopy)->pd_checksum = pg_checksum_page(pageCopy, blkno);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 1c2d1405f8..efa9773a4d 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -453,6 +453,10 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
int nbytes;
MdfdVec *v;
+ /* If this build supports direct I/O, the buffer must be I/O aligned. */
+ if (PG_O_DIRECT != 0)
+ Assert((uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer));
+
/* This assert is too expensive to have on normally ... */
#ifdef CHECK_WRITE_VS_EXTEND
Assert(blocknum >= mdnblocks(reln, forknum));
@@ -783,6 +787,10 @@ mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
int nbytes;
MdfdVec *v;
+ /* If this build supports direct I/O, the buffer must be I/O aligned. */
+ if (PG_O_DIRECT != 0)
+ Assert((uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer));
+
TRACE_POSTGRESQL_SMGR_MD_READ_START(forknum, blocknum,
reln->smgr_rlocator.locator.spcOid,
reln->smgr_rlocator.locator.dbOid,
@@ -848,6 +856,10 @@ mdwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
int nbytes;
MdfdVec *v;
+ /* If this build supports direct I/O, the buffer must be I/O aligned. */
+ if (PG_O_DIRECT != 0)
+ Assert((uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer));
+
/* This assert is too expensive to have on normally ... */
#ifdef CHECK_WRITE_VS_EXTEND
Assert(blocknum < mdnblocks(reln, forknum));
@@ -1424,7 +1436,8 @@ _mdfd_getseg(SMgrRelation reln, ForkNumber forknum, BlockNumber blkno,
*/
if (nblocks < ((BlockNumber) RELSEG_SIZE))
{
- char *zerobuf = palloc0(BLCKSZ);
+ char *zerobuf = palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE,
+ MCXT_ALLOC_ZERO);
mdextend(reln, forknum,
nextsegno * ((BlockNumber) RELSEG_SIZE) - 1,
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 64ea237438..52b8898d5e 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -252,7 +252,7 @@ ltsWriteBlock(LogicalTapeSet *lts, long blocknum, const void *buffer)
*/
while (blocknum > lts->nBlocksWritten)
{
- PGAlignedBlock zerobuf;
+ PGIOAlignedBlock zerobuf;
MemSet(zerobuf.data, 0, sizeof(zerobuf));
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index aa21007497..19eb67e485 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -183,7 +183,7 @@ skipfile(const char *fn)
static void
scan_file(const char *fn, int segmentno)
{
- PGAlignedBlock buf;
+ PGIOAlignedBlock buf;
PageHeader header = (PageHeader) buf.data;
int f;
BlockNumber blockno;
diff --git a/src/bin/pg_rewind/local_source.c b/src/bin/pg_rewind/local_source.c
index da9d75dccb..4e2a1376c6 100644
--- a/src/bin/pg_rewind/local_source.c
+++ b/src/bin/pg_rewind/local_source.c
@@ -77,7 +77,7 @@ static void
local_queue_fetch_file(rewind_source *source, const char *path, size_t len)
{
const char *datadir = ((local_source *) source)->datadir;
- PGAlignedBlock buf;
+ PGIOAlignedBlock buf;
char srcpath[MAXPGPATH];
int srcfd;
size_t written_len;
@@ -129,7 +129,7 @@ local_queue_fetch_range(rewind_source *source, const char *path, off_t off,
size_t len)
{
const char *datadir = ((local_source *) source)->datadir;
- PGAlignedBlock buf;
+ PGIOAlignedBlock buf;
char srcpath[MAXPGPATH];
int srcfd;
off_t begin = off;
diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c
index ed874507ff..d173602882 100644
--- a/src/bin/pg_upgrade/file.c
+++ b/src/bin/pg_upgrade/file.c
@@ -178,8 +178,8 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile,
{
int src_fd;
int dst_fd;
- PGAlignedBlock buffer;
- PGAlignedBlock new_vmbuf;
+ PGIOAlignedBlock buffer;
+ PGIOAlignedBlock new_vmbuf;
ssize_t totalBytesRead = 0;
ssize_t src_filesize;
int rewriteVmBytesPerPage;
diff --git a/src/common/file_utils.c b/src/common/file_utils.c
index d568d83b9f..74833c4acb 100644
--- a/src/common/file_utils.c
+++ b/src/common/file_utils.c
@@ -540,8 +540,8 @@ pg_pwritev_with_retry(int fd, const struct iovec *iov, int iovcnt, off_t offset)
ssize_t
pg_pwrite_zeros(int fd, size_t size, off_t offset)
{
- static const PGAlignedBlock zbuffer = {{0}}; /* worth BLCKSZ */
- void *zerobuf_addr = unconstify(PGAlignedBlock *, &zbuffer)->data;
+ static const PGIOAlignedBlock zbuffer = {{0}}; /* worth BLCKSZ */
+ void *zerobuf_addr = unconstify(PGIOAlignedBlock *, &zbuffer)->data;
struct iovec iov[PG_IOV_MAX];
size_t remaining_size = size;
ssize_t total_written = 0;
diff --git a/src/include/c.h b/src/include/c.h
index 5fe7a97ff0..f69d739be5 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1119,14 +1119,11 @@ extern void ExceptionalCondition(const char *conditionName,
/*
* Use this, not "char buf[BLCKSZ]", to declare a field or local variable
- * holding a page buffer, if that page might be accessed as a page and not
- * just a string of bytes. Otherwise the variable might be under-aligned,
- * causing problems on alignment-picky hardware. (In some places, we use
- * this to declare buffers even though we only pass them to read() and
- * write(), because copying to/from aligned buffers is usually faster than
- * using unaligned buffers.) We include both "double" and "int64" in the
- * union to ensure that the compiler knows the value must be MAXALIGN'ed
- * (cf. configure's computation of MAXIMUM_ALIGNOF).
+ * holding a page buffer, if that page might be accessed as a page. Otherwise
+ * the variable might be under-aligned, causing problems on alignment-picky
+ * hardware. We include both "double" and "int64" in the union to ensure that
+ * the compiler knows the value must be MAXALIGN'ed (cf. configure's
+ * computation of MAXIMUM_ALIGNOF).
*/
typedef union PGAlignedBlock
{
@@ -1135,9 +1132,30 @@ typedef union PGAlignedBlock
int64 force_align_i64;
} PGAlignedBlock;
+/*
+ * Use this to declare a field or local variable holding a page buffer, if that
+ * page might be accessed as a page or passed to an SMgr I/O function. If
+ * allocating using the MemoryContext API, the aligned allocation functions
+ * should be used with PG_IO_ALIGN_SIZE. This alignment may be more efficient
+ * for I/O in general, but may be strictly required on some platforms when
+ * using direct I/O.
+ */
+typedef union PGIOAlignedBlock
+{
+#ifdef pg_attribute_aligned
+ pg_attribute_aligned(PG_IO_ALIGN_SIZE)
+#endif
+ char data[BLCKSZ];
+ double force_align_d;
+ int64 force_align_i64;
+} PGIOAlignedBlock;
+
/* Same, but for an XLOG_BLCKSZ-sized buffer */
typedef union PGAlignedXLogBlock
{
+#ifdef pg_attribute_aligned
+ pg_attribute_aligned(PG_IO_ALIGN_SIZE)
+#endif
char data[XLOG_BLCKSZ];
double force_align_d;
int64 force_align_i64;
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index b586ee269a..c799bc2013 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -227,6 +227,12 @@
*/
#define PG_CACHE_LINE_SIZE 128
+/*
+ * Assumed alignment requirement for direct I/O. 4K corresponds to sector size
+ * on modern storage, and works also for older 512 byte sectors.
+ */
+#define PG_IO_ALIGN_SIZE 4096
+
/*
*------------------------------------------------------------------------
* The following symbols are for enabling debugging code, not for
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index daceafd473..faac4914fe 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -82,9 +82,10 @@ extern PGDLLIMPORT int max_safe_fds;
* to the appropriate Windows flag in src/port/open.c. We simulate it with
* fcntl(F_NOCACHE) on macOS inside fd.c's open() wrapper. We use the name
* PG_O_DIRECT rather than defining O_DIRECT in that case (probably not a good
- * idea on a Unix).
+ * idea on a Unix). We can only use it if the compiler will correctly align
+ * PGIOAlignedBlock for us, though.
*/
-#if defined(O_DIRECT)
+#if defined(O_DIRECT) && defined(pg_attribute_aligned)
#define PG_O_DIRECT O_DIRECT
#elif defined(F_NOCACHE)
#define PG_O_DIRECT 0x80000000
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3219ea5f05..0313b2c93a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1703,6 +1703,7 @@ PGEventResultDestroy
PGFInfoFunction
PGFileType
PGFunction
+PGIOAlignedBlock
PGLZ_HistEntry
PGLZ_Strategy
PGLoadBalanceType
--
2.39.2
From 3db83b7289b85d1c84c5490e1d43e378b5ed3053 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 13 Dec 2022 16:54:18 +1300
Subject: [PATCH v4 2/3] Add io_direct setting (developer-only).
Provide a way to ask the kernel to use O_DIRECT (or local equivalent)
for data and WAL files. This hurts performance currently and is not
intended for end-users yet. Later proposed work would introduce our own
I/O clustering, read-ahead, etc to replace the kernel features that are
disabled with this option.
The only user-visible change, if the developer-only GUC is not used, is
that this commit also removes the obscure logic that would activate
O_DIRECT for the WAL when wal_sync_method=open_[data]sync and
wal_level=minimal (which also requires max_wal_senders=0). Those are
non-default and unlikely settings, and this behavior wasn't (correctly)
documented. In the unlikely event that a user wants that functionality
back, io_direct=wal is a more direct way to say so.
Author: Thomas Munro <thomas.mu...@gmail.com>
Author: Andres Freund <and...@anarazel.de>
Author: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com>
Reviewed-by: Justin Pryzby <pry...@telsasoft.com>
Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com>
Discussion: https://postgr.es/m/CA%2BhUKGK1X532hYqJ_MzFWt0n1zt8trz980D79WbjwnT-yYLZpg%40mail.gmail.com
---
doc/src/sgml/config.sgml | 34 ++++++++-
src/backend/access/transam/xlog.c | 37 ++++-----
src/backend/access/transam/xlogprefetcher.c | 2 +-
src/backend/storage/buffer/bufmgr.c | 16 ++--
src/backend/storage/buffer/localbuf.c | 7 +-
src/backend/storage/file/fd.c | 76 +++++++++++++++++++
src/backend/storage/smgr/md.c | 24 ++++--
src/backend/storage/smgr/smgr.c | 1 +
src/backend/utils/misc/guc_tables.c | 12 +++
src/include/storage/fd.h | 7 ++
src/include/storage/smgr.h | 1 +
src/include/utils/guc_hooks.h | 2 +
src/test/modules/test_misc/meson.build | 1 +
src/test/modules/test_misc/t/004_io_direct.pl | 48 ++++++++++++
14 files changed, 233 insertions(+), 35 deletions(-)
create mode 100644 src/test/modules/test_misc/t/004_io_direct.pl
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 25111d5caf..fc885c43a8 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3155,7 +3155,6 @@ include_dir 'conf.d'
</listitem>
</itemizedlist>
<para>
- The <literal>open_</literal>* options also use <literal>O_DIRECT</literal> if available.
Not all of these choices are available on all platforms.
The default is the first method in the above list that is supported
by the platform, except that <literal>fdatasync</literal> is the default on
@@ -11241,6 +11240,39 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
</listitem>
</varlistentry>
+ <varlistentry id="guc-io-direct" xreflabel="io_direct">
+ <term><varname>io_direct</varname> (<type>string</type>)
+ <indexterm>
+ <primary><varname>io_direct</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Ask the kernel to minimize caching effects for relation data and WAL
+ files using <literal>O_DIRECT</literal> (most Unix-like systems),
+ <literal>F_NOCACHE</literal> (macOS) or
+ <literal>FILE_FLAG_NO_BUFFERING</literal> (Windows).
+ </para>
+ <para>
+ May be set to an empty string (the default) to disable use of direct
+ I/O, or a comma-separated list of types of files for which direct I/O
+ is enabled. The valid types of file are <literal>data</literal> for
+ main data files, <literal>wal</literal> for WAL files, and
+ <literal>wal_init</literal> for WAL files when being initially
+ allocated.
+ </para>
+ <para>
+ Some operating systems and file systems do not support direct I/O, so
+ non-default settings may be rejected at startup, or produce I/O errors
+ at runtime.
+ </para>
+ <para>
+ Currently this feature reduces performance, and is intended for
+ developer testing only.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-post-auth-delay" xreflabel="post_auth_delay">
<term><varname>post_auth_delay</varname> (<type>integer</type>)
<indexterm>
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 3fea8c4082..7a555d8701 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2926,6 +2926,7 @@ XLogFileInitInternal(XLogSegNo logsegno, TimeLineID logtli,
XLogSegNo max_segno;
int fd;
int save_errno;
+ int open_flags = O_RDWR | O_CREAT | O_EXCL | PG_BINARY;
Assert(logtli != 0);
@@ -2959,8 +2960,11 @@ XLogFileInitInternal(XLogSegNo logsegno, TimeLineID logtli,
unlink(tmppath);
+ if (io_direct_flags & IO_DIRECT_WAL_INIT)
+ open_flags |= PG_O_DIRECT;
+
/* do not use get_sync_bit() here --- want to fsync only at end of fill */
- fd = BasicOpenFile(tmppath, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
+ fd = BasicOpenFile(tmppath, open_flags);
if (fd < 0)
ereport(ERROR,
(errcode_for_file_access(),
@@ -3354,7 +3358,7 @@ XLogFileClose(void)
* use the cache to read the WAL segment.
*/
#if defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_DONTNEED)
- if (!XLogIsNeeded())
+ if (!XLogIsNeeded() && (io_direct_flags & IO_DIRECT_WAL) == 0)
(void) posix_fadvise(openLogFile, 0, 0, POSIX_FADV_DONTNEED);
#endif
@@ -4445,7 +4449,6 @@ show_in_hot_standby(void)
return RecoveryInProgress() ? "on" : "off";
}
-
/*
* Read the control file, set respective GUCs.
*
@@ -8030,35 +8033,27 @@ xlog_redo(XLogReaderState *record)
}
/*
- * Return the (possible) sync flag used for opening a file, depending on the
- * value of the GUC wal_sync_method.
+ * Return the extra open flags used for opening a file, depending on the
+ * value of the GUCs wal_sync_method, fsync and io_direct.
*/
static int
get_sync_bit(int method)
{
int o_direct_flag = 0;
- /* If fsync is disabled, never open in sync mode */
- if (!enableFsync)
- return 0;
-
/*
- * Optimize writes by bypassing kernel cache with O_DIRECT when using
- * O_SYNC and O_DSYNC. But only if archiving and streaming are disabled,
- * otherwise the archive command or walsender process will read the WAL
- * soon after writing it, which is guaranteed to cause a physical read if
- * we bypassed the kernel cache. We also skip the
- * posix_fadvise(POSIX_FADV_DONTNEED) call in XLogFileClose() for the same
- * reason.
- *
- * Never use O_DIRECT in walreceiver process for similar reasons; the WAL
+ * Use O_DIRECT if requested, except in walreceiver process. The WAL
* written by walreceiver is normally read by the startup process soon
- * after it's written. Also, walreceiver performs unaligned writes, which
+ * after it's written. Also, walreceiver performs unaligned writes, which
* don't work with O_DIRECT, so it is required for correctness too.
*/
- if (!XLogIsNeeded() && !AmWalReceiverProcess())
+ if ((io_direct_flags & IO_DIRECT_WAL) && !AmWalReceiverProcess())
o_direct_flag = PG_O_DIRECT;
+ /* If fsync is disabled, never open in sync mode */
+ if (!enableFsync)
+ return o_direct_flag;
+
switch (method)
{
/*
@@ -8070,7 +8065,7 @@ get_sync_bit(int method)
case SYNC_METHOD_FSYNC:
case SYNC_METHOD_FSYNC_WRITETHROUGH:
case SYNC_METHOD_FDATASYNC:
- return 0;
+ return o_direct_flag;
#ifdef O_SYNC
case SYNC_METHOD_OPEN:
return O_SYNC | o_direct_flag;
diff --git a/src/backend/access/transam/xlogprefetcher.c b/src/backend/access/transam/xlogprefetcher.c
index 046e40d143..7ba18f2a76 100644
--- a/src/backend/access/transam/xlogprefetcher.c
+++ b/src/backend/access/transam/xlogprefetcher.c
@@ -785,7 +785,7 @@ XLogPrefetcherNextBlock(uintptr_t pgsr_private, XLogRecPtr *lsn)
block->prefetch_buffer = InvalidBuffer;
return LRQ_NEXT_IO;
}
- else
+ else if ((io_direct_flags & IO_DIRECT_DATA) == 0)
{
/*
* This shouldn't be possible, because we already determined
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 033f230b1d..5cb026d1ca 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -541,8 +541,11 @@ PrefetchSharedBuffer(SMgrRelation smgr_reln,
* Try to initiate an asynchronous read. This returns false in
* recovery if the relation file doesn't exist.
*/
- if (smgrprefetch(smgr_reln, forkNum, blockNum))
+ if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+ smgrprefetch(smgr_reln, forkNum, blockNum))
+ {
result.initiated_io = true;
+ }
#endif /* USE_PREFETCH */
}
else
@@ -588,11 +591,11 @@ PrefetchSharedBuffer(SMgrRelation smgr_reln,
* the kernel and therefore didn't really initiate I/O, and no way to know when
* the I/O completes other than using synchronous ReadBuffer().
*
- * 3. Otherwise, the buffer wasn't already cached by PostgreSQL, and either
+ * 3. Otherwise, the buffer wasn't already cached by PostgreSQL, and
* USE_PREFETCH is not defined (this build doesn't support prefetching due to
- * lack of a kernel facility), or the underlying relation file wasn't found and
- * we are in recovery. (If the relation file wasn't found and we are not in
- * recovery, an error is raised).
+ * lack of a kernel facility), direct I/O is enabled, or the underlying
+ * relation file wasn't found and we are in recovery. (If the relation file
+ * wasn't found and we are not in recovery, an error is raised).
*/
PrefetchBufferResult
PrefetchBuffer(Relation reln, ForkNumber forkNum, BlockNumber blockNum)
@@ -5451,6 +5454,9 @@ ScheduleBufferTagForWriteback(WritebackContext *context, BufferTag *tag)
{
PendingWriteback *pending;
+ if (io_direct_flags & IO_DIRECT_DATA)
+ return;
+
/*
* Add buffer to the pending writeback array, unless writeback control is
* disabled.
diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c
index aae02949ce..c6384c9fde 100644
--- a/src/backend/storage/buffer/localbuf.c
+++ b/src/backend/storage/buffer/localbuf.c
@@ -92,8 +92,11 @@ PrefetchLocalBuffer(SMgrRelation smgr, ForkNumber forkNum,
{
#ifdef USE_PREFETCH
/* Not in buffers, so initiate prefetch */
- smgrprefetch(smgr, forkNum, blockNum);
- result.initiated_io = true;
+ if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+ smgrprefetch(smgr, forkNum, blockNum))
+ {
+ result.initiated_io = true;
+ }
#endif /* USE_PREFETCH */
}
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index a280a1e7be..ccc789dc03 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -98,7 +98,9 @@
#include "storage/fd.h"
#include "storage/ipc.h"
#include "utils/guc.h"
+#include "utils/guc_hooks.h"
#include "utils/resowner_private.h"
+#include "utils/varlena.h"
/* Define PG_FLUSH_DATA_WORKS if we have an implementation for pg_flush_data */
#if defined(HAVE_SYNC_FILE_RANGE)
@@ -162,6 +164,9 @@ bool data_sync_retry = false;
/* How SyncDataDirectory() should do its job. */
int recovery_init_sync_method = RECOVERY_INIT_SYNC_METHOD_FSYNC;
+/* Which kinds of files should be opened with PG_O_DIRECT. */
+int io_direct_flags;
+
/* Debugging.... */
#ifdef FDDEBUG
@@ -2022,6 +2027,9 @@ FileWriteback(File file, off_t offset, off_t nbytes, uint32 wait_event_info)
if (nbytes <= 0)
return;
+ if (VfdCache[file].fileFlags & PG_O_DIRECT)
+ return;
+
returnCode = FileAccess(file);
if (returnCode < 0)
return;
@@ -3826,3 +3834,71 @@ data_sync_elevel(int elevel)
{
return data_sync_retry ? elevel : PANIC;
}
+
+bool
+check_io_direct(char **newval, void **extra, GucSource source)
+{
+ int flags;
+
+#if PG_O_DIRECT == 0
+ if (strcmp(*newval, "") != 0)
+ {
+ GUC_check_errdetail("io_direct is not supported on this platform.");
+ return false;
+ }
+ flags = 0;
+#else
+ List *elemlist;
+ ListCell *l;
+ char *rawstring;
+
+ /* Need a modifiable copy of string */
+ rawstring = pstrdup(*newval);
+
+ if (!SplitGUCList(rawstring, ',', &elemlist))
+ {
+ GUC_check_errdetail("invalid list syntax in parameter \"%s\"",
+ "io_direct");
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+
+ flags = 0;
+ foreach (l, elemlist)
+ {
+ char *item = (char *) lfirst(l);
+
+ if (pg_strcasecmp(item, "data") == 0)
+ flags |= IO_DIRECT_DATA;
+ else if (pg_strcasecmp(item, "wal") == 0)
+ flags |= IO_DIRECT_WAL;
+ else if (pg_strcasecmp(item, "wal_init") == 0)
+ flags |= IO_DIRECT_WAL_INIT;
+ else
+ {
+ GUC_check_errdetail("invalid option \"%s\"", item);
+ pfree(rawstring);
+ list_free(elemlist);
+ return false;
+ }
+ }
+
+ pfree(rawstring);
+ list_free(elemlist);
+#endif
+
+ /* Save the flags in *extra, for use by assign_io_direct */
+ *extra = guc_malloc(ERROR, sizeof(int));
+ *((int *) *extra) = flags;
+
+ return true;
+}
+
+extern void
+assign_io_direct(const char *newval, void *extra)
+{
+ int *flags = (int *) extra;
+
+ io_direct_flags = *flags;
+}
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index efa9773a4d..5647abeffd 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -142,6 +142,16 @@ static MdfdVec *_mdfd_getseg(SMgrRelation reln, ForkNumber forknum,
static BlockNumber _mdnblocks(SMgrRelation reln, ForkNumber forknum,
MdfdVec *seg);
+static inline int
+_mdfd_open_flags(ForkNumber forkNum)
+{
+ int flags = O_RDWR | PG_BINARY;
+
+ if (io_direct_flags & IO_DIRECT_DATA)
+ flags |= PG_O_DIRECT;
+
+ return flags;
+}
/*
* mdinit() -- Initialize private state for magnetic disk storage manager.
@@ -205,14 +215,14 @@ mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo)
path = relpath(reln->smgr_rlocator, forknum);
- fd = PathNameOpenFile(path, O_RDWR | O_CREAT | O_EXCL | PG_BINARY);
+ fd = PathNameOpenFile(path, _mdfd_open_flags(forknum) | O_CREAT | O_EXCL);
if (fd < 0)
{
int save_errno = errno;
if (isRedo)
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
+ fd = PathNameOpenFile(path, _mdfd_open_flags(forknum));
if (fd < 0)
{
/* be sure to report the error reported by create, not open */
@@ -635,7 +645,7 @@ mdopenfork(SMgrRelation reln, ForkNumber forknum, int behavior)
path = relpath(reln->smgr_rlocator, forknum);
- fd = PathNameOpenFile(path, O_RDWR | PG_BINARY);
+ fd = PathNameOpenFile(path, _mdfd_open_flags(forknum));
if (fd < 0)
{
@@ -706,6 +716,8 @@ mdprefetch(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum)
off_t seekpos;
MdfdVec *v;
+ Assert((io_direct_flags & IO_DIRECT_DATA) == 0);
+
v = _mdfd_getseg(reln, forknum, blocknum, false,
InRecovery ? EXTENSION_RETURN_NULL : EXTENSION_FAIL);
if (v == NULL)
@@ -731,6 +743,8 @@ void
mdwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks)
{
+ Assert((io_direct_flags & IO_DIRECT_DATA) == 0);
+
/*
* Issue flush requests in as few requests as possible; have to split at
* segment boundaries though, since those are actually separate files.
@@ -1330,7 +1344,7 @@ _mdfd_openseg(SMgrRelation reln, ForkNumber forknum, BlockNumber segno,
fullpath = _mdfd_segpath(reln, forknum, segno);
/* open the file */
- fd = PathNameOpenFile(fullpath, O_RDWR | PG_BINARY | oflags);
+ fd = PathNameOpenFile(fullpath, _mdfd_open_flags(forknum) | oflags);
pfree(fullpath);
@@ -1540,7 +1554,7 @@ mdsyncfiletag(const FileTag *ftag, char *path)
strlcpy(path, p, MAXPGPATH);
pfree(p);
- file = PathNameOpenFile(path, O_RDWR | PG_BINARY);
+ file = PathNameOpenFile(path, _mdfd_open_flags(ftag->forknum));
if (file < 0)
return -1;
need_to_close = true;
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index c37c246b77..70d0d570b1 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -20,6 +20,7 @@
#include "access/xlogutils.h"
#include "lib/ilist.h"
#include "storage/bufmgr.h"
+#include "storage/fd.h"
#include "storage/ipc.h"
#include "storage/md.h"
#include "storage/smgr.h"
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index e8e8245e91..d3ed527e3b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -568,6 +568,7 @@ static char *locale_ctype;
static char *server_encoding_string;
static char *server_version_string;
static int server_version_num;
+static char *io_direct_string;
#ifdef HAVE_SYSLOG
#define DEFAULT_SYSLOG_FACILITY LOG_LOCAL0
@@ -4565,6 +4566,17 @@ struct config_string ConfigureNamesString[] =
check_backtrace_functions, assign_backtrace_functions, NULL
},
+ {
+ {"io_direct", PGC_POSTMASTER, DEVELOPER_OPTIONS,
+ gettext_noop("Use direct I/O for file access."),
+ NULL,
+ GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE
+ },
+ &io_direct_string,
+ "",
+ check_io_direct, assign_io_direct, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, NULL, NULL, NULL, NULL
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index faac4914fe..6791a406fc 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -44,6 +44,7 @@
#define FD_H
#include <dirent.h>
+#include <fcntl.h>
typedef enum RecoveryInitSyncMethod
{
@@ -54,10 +55,16 @@ typedef enum RecoveryInitSyncMethod
typedef int File;
+#define IO_DIRECT_DATA 0x01
+#define IO_DIRECT_WAL 0x02
+#define IO_DIRECT_WAL_INIT 0x04
+
+
/* GUC parameter */
extern PGDLLIMPORT int max_files_per_process;
extern PGDLLIMPORT bool data_sync_retry;
extern PGDLLIMPORT int recovery_init_sync_method;
+extern PGDLLIMPORT int io_direct_flags;
/*
* This is private to fd.c, but exported for save/restore_backend_variables()
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index a9a179aaba..17fba6f91a 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -17,6 +17,7 @@
#include "lib/ilist.h"
#include "storage/block.h"
#include "storage/relfilelocator.h"
+#include "utils/guc.h"
/*
* smgr.c maintains a table of SMgrRelation objects, which are essentially
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index f722fb250a..a82a85c940 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -156,5 +156,7 @@ extern bool check_wal_consistency_checking(char **newval, void **extra,
GucSource source);
extern void assign_wal_consistency_checking(const char *newval, void *extra);
extern void assign_xlog_sync_method(int new_sync_method, void *extra);
+extern bool check_io_direct(char **newval, void **extra, GucSource source);
+extern void assign_io_direct(const char *newval, void *extra);
#endif /* GUC_HOOKS_H */
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.build
index 21bde427b4..911084ac0f 100644
--- a/src/test/modules/test_misc/meson.build
+++ b/src/test/modules/test_misc/meson.build
@@ -9,6 +9,7 @@ tests += {
't/001_constraint_validation.pl',
't/002_tablespace.pl',
't/003_check_guc.pl',
+ 't/004_io_direct.pl',
],
},
}
diff --git a/src/test/modules/test_misc/t/004_io_direct.pl b/src/test/modules/test_misc/t/004_io_direct.pl
new file mode 100644
index 0000000000..78646e945e
--- /dev/null
+++ b/src/test/modules/test_misc/t/004_io_direct.pl
@@ -0,0 +1,48 @@
+# Very simple exercise of direct I/O GUC.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+# Systems that we know to have direct I/O support, and whose typical local
+# filesystems support it or at least won't fail with an error. (illumos should
+# probably be in this list, but perl reports it as solaris. Solaris should not
+# be in the list because we don't support its way of turning on direct I/O, and
+# even if we did, its version of ZFS rejects it, and OpenBSD just doesn't have
+# it.)
+if (!grep { $^O eq $_ } qw(aix darwin dragonfly freebsd linux MSWin32 netbsd))
+{
+ plan skip_all => "no direct I/O support";
+}
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf', qq{
+io_direct = 'data,wal,wal_init'
+shared_buffers = '256kB' # tiny to force I/O
+});
+$node->start;
+
+# Do some work that is bound to generate shared and local writes and reads as a
+# simple exercise.
+$node->safe_psql('postgres', 'create table t1 as select 1 as i from generate_series(1, 10000)');
+$node->safe_psql('postgres', 'create table t2count (i int)');
+$node->safe_psql('postgres', qq{
+begin;
+create temporary table t2 as select 1 as i from generate_series(1, 10000);
+update t2 set i = i;
+insert into t2count select count(*) from t2;
+commit;
+});
+$node->safe_psql('postgres', 'update t1 set i = i');
+is('10000', $node->safe_psql('postgres', 'select count(*) from t1'), "read back from shared");
+is('10000', $node->safe_psql('postgres', 'select * from t2count'), "read back from local");
+$node->stop('immediate');
+
+$node->start;
+is('10000', $node->safe_psql('postgres', 'select count(*) from t1'), "read back from shared after crash recovery");
+$node->stop;
+
+done_testing();
--
2.39.2
From f5318b888ad14f4f88ccd71511c64b3b990d939b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Tue, 13 Dec 2022 16:55:09 +1300
Subject: [PATCH v4 3/3] XXX turn on direct I/O by default, just for CI
---
src/backend/utils/misc/guc_tables.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index d3ed527e3b..2f95b86e19 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4573,7 +4573,7 @@ struct config_string ConfigureNamesString[] =
GUC_LIST_INPUT | GUC_NOT_IN_SAMPLE
},
&io_direct_string,
- "",
+ "data,wal,wal_init",
check_io_direct, assign_io_direct, NULL
},
--
2.39.2
