From 44b6210deb73212a6b66aa2cd6696730b8a16894 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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 Reported-by: Andres Freund --- 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 311391d7acd6..8e7ccd2dad0c 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 @@ sub get_dump_for_comparison $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 @@ sub get_dump_for_comparison # 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 @@ sub get_dump_for_comparison 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(); From a341700a9ee4b3bf43d333e2ae5bb25cb25cecfb Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat 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 Reviewed-by: Ashutosh Bapat Suggested-by: Andres Freund --- 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 8e7ccd2dad0c..96fc0b4dc734 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 @@ sub get_dump_for_comparison $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 @@ sub get_dump_for_comparison 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');