On Thu, Apr 3, 2025 at 10:44 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
>
> On 2025-Apr-03, Andres Freund wrote:
>
> > I've increased the timeout even further, but I can't say that I am happy
> > about
> > the slowest test getting even slower. Adding test time in the serially
> > slowest
> > test is way worse than adding the same time in a concurrent test.
>
> Yeah. We discussed strategies to shorten the runtime, but the agreement
> upthread was that we'd look for more elaborate ways to do that
> afterwards. As I mentioned, I can see adding something like
> PG_TEST_EXCLUDE that we could use to suppress this test on slow hosts.
> Would that work for you?
>
> (We also discussed the fact that this was part of 002_pg_upgrade.pl
> instead of being elsewhere. The reason is that this depends on the
> regression tests having run, and this is the only TAP test that does
> that. Well, this one and 027_stream_regress.pl which is even slower.)
>
> > I suspect that the test will go a bit faster if log_statement weren't forced
> > on, printing that many log lines, with context, does make valgrind slower,
> > IME. But Cluster.pm forces it to on, and I suspect that putting a global
> > log_statement=false into TEMP_CONFIG would have it's own disadvantages.
>
> I'm sure we can make this change as well somehow, overridding the
> setting just 002_pg_upgrade.pl, as attached. I don't think it's
> relevant for this particular test. The log files go from 21 MB to
> 2.4 MB. It's not nothing ...
It doesn't show any time improvement on my laptop, but it may improve
valgrind timing. My valgrind setup is broken, trying to fix it and run
it. I have included this as 0002 in the attached patchset.
0001 is an attempt to reduce runtime of the test by not setting up a
cluster for restoring the database. Instead the test uses the upgraded
node as the target. This works well since we expect the old node and
new node to be running the same version and default install. The only
unpleasantness is 1. dump and restore phases are spatially and
temporally separated 2. The upgraded regression database needs to be
renamed to save its state for diagnosis, if required. But as a result
this saves 3 seconds on my laptop. Earlier we saw that the test added
9 seconds on my laptop and we gained back 3 seconds; doesn't seem bad.
It will show a significant difference in valgrind run.
--
Best Wishes,
Ashutosh Bapat
From ef7880e1f1774c6875f4b265287f70d844e20ca4 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Date: Fri, 4 Apr 2025 14:26:48 +0530
Subject: [PATCH 1/2] Reduce time taken by 002_pg_upgrade test to run
This test is one of the longest running tests and
172259afb563d35001410dc6daad78b250924038 makes it even longer by adding a test
to dump/restore roundtrip of regression database.
The test already creates two clusters for pg_upgrade test. When these clusters
have same version and do not use custom installation, we run dump/restore test.
The new upgraded cluster could be used as target of restore instead of creating
a new cluster.
But this separates the dump and restore phases of the test spatially and
temporaly since the dump needs to be taken while the old cluster is running and
it can be restored only after new upgraded cluster is running; in-between we run
upgrade test. This separation affects readability of the test and hence wasn't
attempted before. But since runtime of test seems to be more important, we take
a hit on readability. We have added additional comments so as to link the two
phases and improve readability.
Author: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Reported-by: Andres Freund <and...@anarazel.de>
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 72 ++++++++++++++++----------
1 file changed, 46 insertions(+), 26 deletions(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 311391d7acd..8e7ccd2dad0 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -359,30 +359,34 @@ if (defined($ENV{oldinstall}))
$oldnode->append_conf('postgresql.conf', 'autovacuum = off');
$oldnode->restart;
+# Dump/restore Test.
+#
# Test that dump/restore of the regression database roundtrips cleanly. This
# doesn't work well when the nodes are different versions, so skip it in that
# case. Note that this isn't a pg_upgrade test, but it's convenient to do it
# here because we've gone to the trouble of creating the regression database.
#
-# Do this while the old cluster is running before it is shut down by the
-# upgrade test but after turning its autovacuum off for stable statistics.
+# We execute this in two parts as follows:
+#
+# Part 1: Take dump from the old cluster while it is running before being shut
+# down by the upgrade test but after turning its autovacuum off for stable
+# statistics. If this part succeeds and is not skipped, it will leave behind
+# dump to be restored and a dump file for comparison.
+#
+# Part 2: The dump is restored on the upgraded cluster once it is running.
+#
+# Though this separates the two parts spatially and temporally, it avoids
+# creating a new cluster, thus saving time (and resources) in this already long
+# running test.
+my $regress_dump_file;
+my $src_dump;
SKIP:
{
- my $dstnode = PostgreSQL::Test::Cluster->new('dst_node');
-
skip "different Postgres versions"
- if ($oldnode->pg_version != $dstnode->pg_version);
+ if ($oldnode->pg_version != $newnode->pg_version);
skip "source node not using default install"
if (defined $oldnode->install_path);
- # Setup destination database cluster with the same configuration as the
- # source cluster to avoid any differences between dumps taken from both the
- # clusters caused by differences in their configurations.
- $dstnode->init(%old_node_params);
- # Stabilize stats for comparison.
- $dstnode->append_conf('postgresql.conf', 'autovacuum = off');
- $dstnode->start;
-
# Use --create in dump and restore commands so that the restored
# database has the same configurable variable settings as the original
# database so that the dumps taken from both databases taken do not
@@ -390,27 +394,18 @@ SKIP:
# coverage for --create option.
#
# Use directory format so that we can use parallel dump/restore.
- my $dump_file = "$tempdir/regression.dump";
+ $regress_dump_file = "$tempdir/regression.dump";
$oldnode->command_ok(
[
'pg_dump', '-Fd', '-j2', '--no-sync',
'-d' => $oldnode->connstr('regression'),
- '--create', '-f' => $dump_file
+ '--create', '-f' => $regress_dump_file
],
'pg_dump on source instance');
- $dstnode->command_ok(
- [ 'pg_restore', '--create', '-j2', '-d' => 'postgres', $dump_file ],
- 'pg_restore to destination instance');
-
- # Dump original and restored database for comparison.
- my $src_dump =
+ # Dump original database for comparison.
+ $src_dump =
get_dump_for_comparison($oldnode, 'regression', 'src_dump', 1);
- my $dst_dump =
- get_dump_for_comparison($dstnode, 'regression', 'dest_dump', 0);
-
- compare_files($src_dump, $dst_dump,
- 'dump outputs from original and restored regression databases match');
}
# Take a dump before performing the upgrade as a base comparison. Note
@@ -629,4 +624,29 @@ my $dump2_filtered = filter_dump(0, $oldnode->pg_version, $dump2_file);
compare_files($dump1_filtered, $dump2_filtered,
'old and new dumps match after pg_upgrade');
+# Execute Part 2 of Dump/restore Test.
+SKIP:
+{
+ # Skip Part 2 if the dump to be restored and the dump file for comparison do
+ # not exist. Part 1 was not executed or did not succeed.
+ skip "no dump/restore test"
+ if not defined $regress_dump_file or not defined $src_dump;
+
+ # Use --create option as explained in Part 1. Rename upgraded regression
+ # database so that pg_restore can succeed and the so that it's available for
+ # diagnosing problems if any.
+ $newnode->safe_psql('postgres', 'ALTER DATABASE regression RENAME TO
+ regression_upgraded');
+ $newnode->command_ok(
+ [ 'pg_restore', '--create', '-j2', '-d' => 'postgres', $regress_dump_file ],
+ 'pg_restore to destination instance');
+
+ # Dump restored database for comparison.
+ my $dst_dump =
+ get_dump_for_comparison($newnode, 'regression', 'dest_dump', 0);
+
+ compare_files($src_dump, $dst_dump,
+ 'dump outputs from original and restored regression databases match');
+}
+
done_testing();
base-commit: 7afca7edef751b8d7c0f5b6402ffcefc11c67fdd
--
2.34.1
From 262fcdab9568e447fade2fba886503aef27dcda7 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Date: Fri, 4 Apr 2025 16:28:20 +0530
Subject: [PATCH 2/2] Turn off log_statement to save CPU cycles
The test tests pg_dump and pg_upgrade utilities. Hence logging actual statements
executed on the server is not worth the CPU cycles spent for that. This
is significant in valgrind environment which slows tests down
considerably.
Author: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Reviewed-by: Ashutosh Bapat <ashutosh.bapat....@gmail.com>
Suggested-by: Andres Freund <and...@anarazel.de>
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 8e7ccd2dad0..96fc0b4dc73 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -221,6 +221,9 @@ push @old_initdb_params, '-k' if $oldnode->pg_version < 18;
$old_node_params{extra} = \@old_initdb_params;
$oldnode->init(%old_node_params);
+# Override log_statement set by Cluster.pm; logged statements are not as useful
+# and consume CPU cycles unnecessarily.
+$oldnode->append_conf('postgresql.conf', 'log_statement = none');
$oldnode->start;
my $result;
@@ -312,6 +315,8 @@ push @new_initdb_params, ('--encoding', 'SQL_ASCII');
push @new_initdb_params, ('--locale-provider', 'libc');
$new_node_params{extra} = \@new_initdb_params;
$newnode->init(%new_node_params);
+# Override log_statement set by Cluster.pm; as explained in case of oldnode.
+$newnode->append_conf('postgresql.conf', 'log_statement=none');
# Stabilize stats for comparison.
$newnode->append_conf('postgresql.conf', 'autovacuum = off');
--
2.34.1
