On Fri, Jun 16, 2023 at 8:00 AM Michael Paquier <mich...@paquier.xyz> wrote:
>
> On Thu, Jun 15, 2023 at 01:40:15PM +0900, Kyotaro Horiguchi wrote:
> > + "CREATE TABLE tt (); DROP TABLE tt; SELECT
> > pg_switch_wal();");
> >
> > At least since 11, we can utilize pg_logical_emit_message() for this
> > purpose. It's more lightweight and seems appropriate, not only because
> > it doesn't cause any side effects but also bacause we don't have to
> > worry about name conflicts.
>
> Making this as cheap as possible by design is a good concept for a
> common routine. +1.
While it seems reasonable to use pg_logical_emit_message, it won't
work for all the cases - what if someone wants to advance WAL by a few
WAL files? I think pg_switch_wal() is the way, no? For instance, the
replslot_limit.pl test increases the WAL in a very calculated way - it
increases by 5 WAL files. So, -1 to use pg_logical_emit_message.
I understand the naming conflicts for the table name used ('tt' in
this case). If the table name 'tt' looks so simple and easy for
someone to have tables with that name in their tests file, we can
generate a random table name in advance_wal, something like in the
attached v2 patch.
> > - SELECT 'finished';",
> > - timeout => $PostgreSQL::Test::Utils::timeout_default));
> > -is($result[1], 'finished', 'check if checkpoint command is not blocked');
> > -
> > +$node_primary2->advance_wal(1);
> > +$node_primary2->safe_psql('postgres', 'CHECKPOINT;');
> >
> > This test anticipates that the checkpoint could get blocked. Shouldn't
> > we keep the timeout?
>
> Indeed, this would partially invalidate what's getting tested in light
> of 1816a1c6 where we run a secondary command after the checkpoint. So
> the last SELECT should remain around.
Changed.
> > -$node_primary->safe_psql(
> > - 'postgres', "create table retain_test(a int);
> > - select pg_switch_wal();
> > - insert into retain_test values(1);
> > - checkpoint;");
> > +$node_primary->advance_wal(1);
> > +$node_primary->safe_psql('postgres', "checkpoint;");
> >
> > The original test generated some WAL after the segment switch, which
> > appears to be a significant characteristics of the test.
>
> Still it does not matter for this specific case? The logical slot has
> been already invalidated, so we don't care much about logical changes
> in WAL, do we?
Correct, the slot has already been invalidated and the test is
verifying that WAL isn't retained by the invalidated slot, so
essentially what it needs is to generate "some" wal. So, using
advance_wal there seems fine to me. CFBot doesn't complain anything -
https://github.com/BRupireddy/postgres/tree/add_a_TAP_test_function_to_generate_WAL_v2.
Attached the v2 patch. Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 959bd5c7dd00be05c98ab5a68406ac37af61a52f Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Wed, 19 Jul 2023 09:58:11 +0000
Subject: [PATCH v2] Add a TAP test function to generate WAL
This commit adds a perl function in Cluster.pm to generate WAL.
Some TAP tests are now using their own way to generate WAL.
Generalizing this functionality enables multiple TAP tests to
reuse the functionality.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 24 ++++++++++
src/test/recovery/t/001_stream_rep.pl | 6 +--
src/test/recovery/t/019_replslot_limit.pl | 48 +++++--------------
.../t/035_standby_logical_decoding.pl | 7 +--
4 files changed, 40 insertions(+), 45 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 5e161dbee6..583c63f3db 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -3120,6 +3120,30 @@ sub create_logical_slot_on_standby
=pod
+=item $node->advance_wal($n)
+
+Advance WAL of given node by $n segments
+
+=cut
+
+sub advance_wal
+{
+ my ($self, $n) = @_;
+
+ # Generate a random table name.
+ my $table_name = $self->safe_psql('postgres',
+ "SELECT substr(md5(random()::text), 1, 10)::text;");
+
+ # Advance by $n segments (= (wal_segment_size * $n) bytes).
+ for (my $i = 0; $i < $n; $i++)
+ {
+ $self->safe_psql('postgres',
+ qq{CREATE TABLE "$table_name"(); DROP TABLE "$table_name"; SELECT pg_switch_wal();});
+ }
+}
+
+=pod
+
=back
=cut
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 0c72ba0944..0e256dab8d 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -496,11 +496,7 @@ $node_primary->safe_psql('postgres',
my $segment_removed = $node_primary->safe_psql('postgres',
'SELECT pg_walfile_name(pg_current_wal_lsn())');
chomp($segment_removed);
-$node_primary->psql(
- 'postgres', "
- CREATE TABLE tab_phys_slot (a int);
- INSERT INTO tab_phys_slot VALUES (generate_series(1,10));
- SELECT pg_switch_wal();");
+$node_primary->advance_wal(1);
my $current_lsn =
$node_primary->safe_psql('postgres', "SELECT pg_current_wal_lsn();");
chomp($current_lsn);
diff --git a/src/test/recovery/t/019_replslot_limit.pl b/src/test/recovery/t/019_replslot_limit.pl
index 33e50ad933..a312155250 100644
--- a/src/test/recovery/t/019_replslot_limit.pl
+++ b/src/test/recovery/t/019_replslot_limit.pl
@@ -59,7 +59,7 @@ $result = $node_primary->safe_psql('postgres',
is($result, "reserved|t", 'check the catching-up state');
# Advance WAL by five segments (= 5MB) on primary
-advance_wal($node_primary, 1);
+$node_primary->advance_wal(1);
$node_primary->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when fitting max_wal_size
@@ -69,7 +69,7 @@ $result = $node_primary->safe_psql('postgres',
is($result, "reserved|t",
'check that it is safe if WAL fits in max_wal_size');
-advance_wal($node_primary, 4);
+$node_primary->advance_wal(4);
$node_primary->safe_psql('postgres', "CHECKPOINT;");
# The slot is always "safe" when max_slot_wal_keep_size is not set
@@ -100,7 +100,7 @@ $result = $node_primary->safe_psql('postgres',
is($result, "reserved", 'check that max_slot_wal_keep_size is working');
# Advance WAL again then checkpoint, reducing remain by 2 MB.
-advance_wal($node_primary, 2);
+$node_primary->advance_wal(2);
$node_primary->safe_psql('postgres', "CHECKPOINT;");
# The slot is still working
@@ -118,7 +118,7 @@ $node_standby->stop;
$result = $node_primary->safe_psql('postgres',
"ALTER SYSTEM SET wal_keep_size to '8MB'; SELECT pg_reload_conf();");
# Advance WAL again then checkpoint, reducing remain by 6 MB.
-advance_wal($node_primary, 6);
+$node_primary->advance_wal(6);
$result = $node_primary->safe_psql('postgres',
"SELECT wal_status as remain FROM pg_replication_slots WHERE slot_name = 'rep1'"
);
@@ -134,7 +134,7 @@ $node_primary->wait_for_catchup($node_standby);
$node_standby->stop;
# Advance WAL again without checkpoint, reducing remain by 6 MB.
-advance_wal($node_primary, 6);
+$node_primary->advance_wal(6);
# Slot gets into 'reserved' state
$result = $node_primary->safe_psql('postgres',
@@ -145,7 +145,7 @@ is($result, "extended", 'check that the slot state changes to "extended"');
$node_primary->safe_psql('postgres', "CHECKPOINT;");
# Advance WAL again without checkpoint; remain goes to 0.
-advance_wal($node_primary, 1);
+$node_primary->advance_wal(1);
# Slot gets into 'unreserved' state and safe_wal_size is negative
$result = $node_primary->safe_psql('postgres',
@@ -174,7 +174,7 @@ $node_primary->safe_psql('postgres',
# Advance WAL again. The slot loses the oldest segment by the next checkpoint
my $logstart = get_log_size($node_primary);
-advance_wal($node_primary, 7);
+$node_primary->advance_wal(7);
# Now create another checkpoint and wait until the WARNING is issued
$node_primary->safe_psql('postgres',
@@ -275,18 +275,11 @@ $node_standby->init_from_backup($node_primary2, $backup_name,
has_streaming => 1);
$node_standby->append_conf('postgresql.conf', "primary_slot_name = 'rep1'");
$node_standby->start;
-my @result =
- split(
- '\n',
- $node_primary2->safe_psql(
- 'postgres',
- "CREATE TABLE tt();
- DROP TABLE tt;
- SELECT pg_switch_wal();
- CHECKPOINT;
- SELECT 'finished';",
- timeout => $PostgreSQL::Test::Utils::timeout_default));
-is($result[1], 'finished', 'check if checkpoint command is not blocked');
+$node_primary2->advance_wal(1);
+$result = $node_primary2->safe_psql('postgres',
+ "CHECKPOINT; SELECT 'finished';",
+ timeout => $PostgreSQL::Test::Utils::timeout_default);
+is($result, 'finished', 'check if checkpoint command is not blocked');
$node_primary2->stop;
$node_standby->stop;
@@ -372,7 +365,7 @@ $logstart = get_log_size($node_primary3);
# freeze walsender and walreceiver. Slot will still be active, but walreceiver
# won't get anything anymore.
kill 'STOP', $senderpid, $receiverpid;
-advance_wal($node_primary3, 2);
+$node_primary3->advance_wal(2);
my $msg_logged = 0;
my $max_attempts = $PostgreSQL::Test::Utils::timeout_default;
@@ -418,21 +411,6 @@ kill 'CONT', $receiverpid;
$node_primary3->stop;
$node_standby3->stop;
-#####################################
-# Advance WAL of $node by $n segments
-sub advance_wal
-{
- my ($node, $n) = @_;
-
- # Advance by $n segments (= (wal_segment_size * $n) bytes) on primary.
- for (my $i = 0; $i < $n; $i++)
- {
- $node->safe_psql('postgres',
- "CREATE TABLE t (); DROP TABLE t; SELECT pg_switch_wal();");
- }
- return;
-}
-
# return the size of logfile of $node in bytes
sub get_log_size
{
diff --git a/src/test/recovery/t/035_standby_logical_decoding.pl b/src/test/recovery/t/035_standby_logical_decoding.pl
index 480e6d6caa..2d19adab4e 100644
--- a/src/test/recovery/t/035_standby_logical_decoding.pl
+++ b/src/test/recovery/t/035_standby_logical_decoding.pl
@@ -524,11 +524,8 @@ my $walfile_name = $node_primary->safe_psql('postgres',
chomp($walfile_name);
# Generate some activity and switch WAL file on the primary
-$node_primary->safe_psql(
- 'postgres', "create table retain_test(a int);
- select pg_switch_wal();
- insert into retain_test values(1);
- checkpoint;");
+$node_primary->advance_wal(1);
+$node_primary->safe_psql('postgres', "checkpoint;");
# Wait for the standby to catch up
$node_primary->wait_for_replay_catchup($node_standby);
--
2.34.1
