Thread: Re: multithreaded zstd backup compression for client and server
Hi Robert,
I haven't reviewed the meat of the patch in detail, but I noticed
something in the tests:
Robert Haas <robertmhaas@gmail.com> writes:
> diff --git a/src/bin/pg_verifybackup/t/009_extract.pl b/src/bin/pg_verifybackup/t/009_extract.pl
> index 9f9cc7540b..e17e7cad51 100644
> --- a/src/bin/pg_verifybackup/t/009_extract.pl
> +++ b/src/bin/pg_verifybackup/t/009_extract.pl
[…]
> + if ($backup_stdout ne '')
> + {
> + print "# standard output was:\n$backup_stdout";
> + }
> + if ($backup_stderr ne '')
> + {
> + print "# standard error was:\n$backup_stderr";
> + }
[…]
> diff --git a/src/bin/pg_verifybackup/t/010_client_untar.pl b/src/bin/pg_verifybackup/t/010_client_untar.pl
> index 487e30e826..5f6a4b9963 100644
> --- a/src/bin/pg_verifybackup/t/010_client_untar.pl
> +++ b/src/bin/pg_verifybackup/t/010_client_untar.pl
[…]
> + if ($backup_stdout ne '')
> + {
> + print "# standard output was:\n$backup_stdout";
> + }
> + if ($backup_stderr ne '')
> + {
> + print "# standard error was:\n$backup_stderr";
> + }
Per the TAP protocol, every line of non-test-result output should be
prefixed by "# ". The note() function does this for you, see
https://metacpan.org/pod/Test::More#Diagnostics for details.
- ilmari
On Thu, Mar 24, 2022 at 9:19 AM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Per the TAP protocol, every line of non-test-result output should be > prefixed by "# ". The note() function does this for you, see > https://metacpan.org/pod/Test::More#Diagnostics for details. True, but that also means it shows up in the actual failure message, which seems too verbose. By just using 'print', it ends up in the log file if it's needed, but not anywhere else. Maybe there's a better way to do this, but I don't think using note() is what I want. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Mar 24, 2022 at 9:19 AM Dagfinn Ilmari Mannsåker > <ilmari@ilmari.org> wrote: >> Per the TAP protocol, every line of non-test-result output should be >> prefixed by "# ". The note() function does this for you, see >> https://metacpan.org/pod/Test::More#Diagnostics for details. > > True, but that also means it shows up in the actual failure message, > which seems too verbose. By just using 'print', it ends up in the log > file if it's needed, but not anywhere else. Maybe there's a better way > to do this, but I don't think using note() is what I want. That is the difference between note() and diag(): note() prints to stdout so is not visible under a non-verbose prove run, while diag() prints to stderr so it's always visible. - ilmari
On Mon, Mar 28, 2022 at 12:52 PM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > > True, but that also means it shows up in the actual failure message, > > which seems too verbose. By just using 'print', it ends up in the log > > file if it's needed, but not anywhere else. Maybe there's a better way > > to do this, but I don't think using note() is what I want. > > That is the difference between note() and diag(): note() prints to > stdout so is not visible under a non-verbose prove run, while diag() > prints to stderr so it's always visible. OK, but print doesn't do either of those things. The output only shows up in the log file, even with --verbose. Here's an example of what the log file looks like: # Running: pg_verifybackup -n -m /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/server-backup/backup_manifest -e /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/extracted-backup backup successfully verified ok 6 - verify backup, compression gzip As you can see, there is a line here that does not begin with #. That line is the standard output of a command that was run by the test script. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
> This patch contains a trivial adjustment to
> PostgreSQL::Test::Cluster::run_log to make it return a useful value
> instead of not. I think that should be pulled out and committed
> independently regardless of what happens to this patch overall, and
> possibly back-patched.
run_log() is far from the only such method in PostgreSQL::Test::Cluster.
Here's a patch that gives the same treatment to all the methods that
just pass through to the corresponding PostgreSQL::Test::Utils function.
Also attached is a fix a typo in the _get_env doc comment that I noticed
while auditing the return values.
- ilmari
From 2e6ccdb2148128357e26816776a448a0ef95a1c6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 30 Mar 2022 02:56:51 +0100
Subject: [PATCH] Make more PostgreSQL:Test::Cluster methods return a useful
value
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Commit ad4f2c47de440cdd5d58cf9ffea09afa0da04d6c made run_log() return
the value of the corresponding PostgreSQL::Test::Utils function, but
missed out a lot of other ones. This makes all the methods that call
a corresponding function in ::Utils pass on the underlying function's
return value so they too can be used in the idiomatic fashion of
$node->some_test(…) or diag(…);
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index b6e3351611..c56a7e6c3b 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -2376,8 +2376,7 @@ sub command_ok
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::command_ok(@_);
- return;
+ return PostgreSQL::Test::Utils::command_ok(@_);
}
=pod
@@ -2396,8 +2395,7 @@ sub command_fails
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::command_fails(@_);
- return;
+ return PostgreSQL::Test::Utils::command_fails(@_);
}
=pod
@@ -2416,8 +2414,7 @@ sub command_like
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::command_like(@_);
- return;
+ return PostgreSQL::Test::Utils::command_like(@_);
}
=pod
@@ -2436,8 +2433,7 @@ sub command_fails_like
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::command_fails_like(@_);
- return;
+ return PostgreSQL::Test::Utils::command_fails_like(@_);
}
=pod
@@ -2457,8 +2453,7 @@ sub command_checks_all
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::command_checks_all(@_);
- return;
+ return PostgreSQL::Test::Utils::command_checks_all(@_);
}
=pod
@@ -2483,8 +2478,7 @@ sub issues_sql_like
my $result = PostgreSQL::Test::Utils::run_log($cmd);
ok($result, "@$cmd exit code 0");
my $log = PostgreSQL::Test::Utils::slurp_file($self->logfile, $log_location);
- like($log, $expected_sql, "$test_name: SQL found in server log");
- return;
+ return like($log, $expected_sql, "$test_name: SQL found in server log");
}
=pod
--
2.30.2
From 24423ca6a9cc69adb6d0a08554d94dac25db6d27 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 30 Mar 2022 12:58:25 +0100
Subject: [PATCH 2/2] Fix typo in PostgreSQL::Test::Cluster::_get_env docs
It had the wrong opening brackend on the method call.
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index c56a7e6c3b..b98bff278a 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1368,7 +1368,7 @@ sub _set_pg_version
#
# Routines that call Postgres binaries need to call this routine like this:
#
-# local %ENV = $self->_get_env{[%extra_settings]);
+# local %ENV = $self->_get_env([%extra_settings]);
#
# A copy of the environment is taken and node's host and port settings are
# added as PGHOST and PGPORT, then the extra settings (if any) are applied.
--
2.30.2
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Mar 28, 2022 at 12:52 PM Dagfinn Ilmari Mannsåker > <ilmari@ilmari.org> wrote: >> > True, but that also means it shows up in the actual failure message, >> > which seems too verbose. By just using 'print', it ends up in the log >> > file if it's needed, but not anywhere else. Maybe there's a better way >> > to do this, but I don't think using note() is what I want. >> >> That is the difference between note() and diag(): note() prints to >> stdout so is not visible under a non-verbose prove run, while diag() >> prints to stderr so it's always visible. > > OK, but print doesn't do either of those things. The output only shows > up in the log file, even with --verbose. Here's an example of what the > log file looks like: > > # Running: pg_verifybackup -n -m > /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/server-backup/backup_manifest > -e /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/extracted-backup > backup successfully verified > ok 6 - verify backup, compression gzip > > As you can see, there is a line here that does not begin with #. That > line is the standard output of a command that was run by the test > script. Oh, that must be some non-standard output handling that our test setup does. Plain `prove` shows everything on stdout and stderr in verbose mode, and only stderr in non-vebose mode: $ cat verbosity.t use strict; use warnings; use Test::More; pass "pass"; diag "diag"; note "note"; print "print\n"; system qw(echo system); done_testing; $ prove verbosity.t verbosity.t .. 1/? # diag verbosity.t .. ok All tests successful. Files=1, Tests=1, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.04 cusr 0.01 csys = 0.07 CPU) Result: PASS $ prove -v verbosity.t verbosity.t .. ok 1 - pass # diag # note print system 1..1 ok All tests successful. Files=1, Tests=1, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.06 cusr 0.00 csys = 0.08 CPU) Result: PASS - ilmari
On 3/30/22 08:06, Dagfinn Ilmari Mannsåker wrote: > Robert Haas <robertmhaas@gmail.com> writes: > >> On Mon, Mar 28, 2022 at 12:52 PM Dagfinn Ilmari Mannsåker >> <ilmari@ilmari.org> wrote: >>>> True, but that also means it shows up in the actual failure message, >>>> which seems too verbose. By just using 'print', it ends up in the log >>>> file if it's needed, but not anywhere else. Maybe there's a better way >>>> to do this, but I don't think using note() is what I want. >>> That is the difference between note() and diag(): note() prints to >>> stdout so is not visible under a non-verbose prove run, while diag() >>> prints to stderr so it's always visible. >> OK, but print doesn't do either of those things. The output only shows >> up in the log file, even with --verbose. Here's an example of what the >> log file looks like: >> >> # Running: pg_verifybackup -n -m >> /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/server-backup/backup_manifest >> -e /Users/rhaas/pgsql/src/bin/pg_verifybackup/tmp_check/t_008_untar_primary_data/backup/extracted-backup >> backup successfully verified >> ok 6 - verify backup, compression gzip >> >> As you can see, there is a line here that does not begin with #. That >> line is the standard output of a command that was run by the test >> script. > Oh, that must be some non-standard output handling that our test setup > does. Plain `prove` shows everything on stdout and stderr in verbose > mode, and only stderr in non-vebose mode: > Yes, PostgreSQL::Test::Utils hijacks STDOUT and STDERR (see the INIT block). cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Wed, Mar 30, 2022 at 8:00 AM Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > This patch contains a trivial adjustment to > > PostgreSQL::Test::Cluster::run_log to make it return a useful value > > instead of not. I think that should be pulled out and committed > > independently regardless of what happens to this patch overall, and > > possibly back-patched. > > run_log() is far from the only such method in PostgreSQL::Test::Cluster. > Here's a patch that gives the same treatment to all the methods that > just pass through to the corresponding PostgreSQL::Test::Utils function. > > Also attached is a fix a typo in the _get_env doc comment that I noticed > while auditing the return values. I suggest posting these patches on a new thread with a subject line that matches what the patches do, and adding it to the next CommitFest. It seems like a reasonable thing to do on first glance, but I wouldn't want to commit it without going through and figuring out whether there's any risk of anything breaking, and it doesn't seem like there's a strong need to do it in v15 rather than v16. -- Robert Haas EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes: > On Wed, Mar 30, 2022 at 8:00 AM Dagfinn Ilmari Mannsåker > <ilmari@ilmari.org> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >> > This patch contains a trivial adjustment to >> > PostgreSQL::Test::Cluster::run_log to make it return a useful value >> > instead of not. I think that should be pulled out and committed >> > independently regardless of what happens to this patch overall, and >> > possibly back-patched. >> >> run_log() is far from the only such method in PostgreSQL::Test::Cluster. >> Here's a patch that gives the same treatment to all the methods that >> just pass through to the corresponding PostgreSQL::Test::Utils function. >> >> Also attached is a fix a typo in the _get_env doc comment that I noticed >> while auditing the return values. > > I suggest posting these patches on a new thread with a subject line > that matches what the patches do, and adding it to the next > CommitFest. Will do. > It seems like a reasonable thing to do on first glance, but I wouldn't > want to commit it without going through and figuring out whether > there's any risk of anything breaking, and it doesn't seem like > there's a strong need to do it in v15 rather than v16. Given that the methods don't currently have a useful return value (undef or the empty list, depending on context), I don't expect anything to be relying on it (and it passed check-world with --enable-tap-tests and all the --with-foo flags I could easily get to work), but I can grep the code as well to be extra sure. - ilmari
On 3/30/22 08:00, Dagfinn Ilmari Mannsåker wrote: > Robert Haas <robertmhaas@gmail.com> writes: > >> This patch contains a trivial adjustment to >> PostgreSQL::Test::Cluster::run_log to make it return a useful value >> instead of not. I think that should be pulled out and committed >> independently regardless of what happens to this patch overall, and >> possibly back-patched. > run_log() is far from the only such method in PostgreSQL::Test::Cluster. > Here's a patch that gives the same treatment to all the methods that > just pass through to the corresponding PostgreSQL::Test::Utils function. > > Also attached is a fix a typo in the _get_env doc comment that I noticed > while auditing the return values. > None of these routines in Utils.pm returns a useful value (unlike run_log()). Typically we don't return the value of Test::More routines. So -1 on patch 1. I will fix the typo. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com