agora inbox for pgsql-hackers@postgresql.org  

help / color / mirror / Atom feed

[PATCH] Fix timeline-tracking failure while sending a historic timeline
8+ messages / 2 participants
[nested] [flat]

* [PATCH] Fix timeline-tracking failure while sending a historic timeline
@ 2021-01-05 04:34 Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
  0 siblings, 0 replies; 8+ messages in thread

From: Kyotaro Horiguchi @ 2021-01-05 04:34 UTC (permalink / raw)

Walsender should track timeline switches while sending a historic
timeline. Regain that behavior, which was broken in PG13, by a thinko
of 709d003fbd. Backpatch to PG13.
---
 src/backend/replication/walsender.c       |  2 +-
 src/test/perl/PostgresNode.pm             | 33 ++++++++++++++++++
 src/test/perl/TestLib.pm                  | 16 ++++++---
 src/test/recovery/t/001_stream_rep.pl     | 41 ++++++++++++++++++++++-
 src/test/recovery/t/019_replslot_limit.pl | 37 ++++----------------
 5 files changed, 92 insertions(+), 37 deletions(-)

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 7f87eb7f19..04f6c3ebb4 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2478,7 +2478,7 @@ WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
 		XLogSegNo	endSegNo;
 
 		XLByteToSeg(sendTimeLineValidUpto, endSegNo, state->segcxt.ws_segsize);
-		if (state->seg.ws_segno == endSegNo)
+		if (nextSegNo == endSegNo)
 			*tli_p = sendTimeLineNextTLI;
 	}
 
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 980f1f1533..687aa3ac88 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2138,6 +2138,39 @@ sub pg_recvlogical_upto
 
 =pod
 
+=item $node->current_log_position()
+
+Return the current position of server log.
+
+=cut
+
+sub current_log_position
+{
+	my $self = shift;
+
+	return (stat $self->logfile)[7];
+}
+
+=pod
+
+=item $node->find_in_log($pattern, $startpos)
+
+Returns whether the $pattern occurs after $startpos in the server log.
+
+=cut
+
+sub find_in_log
+{
+	my ($self, $pattern, $startpos) = @_;
+
+	$startpos = 0 unless defined $startpos;
+	my $log = TestLib::slurp_file($self->logfile, $startpos);
+
+	return $log =~ m/$pattern/;
+}
+
+=pod
+
 =back
 
 =cut
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index a7490d2ce7..a0ce9521e2 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -402,30 +402,38 @@ sub slurp_dir
 
 =pod
 
-=item slurp_file(filename)
+=item slurp_file(filename, pos)
 
-Return the full contents of the specified file.
+Return the contents after pos of the specified file.
+Reutrns the full contents if pos is omitted.
 
 =cut
 
 sub slurp_file
 {
-	my ($filename) = @_;
+	my ($filename, $from) = @_;
 	local $/;
 	my $contents;
+
+	$from = 0 unless defined $from;
+
 	if ($Config{osname} ne 'MSWin32')
 	{
 		open(my $in, '<', $filename)
 		  or die "could not read \"$filename\": $!";
+		seek($in, $from, 0)
+		  or die "could not seek \"$filename\" to $from: $!";
 		$contents = <$in>;
 		close $in;
 	}
 	else
 	{
 		my $fHandle = createFile($filename, "r", "rwd")
-		  or die "could not open \"$filename\": $^E";
+		  or die "could not open \"$filename\": $^E\n";
 		OsFHandleOpen(my $fh = IO::Handle->new(), $fHandle, 'r')
 		  or die "could not read \"$filename\": $^E\n";
+		seek($fh, $from, 0)
+		  or die "could not seek \"$filename\" to $from: $^E\n";
 		$contents = <$fh>;
 		CloseHandle($fHandle)
 		  or die "could not close \"$filename\": $^E\n";
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 778f11b28b..8d2b24fe55 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -2,8 +2,9 @@
 use strict;
 use warnings;
 use PostgresNode;
+use Time::HiRes qw(usleep);
 use TestLib;
-use Test::More tests => 36;
+use Test::More tests => 37;
 
 # Initialize master node
 my $node_master = get_new_node('master');
@@ -409,3 +410,41 @@ ok( ($phys_restart_lsn_pre cmp $phys_restart_lsn_post) == 0,
 my $master_data = $node_master->data_dir;
 ok(!-f "$master_data/pg_wal/$segment_removed",
 	"WAL segment $segment_removed recycled after physical slot advancing");
+
+#
+# Check if timeline-increment works while reading a historic timeline.
+my $node_primary_2 = get_new_node('primary_2');
+# archiving is needed to create .paritial segment
+$node_primary_2->init(allows_streaming => 1, has_archiving => 1);
+$node_primary_2->start;
+$node_primary_2->backup($backup_name);
+my $node_standby_3 = get_new_node('standby_3');
+$node_standby_3->init_from_backup($node_primary_2, $backup_name,
+								  has_streaming => 1);
+$node_primary_2->stop;
+$node_primary_2->set_standby_mode; # increment primary timeline
+$node_primary_2->start;
+$node_primary_2->promote;
+my $logstart = $node_standby_3->current_log_position();
+$node_standby_3->start;
+
+my $success = 0;
+for (my $i = 0 ; $i < 1000; $i++)
+{
+	if ($node_standby_3->find_in_log(
+			"requested WAL segment [0-9A-F]+ has already been removed",
+			$logstart))
+	{
+		last;
+	}
+	elsif ($node_standby_3->find_in_log(
+			"End of WAL reached on timeline",
+			   $logstart))
+	{
+		$success = 1;
+		last;
+	}
+	usleep(100_000);
+}
+
+ok($success, 'Timeline increment while reading a historic timeline');
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index a7231dcd47..8b3c5de057 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -165,19 +165,17 @@ $node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
 
 $node_standby->stop;
 
-ok( !find_in_log(
-		$node_standby,
-		"requested WAL segment [0-9A-F]+ has already been removed"),
+ok( !$node_standby->find_in_log(
+		 "requested WAL segment [0-9A-F]+ has already been removed"),
 	'check that required WAL segments are still available');
 
 # Advance WAL again, the slot loses the oldest segment.
-my $logstart = get_log_size($node_master);
+my $logstart = $node_master->current_log_position();
 advance_wal($node_master, 7);
 $node_master->safe_psql('postgres', "CHECKPOINT;");
 
 # WARNING should be issued
-ok( find_in_log(
-		$node_master,
+ok( $node_master->find_in_log(
 		"invalidating slot \"rep1\" because its restart_lsn [0-9A-F/]+ exceeds max_slot_wal_keep_size",
 		$logstart),
 	'check that the warning is logged');
@@ -190,14 +188,13 @@ is($result, "rep1|f|t|lost|",
 	'check that the slot became inactive and the state "lost" persists');
 
 # The standby no longer can connect to the master
-$logstart = get_log_size($node_standby);
+$logstart = $node_standby->current_log_position();
 $node_standby->start;
 
 my $failed = 0;
 for (my $i = 0; $i < 10000; $i++)
 {
-	if (find_in_log(
-			$node_standby,
+	if ($node_standby->find_in_log(
 			"requested WAL segment [0-9A-F]+ has already been removed",
 			$logstart))
 	{
@@ -264,25 +261,3 @@ sub advance_wal
 	}
 	return;
 }
-
-# return the size of logfile of $node in bytes
-sub get_log_size
-{
-	my ($node) = @_;
-
-	return (stat $node->logfile)[7];
-}
-
-# find $pat in logfile of $node after $off-th byte
-sub find_in_log
-{
-	my ($node, $pat, $off) = @_;
-
-	$off = 0 unless defined $off;
-	my $log = TestLib::slurp_file($node->logfile);
-	return 0 if (length($log) <= $off);
-
-	$log = substr($log, $off);
-
-	return $log =~ m/$pat/;
-}
-- 
2.27.0


----Next_Part(Wed_Jan__6_10_48_04_2021_104)----





^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.8 26/38] aio: Basic read_stream adjustments for real AIO
@ 2025-03-14 15:39 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-14 15:39 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency

There is one comment talking about max_ios logic with "real asynchronous I/O"
that I am not sure about, so I left it alone for now.

There are further improvements we should consider, e.g. waiting to issue IOs
until we can issue multiple IOs at once. But that's left for a future change,
since it would involve additional heuristics.
---
 src/backend/storage/aio/read_stream.c | 39 ++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 0f1525f46c5..b44cc358f29 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,7 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	bool		sync_mode;
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -420,6 +422,13 @@ read_stream_start_pending_read(ReadStream *stream)
 static void
 read_stream_look_ahead(ReadStream *stream)
 {
+	/*
+	 * Allow amortizing the cost of submitting IO over multiple IOs. This
+	 * requires that we don't do any operations that could lead to a deadlock
+	 * with staged-but-unsubmitted IO.
+	 */
+	pgaio_enter_batchmode();
+
 	while (stream->ios_in_progress < stream->max_ios &&
 		   stream->pinned_buffers + stream->pending_read_nblocks < stream->distance)
 	{
@@ -467,6 +476,7 @@ read_stream_look_ahead(ReadStream *stream)
 			{
 				/* We've hit the buffer or I/O limit.  Rewind and stop here. */
 				read_stream_unget_block(stream, blocknum);
+				pgaio_exit_batchmode();
 				return;
 			}
 		}
@@ -501,6 +511,8 @@ read_stream_look_ahead(ReadStream *stream)
 	 * time.
 	 */
 	Assert(stream->pinned_buffers > 0 || stream->distance == 0);
+
+	pgaio_exit_batchmode();
 }
 
 /*
@@ -556,12 +568,12 @@ read_stream_begin_impl(int flags,
 		max_ios = get_tablespace_io_concurrency(tablespace_id);
 
 	/*
-	 * XXX Since we don't have asynchronous I/O yet, if direct I/O is enabled
-	 * then just behave as though I/O concurrency is set to 0.  Otherwise we
-	 * would look ahead pinning many buffers for no benefit, for lack of
-	 * advice and AIO.
+	 * If real asynchronous I/O is disabled, and direct I/O is enabled, just
+	 * behave as though I/O concurrency is set to 0.  Otherwise we would look
+	 * ahead pinning many buffers for no benefit, as the advice-based
+	 * readahead doesn't support direct I/O.
 	 */
-	if (io_direct_flags & IO_DIRECT_DATA)
+	if (io_method == IOMETHOD_SYNC && (io_direct_flags & IO_DIRECT_DATA))
 		max_ios = 0;
 
 	/* Cap to INT16_MAX to avoid overflowing below */
@@ -641,15 +653,19 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * This system supports prefetching advice.
+	 *
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
@@ -659,6 +675,9 @@ read_stream_begin_impl(int flags,
 	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
 	 * above.  If we had real asynchronous I/O we might need a slightly
 	 * different definition.
+	 *
+	 * FIXME: Not sure what different definition we would need? I guess we
+	 * could add the READ_BUFFERS_SYNCHRONOUSLY flag automatically?
 	 */
 	if (max_ios == 0)
 		max_ios = 1;
-- 
2.48.1.76.g4e746b1a31.dirty


--ow5flh3n247znjrs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.8-0027-aio-Experimental-heuristics-to-increase-batchin.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.9 18/30] aio: Basic read_stream adjustments for real AIO
@ 2025-03-15 16:29 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-15 16:29 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency

There is one comment talking about max_ios logic with "real asynchronous I/O"
that I am not sure about, so I left it alone for now.

There are further improvements we should consider, e.g. waiting to issue IOs
until we can issue multiple IOs at once. But that's left for a future change,
since it would involve additional heuristics.
---
 src/backend/storage/aio/read_stream.c | 29 ++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index cdf4b5a86a2..d7b395b86a3 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,7 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	bool		sync_mode;
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -416,6 +418,13 @@ read_stream_start_pending_read(ReadStream *stream)
 static void
 read_stream_look_ahead(ReadStream *stream)
 {
+	/*
+	 * Allow amortizing the cost of submitting IO over multiple IOs. This
+	 * requires that we don't do any operations that could lead to a deadlock
+	 * with staged-but-unsubmitted IO.
+	 */
+	pgaio_enter_batchmode();
+
 	while (stream->ios_in_progress < stream->max_ios &&
 		   stream->pinned_buffers + stream->pending_read_nblocks < stream->distance)
 	{
@@ -463,6 +472,7 @@ read_stream_look_ahead(ReadStream *stream)
 			{
 				/* We've hit the buffer or I/O limit.  Rewind and stop here. */
 				read_stream_unget_block(stream, blocknum);
+				pgaio_exit_batchmode();
 				return;
 			}
 		}
@@ -497,6 +507,8 @@ read_stream_look_ahead(ReadStream *stream)
 	 * time.
 	 */
 	Assert(stream->pinned_buffers > 0 || stream->distance == 0);
+
+	pgaio_exit_batchmode();
 }
 
 /*
@@ -628,15 +640,19 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * This system supports prefetching advice.
+	 *
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
@@ -646,6 +662,9 @@ read_stream_begin_impl(int flags,
 	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
 	 * above.  If we had real asynchronous I/O we might need a slightly
 	 * different definition.
+	 *
+	 * FIXME: Not sure what different definition we would need? I guess we
+	 * could add the READ_BUFFERS_SYNCHRONOUSLY flag automatically?
 	 */
 	if (max_ios == 0)
 		max_ios = 1;
-- 
2.48.1.76.g4e746b1a31.dirty


--23jbdfobqrqxnmx5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.9-0019-aio-Experimental-heuristics-to-increase-batchin.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.10 12/28] aio: Basic read_stream adjustments for real AIO
@ 2025-03-18 18:40 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-18 18:40 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency

There is one comment talking about max_ios logic with "real asynchronous I/O"
that I am not sure about, so I left it alone for now.

There are further improvements we should consider, e.g. waiting to issue IOs
until we can issue multiple IOs at once. But that's left for a future change,
since it would involve additional heuristics.
---
 src/backend/storage/aio/read_stream.c | 29 ++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index cdf4b5a86a2..d7b395b86a3 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,7 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	bool		sync_mode;
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -416,6 +418,13 @@ read_stream_start_pending_read(ReadStream *stream)
 static void
 read_stream_look_ahead(ReadStream *stream)
 {
+	/*
+	 * Allow amortizing the cost of submitting IO over multiple IOs. This
+	 * requires that we don't do any operations that could lead to a deadlock
+	 * with staged-but-unsubmitted IO.
+	 */
+	pgaio_enter_batchmode();
+
 	while (stream->ios_in_progress < stream->max_ios &&
 		   stream->pinned_buffers + stream->pending_read_nblocks < stream->distance)
 	{
@@ -463,6 +472,7 @@ read_stream_look_ahead(ReadStream *stream)
 			{
 				/* We've hit the buffer or I/O limit.  Rewind and stop here. */
 				read_stream_unget_block(stream, blocknum);
+				pgaio_exit_batchmode();
 				return;
 			}
 		}
@@ -497,6 +507,8 @@ read_stream_look_ahead(ReadStream *stream)
 	 * time.
 	 */
 	Assert(stream->pinned_buffers > 0 || stream->distance == 0);
+
+	pgaio_exit_batchmode();
 }
 
 /*
@@ -628,15 +640,19 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * This system supports prefetching advice.
+	 *
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
@@ -646,6 +662,9 @@ read_stream_begin_impl(int flags,
 	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
 	 * above.  If we had real asynchronous I/O we might need a slightly
 	 * different definition.
+	 *
+	 * FIXME: Not sure what different definition we would need? I guess we
+	 * could add the READ_BUFFERS_SYNCHRONOUSLY flag automatically?
 	 */
 	if (max_ios == 0)
 		max_ios = 1;
-- 
2.48.1.76.g4e746b1a31.dirty


--w6dfit2y42fwvotd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.10-0013-docs-Reframe-track_io_timing-related-docs-as-w.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.11 13/27] aio: Basic read_stream adjustments for real AIO
@ 2025-03-18 18:40 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-18 18:40 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency

There is one comment talking about max_ios logic with "real asynchronous I/O"
that I am not sure about, so I left it alone for now.

There are further improvements we should consider, e.g. waiting to issue IOs
until we can issue multiple IOs at once. But that's left for a future change,
since it would involve additional heuristics.
---
 src/backend/storage/aio/read_stream.c | 29 ++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index 18ecf4affc7..60a841816d8 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,7 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	bool		sync_mode;
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -416,6 +418,13 @@ read_stream_start_pending_read(ReadStream *stream)
 static void
 read_stream_look_ahead(ReadStream *stream)
 {
+	/*
+	 * Allow amortizing the cost of submitting IO over multiple IOs. This
+	 * requires that we don't do any operations that could lead to a deadlock
+	 * with staged-but-unsubmitted IO.
+	 */
+	pgaio_enter_batchmode();
+
 	while (stream->ios_in_progress < stream->max_ios &&
 		   stream->pinned_buffers + stream->pending_read_nblocks < stream->distance)
 	{
@@ -463,6 +472,7 @@ read_stream_look_ahead(ReadStream *stream)
 			{
 				/* We've hit the buffer or I/O limit.  Rewind and stop here. */
 				read_stream_unget_block(stream, blocknum);
+				pgaio_exit_batchmode();
 				return;
 			}
 		}
@@ -497,6 +507,8 @@ read_stream_look_ahead(ReadStream *stream)
 	 * time.
 	 */
 	Assert(stream->pinned_buffers > 0 || stream->distance == 0);
+
+	pgaio_exit_batchmode();
 }
 
 /*
@@ -629,15 +641,19 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * This system supports prefetching advice.
+	 *
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
@@ -647,6 +663,9 @@ read_stream_begin_impl(int flags,
 	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
 	 * above.  If we had real asynchronous I/O we might need a slightly
 	 * different definition.
+	 *
+	 * FIXME: Not sure what different definition we would need? I guess we
+	 * could add the READ_BUFFERS_SYNCHRONOUSLY flag automatically?
 	 */
 	if (max_ios == 0)
 		max_ios = 1;
-- 
2.48.1.76.g4e746b1a31.dirty


--bjnmbpad43bpmfxt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.11-0014-docs-Reframe-track_io_timing-related-docs-as-w.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.12 10/28] aio: Basic read_stream adjustments for real AIO
@ 2025-03-18 18:40 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-18 18:40 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO

There is one comment talking about max_ios logic with "real asynchronous I/O"
that I am not sure about, so I left it alone for now.

There are further improvements we should consider:
- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency
- We could wait to issue IOs until we can issue multiple IOs at once
---
 src/backend/storage/aio/read_stream.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index c60e37e7f7f..df16530d673 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,7 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	bool		sync_mode;		/* using io_method=sync */
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -613,15 +615,19 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * This system supports prefetching advice.
+	 *
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
@@ -631,6 +637,9 @@ read_stream_begin_impl(int flags,
 	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
 	 * above.  If we had real asynchronous I/O we might need a slightly
 	 * different definition.
+	 *
+	 * FIXME: Not sure what different definition we would need? I guess we
+	 * could add the READ_BUFFERS_SYNCHRONOUSLY flag automatically?
 	 */
 	if (max_ios == 0)
 		max_ios = 1;
-- 
2.48.1.76.g4e746b1a31.dirty


--5i73spx2p4vwf7fe
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.12-0011-read_stream-Introduce-and-use-optional-batchmo.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.13 13/28] aio: Basic read_stream adjustments for real AIO
@ 2025-03-18 18:40 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-18 18:40 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- If effective_io_concurrency=0, pass READ_BUFFERS_SYNCHRONOUSLY to
  StartReadBuffers() to ensure synchronous IO execution

There are further improvements we should consider:

- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency. That however requires care to avoid deadlocks and thus
  done separately.
- It can be beneficial to defer starting new IOs until we can issue multiple
  IOs at once. That however requires non-trivial heuristics to decide when to
  do so.

Co-authored-by: Andres Freund <andres@anarazel.de>
Co-authored-by: Thomas Munro <thomas.munro@gmail.com>
---
 src/backend/storage/aio/read_stream.c | 39 ++++++++++++++++++---------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index c60e37e7f7f..26e5dfe77db 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,8 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	int			read_buffers_flags;
+	bool		sync_mode;		/* using io_method=sync */
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -250,7 +253,7 @@ read_stream_start_pending_read(ReadStream *stream)
 		Assert(stream->next_buffer_index == stream->oldest_buffer_index);
 
 	/* Do we need to issue read-ahead advice? */
-	flags = 0;
+	flags = stream->read_buffers_flags;
 	if (stream->advice_enabled)
 	{
 		if (stream->pending_read_blocknum == stream->seq_blocknum)
@@ -261,7 +264,7 @@ read_stream_start_pending_read(ReadStream *stream)
 			 * then stay of the way of the kernel's own read-ahead.
 			 */
 			if (stream->seq_until_processed != InvalidBlockNumber)
-				flags = READ_BUFFERS_ISSUE_ADVICE;
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
 		}
 		else
 		{
@@ -272,7 +275,7 @@ read_stream_start_pending_read(ReadStream *stream)
 			 */
 			stream->seq_until_processed = stream->pending_read_blocknum;
 			if (stream->pinned_buffers > 0)
-				flags = READ_BUFFERS_ISSUE_ADVICE;
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
 		}
 	}
 
@@ -613,27 +616,33 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * Read-ahead advice simulating asynchronous I/O with synchronous calls.
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
 #endif
 
 	/*
-	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
-	 * above.  If we had real asynchronous I/O we might need a slightly
-	 * different definition.
+	 * Setting max_ios to zero disables AIO and advice-based pseudo AIO, but
+	 * we still need to allocate space to combine and run one I/O.  Bump it up
+	 * to one, and remember to ask for synchronous I/O only.
 	 */
 	if (max_ios == 0)
+	{
 		max_ios = 1;
+		stream->read_buffers_flags = READ_BUFFERS_SYNCHRONOUSLY;
+	}
 
 	/*
 	 * Capture stable values for these two GUC-derived numbers for the
@@ -777,6 +786,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
 
 		if (likely(next_blocknum != InvalidBlockNumber))
 		{
+			int			flags = stream->read_buffers_flags;
+
+			if (stream->advice_enabled)
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
+
 			/*
 			 * Pin a buffer for the next call.  Same buffer entry, and
 			 * arbitrary I/O entry (they're all free).  We don't have to
@@ -792,8 +806,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
 			if (likely(!StartReadBuffer(&stream->ios[0].op,
 										&stream->buffers[oldest_buffer_index],
 										next_blocknum,
-										stream->advice_enabled ?
-										READ_BUFFERS_ISSUE_ADVICE : 0)))
+										flags)))
 			{
 				/* Fast return. */
 				return buffer;
-- 
2.48.1.76.g4e746b1a31.dirty


--pro7bqageygxfsvg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.13-0014-read_stream-Introduce-and-use-optional-batchmo.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* [PATCH v2.14 14/29] aio: Basic read_stream adjustments for real AIO
@ 2025-03-18 18:40 Andres Freund <andres@anarazel.de>
  0 siblings, 0 replies; 8+ messages in thread

From: Andres Freund @ 2025-03-18 18:40 UTC (permalink / raw)

Adapt the read stream logic for real AIO:
- If AIO is enabled, we shouldn't issue advice, but if it isn't, we should
  continue issuing advice
- AIO benefits from reading ahead with direct IO
- If effective_io_concurrency=0, pass READ_BUFFERS_SYNCHRONOUSLY to
  StartReadBuffers() to ensure synchronous IO execution

There are further improvements we should consider:

- While in read_stream_look_ahead(), we can use AIO batch submission mode for
  increased efficiency. That however requires care to avoid deadlocks and thus
  done separately.
- It can be beneficial to defer starting new IOs until we can issue multiple
  IOs at once. That however requires non-trivial heuristics to decide when to
  do so.

Co-authored-by: Andres Freund <andres@anarazel.de>
Co-authored-by: Thomas Munro <thomas.munro@gmail.com>
---
 src/backend/storage/aio/read_stream.c | 39 ++++++++++++++++++---------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c
index c60e37e7f7f..26e5dfe77db 100644
--- a/src/backend/storage/aio/read_stream.c
+++ b/src/backend/storage/aio/read_stream.c
@@ -72,6 +72,7 @@
 #include "postgres.h"
 
 #include "miscadmin.h"
+#include "storage/aio.h"
 #include "storage/fd.h"
 #include "storage/smgr.h"
 #include "storage/read_stream.h"
@@ -99,6 +100,8 @@ struct ReadStream
 	int16		pinned_buffers;
 	int16		distance;
 	int16		initialized_buffers;
+	int			read_buffers_flags;
+	bool		sync_mode;		/* using io_method=sync */
 	bool		advice_enabled;
 	bool		temporary;
 
@@ -250,7 +253,7 @@ read_stream_start_pending_read(ReadStream *stream)
 		Assert(stream->next_buffer_index == stream->oldest_buffer_index);
 
 	/* Do we need to issue read-ahead advice? */
-	flags = 0;
+	flags = stream->read_buffers_flags;
 	if (stream->advice_enabled)
 	{
 		if (stream->pending_read_blocknum == stream->seq_blocknum)
@@ -261,7 +264,7 @@ read_stream_start_pending_read(ReadStream *stream)
 			 * then stay of the way of the kernel's own read-ahead.
 			 */
 			if (stream->seq_until_processed != InvalidBlockNumber)
-				flags = READ_BUFFERS_ISSUE_ADVICE;
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
 		}
 		else
 		{
@@ -272,7 +275,7 @@ read_stream_start_pending_read(ReadStream *stream)
 			 */
 			stream->seq_until_processed = stream->pending_read_blocknum;
 			if (stream->pinned_buffers > 0)
-				flags = READ_BUFFERS_ISSUE_ADVICE;
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
 		}
 	}
 
@@ -613,27 +616,33 @@ read_stream_begin_impl(int flags,
 		stream->per_buffer_data = (void *)
 			MAXALIGN(&stream->ios[Max(1, max_ios)]);
 
+	stream->sync_mode = io_method == IOMETHOD_SYNC;
+
 #ifdef USE_PREFETCH
 
 	/*
-	 * This system supports prefetching advice.  We can use it as long as
-	 * direct I/O isn't enabled, the caller hasn't promised sequential access
-	 * (overriding our detection heuristics), and max_ios hasn't been set to
-	 * zero.
+	 * Read-ahead advice simulating asynchronous I/O with synchronous calls.
+	 * Issue advice only if AIO is not used, direct I/O isn't enabled, the
+	 * caller hasn't promised sequential access (overriding our detection
+	 * heuristics), and max_ios hasn't been set to zero.
 	 */
-	if ((io_direct_flags & IO_DIRECT_DATA) == 0 &&
+	if (stream->sync_mode &&
+		(io_direct_flags & IO_DIRECT_DATA) == 0 &&
 		(flags & READ_STREAM_SEQUENTIAL) == 0 &&
 		max_ios > 0)
 		stream->advice_enabled = true;
 #endif
 
 	/*
-	 * For now, max_ios = 0 is interpreted as max_ios = 1 with advice disabled
-	 * above.  If we had real asynchronous I/O we might need a slightly
-	 * different definition.
+	 * Setting max_ios to zero disables AIO and advice-based pseudo AIO, but
+	 * we still need to allocate space to combine and run one I/O.  Bump it up
+	 * to one, and remember to ask for synchronous I/O only.
 	 */
 	if (max_ios == 0)
+	{
 		max_ios = 1;
+		stream->read_buffers_flags = READ_BUFFERS_SYNCHRONOUSLY;
+	}
 
 	/*
 	 * Capture stable values for these two GUC-derived numbers for the
@@ -777,6 +786,11 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
 
 		if (likely(next_blocknum != InvalidBlockNumber))
 		{
+			int			flags = stream->read_buffers_flags;
+
+			if (stream->advice_enabled)
+				flags |= READ_BUFFERS_ISSUE_ADVICE;
+
 			/*
 			 * Pin a buffer for the next call.  Same buffer entry, and
 			 * arbitrary I/O entry (they're all free).  We don't have to
@@ -792,8 +806,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
 			if (likely(!StartReadBuffer(&stream->ios[0].op,
 										&stream->buffers[oldest_buffer_index],
 										next_blocknum,
-										stream->advice_enabled ?
-										READ_BUFFERS_ISSUE_ADVICE : 0)))
+										flags)))
 			{
 				/* Fast return. */
 				return buffer;
-- 
2.48.1.76.g4e746b1a31.dirty


--m2na7lgr3zfazgom
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v2.14-0015-read_stream-Introduce-and-use-optional-batchmo.patch"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

end of thread, other threads:[~2025-03-18 18:40 UTC | newest]

Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 04:34 [PATCH] Fix timeline-tracking failure while sending a historic timeline Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
2025-03-14 15:39 [PATCH v2.8 26/38] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-15 16:29 [PATCH v2.9 18/30] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-18 18:40 [PATCH v2.10 12/28] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-18 18:40 [PATCH v2.11 13/27] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-18 18:40 [PATCH v2.12 10/28] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-18 18:40 [PATCH v2.13 13/28] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>
2025-03-18 18:40 [PATCH v2.14 14/29] aio: Basic read_stream adjustments for real AIO Andres Freund <andres@anarazel.de>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox