Hi, On Thu, Sep 3, 2026 at 10:41 PM Michael Paquier <[email protected]> wrote: > > + } > + else if (state->seg.ws_file >= 0) > + { > + /* > + * A read fully satisfied from WAL buffers skips WALRead(), which is > + * where ws_file is closed and reopened as the reader crosses > + * segments. So a buffer-only read never notices the segment change. > + * ws_file stays open on the old segment while ReadPageInternal() > + * advances ws_segno. For example, when the first page of segment 2 > + * comes from buffers, ws_segno becomes 2 but ws_file is still open > on > + * segment 1. A later read of segment 2 that falls back to the file > + * reuses the stale descriptor, since WALRead() decides whether to > + * reopen from ws_segno (already 2) rather than the open file. It > + * reads segment 1 and returns the wrong segment's WAL, seen during > + * decoding as an "unexpected pageaddr" error. Close the segment > after > + * a buffer-only read so the next file read reopens the correct one. > + */ > + state->routine.segment_close(state); > + } > > Cannot that become wasteful for the logical path when reading pages > from the same segment repeatedly causing opening and closing of the > same file? That sounds relevant to me if we are still attempting > to read from the same segment, depending on wal_buffers whose default > is at 4MB. Something like an extra check based on XLByteInSeg() may > be adapted, using the targetPagePtr, where we could close the segment > only if we target a page not on the same segment? > > The same argument applies to both v6-0001 and v6-0002, both > unconditionally closing a segment after completing a read from buffer > or even not completing a read from buffers and completing the read > with an extra WALRead().
Ah, yes, that is not acceptable. Nice catch! Fixed it by closing the old segment only when the first WAL page of the new segment is fully read from buffers. > + Assert(rbytes == count); > > This assert feels redundant due to the other checks done above. > Applies to patches 0001 and 0002. Removed. Please find the attached v7 patches. -- Bharath Rupireddy Amazon Web Services: https://aws.amazon.com
From 0f8b4da1d9f0cc767ca178e6051c63ad6b4c345f Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Sat, 5 Sep 2026 23:22:33 +0000 Subject: [PATCH v7 1/3] Use WALReadFromBuffers() for logical replication walsenders. Commit 91f2cae7a4 introduced WALReadFromBuffers() but used it only for physical replication walsenders. This commit uses it for logical replication walsenders as well, so that logical decoding can also read WAL from the WAL buffers instead of always going to a file. When a logical replication consumer keeps up with WAL generation, the requested WAL is often still in the WAL buffers, so it can be read from there instead of from a file. The gain is largest with WAL direct I/O, where a file read is a physical disk read. Without direct I/O it still saves a syscall and does not regress. The benefit depends on the workload and how closely the consumer follows the insertion point. A read fully satisfied from WAL buffers skips the file read path, which is also where the reader closes and reopens its segment file as it crosses a segment boundary. So a buffer-only read never notices a segment change. The segment file stays open on the old segment while the reader's segment number advances to the new one. For example, when the first page of segment 2 comes from buffers, the reader's segment number becomes 2 but its file is still open on segment 1. A later read of segment 2 that falls back to the file reuses that stale open file and returns segment 1's data, seen during decoding as an "unexpected pageaddr" error. Fix this by closing the open segment when a buffer-only read is not in the open segment, so the next file read reopens the correct one. Reads that stay within the open segment leave it alone, as the reader's segment number does not change. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Jingtang Zhang <[email protected]> Reviewed-by: Nitin Jadhav <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://www.postgresql.org/message-id/CALj2ACVfF2Uj9NoFy-5m98HNtjHpuD17EDE9twVeJng-jTAe7A%40mail.gmail.com --- src/backend/replication/walsender.c | 45 ++++++++++++++++++++++++----- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c index e9331de3df5..9f9b3b3f729 100644 --- a/src/backend/replication/walsender.c +++ b/src/backend/replication/walsender.c @@ -1098,6 +1098,7 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req WALReadError errinfo; XLogSegNo segno; TimeLineID currTLI; + Size rbytes; /* * Make sure we have enough WAL available before retrieving the current @@ -1157,16 +1158,44 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req else count = flushptr - targetPagePtr; /* part of the page available */ - /* now actually read the data, we know it's there */ - if (!WALRead(state, - cur_page, - targetPagePtr, - count, - currTLI, /* Pass the current TLI because only + /* attempt to read WAL from WAL buffers first */ + rbytes = WALReadFromBuffers(cur_page, targetPagePtr, count, currTLI); + + /* now read the remaining WAL from WAL file */ + if (rbytes < count) + { + if (!WALRead(state, + cur_page + rbytes, + targetPagePtr + rbytes, + count - rbytes, + currTLI, /* Pass the current TLI because only * WalSndSegmentOpen controls whether new TLI * is needed. */ - &errinfo)) - WALReadRaiseError(&errinfo); + &errinfo)) + WALReadRaiseError(&errinfo); + } + else if (state->seg.ws_file >= 0 && + !XLByteInSeg(targetPagePtr, state->seg.ws_segno, + state->segcxt.ws_segsize)) + { + /* + * A read fully satisfied from WAL buffers skips WALRead(), which is + * where ws_file is closed and reopened as the reader crosses + * segments. So a buffer-only read never notices the segment change. + * ws_file stays open on the old segment while ReadPageInternal() + * advances ws_segno. For example, when the first page of segment 2 + * comes from buffers, ws_segno becomes 2 but ws_file is still open on + * segment 1. A later read of segment 2 that falls back to the file + * reuses the stale descriptor, since WALRead() decides whether to + * reopen from ws_segno (already 2) rather than the open file. It + * reads segment 1 and returns the wrong segment's WAL, seen during + * decoding as an "unexpected pageaddr" error. Close the segment when + * the page just read from buffers is not in the open segment, so the + * next file read reopens the correct one. Reads staying within the + * open segment leave it alone, because ws_segno does not change. + */ + state->routine.segment_close(state); + } /* * After reading into the buffer, check that what we read was valid. We do -- 2.47.3
From 309e5f0fa1c43f40d1ab6a5579f8cb39e3d1a70f Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Sat, 5 Sep 2026 23:22:33 +0000 Subject: [PATCH v7 2/3] Use WALReadFromBuffers() for local WAL reads. Commit 91f2cae7a4 introduced WALReadFromBuffers() for physical replication walsenders, and it has since been used for logical replication walsenders too. This commit uses it for the remaining callers that read WAL from the local server through the shared page-read callback, namely logical decoding driven by SQL functions, two-phase commit, repack workers, and pg_walinspect. When the requested WAL is still in the WAL buffers, it can be read from there instead of from a file. The gain is largest with WAL direct I/O, where a file read is a physical disk read. Without direct I/O it still saves a syscall and does not regress. The benefit depends on the workload and how closely the caller follows the insertion point. The timeline passed to WALReadFromBuffers() is the one the read targets, the same one passed to WALRead(), so that a read on a historical timeline never comes from the WAL buffers. As for the logical replication walsender, a read fully satisfied from WAL buffers can leave the segment file open on the wrong segment. Fix this by closing the open segment when such a read is not in the open segment, so the next file read reopens the correct one. Author: Bharath Rupireddy <[email protected]> Reviewed-by: Jingtang Zhang <[email protected]> Reviewed-by: Nitin Jadhav <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://www.postgresql.org/message-id/CALj2ACVfF2Uj9NoFy-5m98HNtjHpuD17EDE9twVeJng-jTAe7A%40mail.gmail.com --- src/backend/access/transam/xlogutils.c | 29 +++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c index 58b9dab6a90..75c2ef37f95 100644 --- a/src/backend/access/transam/xlogutils.c +++ b/src/backend/access/transam/xlogutils.c @@ -900,6 +900,7 @@ read_local_xlog_page_guts(XLogReaderState *state, XLogRecPtr targetPagePtr, int count; WALReadError errinfo; TimeLineID currTLI; + Size rbytes; loc = targetPagePtr + reqLen; @@ -1031,9 +1032,31 @@ read_local_xlog_page_guts(XLogReaderState *state, XLogRecPtr targetPagePtr, count = read_upto - targetPagePtr; } - if (!WALRead(state, cur_page, targetPagePtr, count, tli, - &errinfo)) - WALReadRaiseError(&errinfo); + /* attempt to read WAL from WAL buffers first */ + rbytes = WALReadFromBuffers(cur_page, targetPagePtr, count, tli); + + /* now read the remaining WAL from WAL file */ + if (rbytes < count) + { + if (!WALRead(state, + cur_page + rbytes, + targetPagePtr + rbytes, + count - rbytes, + tli, + &errinfo)) + WALReadRaiseError(&errinfo); + } + else if (state->seg.ws_file >= 0 && + !XLByteInSeg(targetPagePtr, state->seg.ws_segno, + state->segcxt.ws_segsize)) + { + /* + * Close the segment when a read fully satisfied from WAL buffers is + * not in the open segment, so the next file read reopens the correct + * one. See logical_read_xlog_page() for why this is needed. + */ + state->routine.segment_close(state); + } /* number of valid bytes in the buffer */ return count; -- 2.47.3
From 69c1d5e90b0dba53ef7464000e6f82a15fdd50f7 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <[email protected]> Date: Sat, 5 Sep 2026 23:22:33 +0000 Subject: [PATCH v7 3/3] Test reading WAL from buffers across a segment boundary. The previous two commits let more WAL readers use WALReadFromBuffers() and close the open segment after a read fully satisfied from WAL buffers that crosses into a new segment. This commit adds a test for that. A new injection point, wal-read-from-buffers-force-miss, forces a buffer miss for reads that do not start at a segment boundary. With it attached, the first page of a new segment is read from WAL buffers while the next page falls back to the file, which reproduces the stale open segment unless it is closed after the buffer-only read. The test checks this for both pg_walinspect and a logical walsender. Author: Bharath Rupireddy <[email protected]> Discussion: https://www.postgresql.org/message-id/CALj2ACVfF2Uj9NoFy-5m98HNtjHpuD17EDE9twVeJng-jTAe7A%40mail.gmail.com --- src/backend/access/transam/xlog.c | 10 +++ src/test/subscription/Makefile | 2 + src/test/subscription/meson.build | 1 + .../subscription/t/099_wal_buffers_read.pl | 67 +++++++++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 src/test/subscription/t/099_wal_buffers_read.pl diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 2e3f177100b..862d8966438 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -1805,6 +1805,16 @@ WALReadFromBuffers(char *dstbuf, XLogRecPtr startptr, Size count, if (RecoveryInProgress() || tli != GetWALInsertionTimeLine()) return 0; + /* + * Force a buffer miss for reads not starting at a segment boundary. See + * logical_read_xlog_page() for details. + */ +#ifdef USE_INJECTION_POINTS + if (XLogSegmentOffset(startptr, wal_segment_size) != 0 && + IS_INJECTION_POINT_ATTACHED("wal-read-from-buffers-force-miss")) + return 0; +#endif + Assert(XLogRecPtrIsValid(startptr)); /* diff --git a/src/test/subscription/Makefile b/src/test/subscription/Makefile index 1b22703dc21..01306dbd378 100644 --- a/src/test/subscription/Makefile +++ b/src/test/subscription/Makefile @@ -14,6 +14,8 @@ top_builddir = ../../.. include $(top_builddir)/src/Makefile.global EXTRA_INSTALL = contrib/hstore \ + contrib/pg_walinspect \ + contrib/test_decoding \ src/test/modules/injection_points export with_icu diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build index e71e95c6297..82bb3cad962 100644 --- a/src/test/subscription/meson.build +++ b/src/test/subscription/meson.build @@ -48,6 +48,7 @@ tests += { 't/036_sequences.pl', 't/037_except.pl', 't/038_walsnd_shutdown_timeout.pl', + 't/099_wal_buffers_read.pl', 't/100_bugs.pl', ], }, diff --git a/src/test/subscription/t/099_wal_buffers_read.pl b/src/test/subscription/t/099_wal_buffers_read.pl new file mode 100644 index 00000000000..9ffe6ec9cc0 --- /dev/null +++ b/src/test/subscription/t/099_wal_buffers_read.pl @@ -0,0 +1,67 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# A WAL read served from WAL buffers must not leave a stale open segment +# behind for a later file read. Exercised for read_local_xlog_page() (via +# pg_walinspect) and the logical walsender (via pg_recvlogical). + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init(allows_streaming => 'logical'); +# Keep recent WAL in buffers so the new segment's first page is read from +# buffers rather than from a file, and keep the WAL writer asleep so that its +# opportunistic buffer pre-initialization does not evict that page meanwhile. +$node->append_conf( + 'postgresql.conf', qq(wal_buffers = 64MB +wal_writer_delay = 10s)); +$node->start; + +if (!$node->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points'); +$node->safe_psql('postgres', 'CREATE EXTENSION pg_walinspect'); +$node->safe_psql('postgres', + "SELECT pg_create_logical_replication_slot('slot', 'test_decoding')"); +$node->safe_psql('postgres', + "SELECT injection_points_attach('wal-read-from-buffers-force-miss', 'notice')"); + +# Emit one message per WAL page in a segment, plus a few more, so that the WAL +# written after the switch crosses a segment boundary by a couple of pages. +my $seg_size = $node->safe_psql('postgres', + "SELECT pg_size_bytes(current_setting('wal_segment_size'))"); +$node->safe_psql('postgres', 'SELECT pg_switch_wal()'); +my $start_lsn = $node->safe_psql('postgres', 'SELECT pg_current_wal_lsn()'); +$node->safe_psql('postgres', + "SELECT count(pg_logical_emit_message(false, 'test', repeat('x', 8192))) + FROM generate_series(1, $seg_size / 8192 + 16)"); +my $end_lsn = $node->safe_psql('postgres', + "SELECT pg_logical_emit_message(false, 'test', 'flush', true)"); + +# read_local_xlog_page() path. +my ($ret, $stdout, $stderr) = $node->psql('postgres', + "SELECT count(*) > 0 FROM pg_get_wal_records_info('$start_lsn', '$end_lsn')"); +is($ret, 0, 'pg_walinspect reads across a segment boundary'); +is($stdout, 't', 'pg_walinspect returns records across the boundary'); + +# logical walsender path. +my ($rc, $rout, $rerr) = $node->pg_recvlogical_upto('postgres', 'slot', + $end_lsn, $PostgreSQL::Test::Utils::timeout_default); +is($rc, 0, 'walsender decodes across a segment boundary'); +unlike($rerr, qr/unexpected pageaddr/, 'walsender did not reuse a stale segment'); + +$node->safe_psql('postgres', + "SELECT injection_points_detach('wal-read-from-buffers-force-miss')"); + +done_testing(); -- 2.47.3
