From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 bharath.rupireddyforpostgres@gmail.com, pgsql-hackers@lists.postgresql.org,
 sawada.mshk@gmail.com
Subject: Re: pg_receivewal starting position
Date: Thu, 28 Oct 2021 15:55:12 +0200
Message-ID: <3489026.hdfAi7Kttb@aivenronan>
Organization: aiven
In-Reply-To: <YXqYKAdVEqmyTltK@paquier.xyz>
References: <YXdWvIO52CiT8kg9@paquier.xyz> <12900687.dW097sEU6C@aivenronan>
 <YXqYKAdVEqmyTltK@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nextPart1928009.8hb0ThOEGa"
Content-Transfer-Encoding: 7Bit
Archived-At: 
 <https://www.postgresql.org/message-id/3489026.hdfAi7Kttb%40aivenronan>
Precedence: bulk

This is a multi-part message in MIME format.

--nextPart1928009.8hb0ThOEGa
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"

Le jeudi 28 octobre 2021, 14:31:36 CEST Michael Paquier a =E9crit :
> On Wed, Oct 27, 2021 at 10:11:00AM +0200, Ronan Dunklau wrote:
> > Sorry I sent an intermediary version of the patch, here is the correct
> > one.
>=20
> While looking at this patch, I have figured out a simple way to make
> the tests faster without impacting the coverage.  The size of the WAL
> segments archived is a bit of a bottleneck as they need to be zeroed
> by pg_receivewal at creation.  This finishes by being a waste of time,
> even if we don't flush the data.  So I'd like to switch the test so as
> we use a segment size of 1MB, first.
>=20
> A second thing is that we copy too many segments than really needed
> when using the slot's restart_lsn as starting point as RESERVE_WAL
> would use the current redo location, so it seems to me that a
> checkpoint is in order before the slot creation.  A third thing is
> that generating some extra data after the end LSN we want to use makes
> the test much faster at the end.
>=20
> With those three methods combined, the test goes down from 11s to 9s
> here.  Attached is a patch I'd like to apply to make the test
> cheaper.

Interesting ideas, thanks. For the record, the time drops from ~4.5s to 3s =
on=20
average on my machine.=20
I think if you reduce the size of the generate_series batches, this should=
=20
probably be reduced everywhere. With what we do though, inserting a single=
=20
line should work just as well, I wonder why we insist on inserting a hundre=
d=20
lines ? I updated your patch with that small modification, it also makes th=
e=20
code less verbose.


>=20
> I also had a look at your patch.  Here are some comments.
>=20
> +# Cleanup the previous stream directories to reuse them
> +unlink glob "'${stream_dir}/*'";
> +unlink glob "'${slot_dir}/*'";
> I think that we'd better choose a different location for the
> archives.  Keeping the segments of the previous tests is useful for
> debugging if a previous step of the test failed.

Ok.
>=20
> +$standby->psql('',
> +  "CREATE_REPLICATION_SLOT $folder_slot PHYSICAL (RESERVE_WAL)",
> +  replication =3D> 1);
> Here as well we could use a restart point to reduce the number of
> segments archived.

The restart point should be very close, as we don't generate any activity o=
n=20
the primary between the backup and the slot's creation. I'm not sure adding=
=20
the complexity of triggering a checkpoint on the primary and waiting for th=
e=20
standby to catch up on it would be that useful.=20
>=20
> +# Now, try to resume after the promotion, from the folder.
> +$standby->command_ok(
> +   [ 'pg_receivewal', '-D', $stream_dir, '--verbose', '--endpos', $nextl=
sn,
> +    '--slot', $folder_slot, '--no-sync'],
> +   "Stream some wal after promoting, resuming from the folder's position=
");
> What is called the "resume-from-folder" test in the patch is IMO
> too costly and brings little extra value, requiring two commands of
> pg_receivewal (one to populate the folder and one to test the TLI jump
> from the previously-populated point) to test basically the same thing
> as when the starting point is taken from the slot.  Except that
> restarting from the slot is twice cheaper.  The point of the
> resume-from-folder case is to make sure that we restart from the point
> of the archive folder rather than the slot's restart_lsn, but your
> test fails this part, in fact, because the first command populating
> the archive folder also uses "--slot $folder_slot", updating the
> slot's restart_lsn before the second pg_receivewal uses this slot
> again.
>=20
> I think, as a whole, that testing for the case where an archive folder
> is populated (without a slot!), followed by a second command where we
> use a slot that has a restart_lsn older than the archive's location,
> to not be that interesting.  If we include such a test, there is no
> need to include that within the TLI jump part, in my opinion.  So I
> think that we had better drop this part of the patch, and keep only
> the case where we resume from a slot for the TLI jump.

You're right about the test not being that interesting in that case. I thou=
ght=20
it would be worthwhile to test both cases, but resume_from_folder doesn't=20
actually exercise any code that was not already called before (timeline=20
computation from segment name).

> The commands of pg_receivewal included in the test had better use -n
> so as there is no infinite loop on failure.

Ok.

=2D-=20
Ronan Dunklau
--nextPart1928009.8hb0ThOEGa
Content-Disposition: attachment;
 filename="v16-0002-Speed-up-pg_receivewal-tests.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8";
 name="v16-0002-Speed-up-pg_receivewal-tests.patch"

From ad09605b6f8b2d1cc91e67a255bf469363cad2b3 Mon Sep 17 00:00:00 2001
From: Ronan Dunklau <ronan.dunklau@aiven.io>
Date: Thu, 28 Oct 2021 15:12:04 +0200
Subject: [PATCH v16 2/2] Speed up pg_receivewal tests.

Author: Michael Paquier
---
 src/bin/pg_basebackup/t/020_pg_receivewal.pl | 21 ++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
index e0422e2242..1691054f5c 100644
--- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl
+++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
@@ -15,7 +15,7 @@ program_options_handling_ok('pg_receivewal');
 umask(0077);
 
 my $primary = PostgreSQL::Test::Cluster->new('primary');
-$primary->init(allows_streaming => 1);
+$primary->init(allows_streaming => 1, extra => ['--wal-segsize=1']);
 $primary->start;
 
 my $stream_dir = $primary->basedir . '/archive_wal';
@@ -55,8 +55,7 @@ $primary->psql('postgres', 'SELECT pg_switch_wal();');
 my $nextlsn =
   $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
 chomp($nextlsn);
-$primary->psql('postgres',
-	'INSERT INTO test_table VALUES (generate_series(1,100));');
+$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
 
 # Stream up to the given position.  This is necessary to have a fixed
 # started point for the next commands done in this test, with or without
@@ -85,8 +84,7 @@ SKIP:
 	$nextlsn =
 	  $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
 	chomp($nextlsn);
-	$primary->psql('postgres',
-		'INSERT INTO test_table VALUES (generate_series(100,200));');
+	$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
 
 	# Note the trailing whitespace after the value of --compress, that is
 	# a valid value.
@@ -136,8 +134,7 @@ $primary->psql('postgres', 'SELECT pg_switch_wal();');
 $nextlsn =
   $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
 chomp($nextlsn);
-$primary->psql('postgres',
-	'INSERT INTO test_table VALUES (generate_series(200,300));');
+$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
 $primary->command_ok(
 	[
 		'pg_receivewal', '-D',     $stream_dir, '--verbose',
@@ -167,7 +164,9 @@ mkdir($slot_dir);
 $slot_name = 'archive_slot';
 
 # Setup the slot, reserving WAL at creation (corresponding to the
-# last redo LSN here, actually).
+# last redo LSN here, actually, so use a checkpoint to reduce the
+# number of segments archived).
+$primary->psql('postgres', 'checkpoint;');
 $primary->psql('postgres',
 	"SELECT pg_create_physical_replication_slot('$slot_name', true);");
 
@@ -181,13 +180,15 @@ my $walfile_streamed = $primary->safe_psql(
 
 # Switch to a new segment, to make sure that the segment retained by the
 # slot is still streamed.  This may not be necessary, but play it safe.
-$primary->psql('postgres',
-	'INSERT INTO test_table VALUES (generate_series(1,100));');
+$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
 $primary->psql('postgres', 'SELECT pg_switch_wal();');
 $nextlsn =
   $primary->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
 chomp($nextlsn);
 
+# Add a bit more data to accelerate the end of the next commands.
+$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
+
 # Check case where the slot does not exist.
 $primary->command_fails_like(
 	[
-- 
2.33.1


--nextPart1928009.8hb0ThOEGa
Content-Disposition: attachment;
 filename="v16-0001-Add-a-test-for-pg_receivewal-following-timeline-.patch"
Content-Transfer-Encoding: 7Bit
Content-Type: text/x-patch; charset="UTF-8";
 name="v16-0001-Add-a-test-for-pg_receivewal-following-timeline-.patch"

From 957177881692b772a1415316bcf0babcc7958e09 Mon Sep 17 00:00:00 2001
From: Ronan Dunklau <ronan.dunklau@aiven.io>
Date: Tue, 26 Oct 2021 10:54:12 +0200
Subject: [PATCH v16 1/2] Add a test for pg_receivewal following timeline
 switch.

pg_receivewal is able to follow a timeline switch, but this was not
tested anywher. This test case verify that it works as expected both
when resuming from a replication slot and from the archive directory,
which have different methods of retrieving the timeline it left off.
---
 src/bin/pg_basebackup/t/020_pg_receivewal.pl | 58 +++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/src/bin/pg_basebackup/t/020_pg_receivewal.pl b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
index 092c9b6f25..e0422e2242 100644
--- a/src/bin/pg_basebackup/t/020_pg_receivewal.pl
+++ b/src/bin/pg_basebackup/t/020_pg_receivewal.pl
@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use PostgreSQL::Test::Utils;
 use PostgreSQL::Test::Cluster;
-use Test::More tests => 31;
+use Test::More tests => 35;
 
 program_help_ok('pg_receivewal');
 program_version_ok('pg_receivewal');
@@ -206,3 +206,59 @@ $primary->command_ok(
 	"WAL streamed from the slot's restart_lsn");
 ok(-e "$slot_dir/$walfile_streamed",
 	"WAL from the slot's restart_lsn has been archived");
+
+# Test a timeline switch.
+# This test is split in two, using the same standby: one test check the
+# resume-from-folder case, the other the resume-from-slot one.
+
+# Setup a standby for our tests
+my $backup_name = "basebackup";
+$primary->backup($backup_name);
+my $standby = PostgreSQL::Test::Cluster->new("standby");
+$standby->init_from_backup($primary, $backup_name, has_streaming => 1);
+$standby->start;
+
+# Create a replication slot on this new standby
+my $archive_slot = "archive_slot";
+$standby->psql('',
+  "CREATE_REPLICATION_SLOT $archive_slot PHYSICAL (RESERVE_WAL)",
+  replication => 1);
+# Get a walfilename from before the promotion to make sure it is archived
+# after promotion
+my $walfile_before_promotion = $primary->safe_psql('postgres',
+  "SELECT pg_walfile_name(pg_current_wal_insert_lsn())");
+# Switch wal to make sure it is not a partial file but a complete segment.
+$primary->psql('postgres', 'INSERT INTO test_table VALUES (1);');
+$primary->psql('postgres', 'SELECT pg_switch_wal();');
+$primary->wait_for_catchup($standby, 'replay', $primary->lsn('write'));
+
+# Everything is setup, promote the standby to trigger a timeline switch
+$standby->psql(
+  'postgres',
+  "SELECT pg_promote(wait_seconds => 300)");
+
+# Force a wal switch to make sure at least one full WAL is archived on the new
+# timeline, and fetch this walfilename.
+my $walfile_after_promotion = $standby->safe_psql('postgres',
+  "SELECT pg_walfile_name(pg_current_wal_insert_lsn())");
+$standby->psql('postgres', 'INSERT INTO test_table VALUES (1);');
+$standby->psql('postgres', 'SELECT pg_switch_wal();');
+$nextlsn =
+  $standby->safe_psql('postgres', 'SELECT pg_current_wal_insert_lsn();');
+chomp($nextlsn);
+$standby->psql('postgres', 'INSERT INTO test_table VALUES (1);');
+
+# Now try to resume from the slot after the promotion.
+my $timeline_dir = $primary->basedir . '/timeline_wal';
+mkdir($timeline_dir);
+
+$standby->command_ok(
+	[ 'pg_receivewal', '-D', $timeline_dir, '--verbose', '--endpos', $nextlsn,
+    '--slot', $archive_slot, '--no-sync', '-n'],
+	"Stream some wal after promoting, resuming from the slot's position");
+ok(-e "$timeline_dir/$walfile_before_promotion",
+  "WAL from the old timeline has been archived resuming from the slot");
+ok(-e "$timeline_dir/$walfile_after_promotion",
+  "WAL from the new timeline has been archived resuming from the slot");
+ok(-e "$timeline_dir/00000002.history",
+  "Timeline history file has been archived resuming from the slot");
-- 
2.33.1


--nextPart1928009.8hb0ThOEGa--