Hi Michael,
Thanks for committing!
Michael Paquier <mich...@paquier.xyz> writes:
> On Tue, Jan 21, 2025 at 02:17:03PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Thanks again for reviewing this monster patch.
>
> +$node->command_checks_all(
> + [ 'pg_amcheck', '--exclude-user' => 'no_such_user', 'postgres' ],
>
> Extra error spotted here with s/--exclude-user/--username/.
I've attached a patch to make it check that it's actually complaining
about the role not existing, not an unrecognised option.
> The double --verbose for test 'run pg_createsubscriber on node S' is
> intentional, as mentioned by Euler. Fixed this one by making the two
> --verbose be side-by-side, and added a comment to document the
> intention.
Good call.
> Perhaps pg_resetwal/t/001_basic.pl should also use more long option
> names? Note that there are a few more under "Locale provider
> tests" in the initdb tests. I've left that as a follow-up thing,
> that was already a lot..
As I mentioned elsewhere in the thread, I left those alone since the
error message uses the short spelling even if the command had the long
one. We could add the long spelling to the preceding comments, like the
second attached patch.
> Found some more inconsistencies with the ordering of the options, like
> a few start commands with pg_ctl in the recovery tests. I've
> maintained some consistency here, even if your patch was OK.
Most invocations of pg_ctl actually have the action before any options:
❯ rg --multiline -tperl '(\W)pg_ctl\1,\s*(\W)\w+\2' | rg -cw pg_ctl
23
❯ rg --multiline -tperl '(\W)pg_ctl\1,\s*(\W)--\w+\2' | rg -cw pg_ctl
11
I've attached a third patch to make them consistently have the action
first.
- ilmari
>From e06f64ebfffa2724b9fa9072a9f792c80d285aa8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Wed, 22 Jan 2025 14:45:51 +0000
Subject: [PATCH 1/3] Check that pg_amcheck with a bad username actually
complains about that
---
src/bin/pg_amcheck/t/002_nonesuch.pl | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_amcheck/t/002_nonesuch.pl b/src/bin/pg_amcheck/t/002_nonesuch.pl
index f6011d000b7..2697f1c1b1a 100644
--- a/src/bin/pg_amcheck/t/002_nonesuch.pl
+++ b/src/bin/pg_amcheck/t/002_nonesuch.pl
@@ -85,7 +85,10 @@
# Failing to connect to the initial database due to bad username is an error.
$node->command_checks_all(
[ 'pg_amcheck', '--username' => 'no_such_user', 'postgres' ],
- 1, [qr/^$/], [], 'checking with a non-existent user');
+ 1,
+ [qr/^$/],
+ [qr/role "no_such_user" does not exist/],
+ 'checking with a non-existent user');
#########################################
# Test checking databases without amcheck installed
--
2.48.1
>From 257f1fcf0b5cde89048addba725628b8b040558f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Wed, 22 Jan 2025 15:25:05 +0000
Subject: [PATCH 2/3] pg_resetwal TAP test: more long options
Use --dry-run in the initial test, and include the long option in the
comment for the error cases.
The actual error tests are kept short, because the error message uses
the short spelling even though the invocation used the long one.
---
src/bin/pg_resetwal/t/001_basic.pl | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index 323cd483cf3..4e406dc9aa8 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -16,8 +16,8 @@
$node->init;
$node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
-command_like([ 'pg_resetwal', '-n', $node->data_dir ],
- qr/checkpoint/, 'pg_resetwal -n produces output');
+command_like([ 'pg_resetwal', '--dry-run', $node->data_dir ],
+ qr/checkpoint/, 'pg_resetwal --dry-run produces output');
# Permissions on PGDATA should be default
@@ -79,7 +79,7 @@
'fails with too few command-line arguments');
# error cases
-# -c
+# -c / --commit-timestamp-ids=xid,xid
command_fails_like(
[ 'pg_resetwal', '-c' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -c/,
@@ -96,7 +96,7 @@
[ 'pg_resetwal', '-c' => '10,1', $node->data_dir ],
qr/greater than/,
'fails with -c value 1 part 2');
-# -e
+# -e / --epoch=xid_epoch
command_fails_like(
[ 'pg_resetwal', '-e' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -e/,
@@ -105,12 +105,12 @@
[ 'pg_resetwal', '-e' => '-1', $node->data_dir ],
qr/must not be -1/,
'fails with -e value -1');
-# -l
+# -l / --next-wal-file=walfile
command_fails_like(
[ 'pg_resetwal', '-l' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -l/,
'fails with incorrect -l option');
-# -m
+# -m / --multixact-ids=mxid,mxid
command_fails_like(
[ 'pg_resetwal', '-m' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -m/,
@@ -127,7 +127,7 @@
[ 'pg_resetwal', '-m' => '10,0', $node->data_dir ],
qr/must not be 0/,
'fails with -m value 0 part 2');
-# -o
+# -o / --next-oid=oid
command_fails_like(
[ 'pg_resetwal', '-o' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -o/,
@@ -136,7 +136,7 @@
[ 'pg_resetwal', '-o' => '0', $node->data_dir ],
qr/must not be 0/,
'fails with -o value 0');
-# -O
+# -O / --multixact-offset=mxoff
command_fails_like(
[ 'pg_resetwal', '-O' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -O/,
@@ -154,7 +154,7 @@
[ 'pg_resetwal', '--wal-segsize' => '13', $node->data_dir ],
qr/must be a power/,
'fails with invalid --wal-segsize value');
-# -u
+# -u / --oldest-transaction-id=xid
command_fails_like(
[ 'pg_resetwal', '-u' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -u/,
@@ -163,7 +163,7 @@
[ 'pg_resetwal', '-u' => '1', $node->data_dir ],
qr/must be greater than/,
'fails with -u value too small');
-# -x
+# -x / --next-transaction-id=xid
command_fails_like(
[ 'pg_resetwal', '-x' => 'foo', $node->data_dir ],
qr/error: invalid argument for option -x/,
--
2.48.1
>From 67bd8fbf906111b7d172777bcea88d6653b35085 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilm...@ilmari.org>
Date: Wed, 22 Jan 2025 14:53:16 +0000
Subject: [PATCH 3/3] TAP tests: consistently specify the pg_ctl action before
options
Most pg_ctl invocations in the tests are this way, this brings the
rest into line.
---
src/bin/pg_ctl/t/002_status.pl | 4 ++--
src/bin/pg_ctl/t/003_promote.pl | 14 +++++++-------
src/test/recovery/t/003_recovery_targets.pl | 6 ++----
src/test/recovery/t/024_archive_recovery.pl | 3 +--
4 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/src/bin/pg_ctl/t/002_status.pl b/src/bin/pg_ctl/t/002_status.pl
index 346f6919ac6..ba8db118aa4 100644
--- a/src/bin/pg_ctl/t/002_status.pl
+++ b/src/bin/pg_ctl/t/002_status.pl
@@ -20,10 +20,10 @@
3, 'pg_ctl status with server not running');
system_or_bail(
- 'pg_ctl',
+ 'pg_ctl', 'start',
'--log' => "$tempdir/logfile",
'--pgdata' => $node->data_dir,
- '--wait', 'start');
+ '--wait');
command_exit_is([ 'pg_ctl', 'status', '--pgdata' => $node->data_dir ],
0, 'pg_ctl status with server running');
diff --git a/src/bin/pg_ctl/t/003_promote.pl b/src/bin/pg_ctl/t/003_promote.pl
index 43a9bbac2ac..20948a558cd 100644
--- a/src/bin/pg_ctl/t/003_promote.pl
+++ b/src/bin/pg_ctl/t/003_promote.pl
@@ -11,7 +11,7 @@
my $tempdir = PostgreSQL::Test::Utils::tempdir;
command_fails_like(
- [ 'pg_ctl', '--pgdata' => "$tempdir/nonexistent", 'promote' ],
+ [ 'pg_ctl', 'promote', '--pgdata' => "$tempdir/nonexistent" ],
qr/directory .* does not exist/,
'pg_ctl promote with nonexistent directory');
@@ -19,14 +19,14 @@
$node_primary->init(allows_streaming => 1);
command_fails_like(
- [ 'pg_ctl', '--pgdata' => $node_primary->data_dir, 'promote' ],
+ [ 'pg_ctl', 'promote', '--pgdata' => $node_primary->data_dir ],
qr/PID file .* does not exist/,
'pg_ctl promote of not running instance fails');
$node_primary->start;
command_fails_like(
- [ 'pg_ctl', '--pgdata' => $node_primary->data_dir, 'promote' ],
+ [ 'pg_ctl', 'promote', '--pgdata' => $node_primary->data_dir ],
qr/not in standby mode/,
'pg_ctl promote of primary instance fails');
@@ -41,11 +41,11 @@
command_ok(
[
- 'pg_ctl',
+ 'pg_ctl', 'promote',
'--pgdata' => $node_standby->data_dir,
- '--no-wait', 'promote'
+ '--no-wait',
],
- 'pg_ctl --no-wait promote of standby runs');
+ 'pg_ctl promote --no-wait of standby runs');
ok( $node_standby->poll_query_until(
'postgres', 'SELECT NOT pg_is_in_recovery()'),
@@ -60,7 +60,7 @@
is($node_standby->safe_psql('postgres', 'SELECT pg_is_in_recovery()'),
't', 'standby is in recovery');
-command_ok([ 'pg_ctl', '--pgdata' => $node_standby->data_dir, 'promote' ],
+command_ok([ 'pg_ctl', 'promote', '--pgdata' => $node_standby->data_dir ],
'pg_ctl promote of standby runs');
# no wait here
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 0ae2e982727..65dcb6ce04a 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -147,10 +147,9 @@ sub test_recovery_standby
my $res = run_log(
[
- 'pg_ctl',
+ 'pg_ctl', 'start',
'--pgdata' => $node_standby->data_dir,
'--log' => $node_standby->logfile,
- 'start',
]);
ok(!$res, 'invalid recovery startup fails');
@@ -170,10 +169,9 @@ sub test_recovery_standby
run_log(
[
- 'pg_ctl',
+ 'pg_ctl', 'start',
'--pgdata' => $node_standby->data_dir,
'--log' => $node_standby->logfile,
- 'start',
]);
# wait for postgres to terminate
diff --git a/src/test/recovery/t/024_archive_recovery.pl b/src/test/recovery/t/024_archive_recovery.pl
index b4527ec0843..fb4ba980743 100644
--- a/src/test/recovery/t/024_archive_recovery.pl
+++ b/src/test/recovery/t/024_archive_recovery.pl
@@ -76,10 +76,9 @@ sub test_recovery_wal_level_minimal
# that the server ends with an error during recovery.
run_log(
[
- 'pg_ctl',
+ 'pg_ctl', 'start',
'--pgdata' => $recovery_node->data_dir,
'--log' => $recovery_node->logfile,
- 'start',
]);
# wait for postgres to terminate
--
2.48.1
