Re: pg_createsubscriber TAP test wrapping makes command options hard to read. - Mailing list pgsql-hackers

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Peter Smith <smithpb2250@gmail.com> writes:
>
>> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier <michael@paquier.xyz> wrote:
>> ...
>>
>>> > So, AFAICT I can workaround the perltidy wrapping just by putting all
>>> > the noarg options at the bottom of the command, then all the
>>> > option/optarg pairs (ie 2s) will stay together. I can post another
>>> > patch to do it this way unless you think it is too hacky.
>>>
>>> This trick works for me if that makes the long list of option easier
>>> to read.  With two elements of the array perl line, I would just put
>>> some --dry-run or --verbose at the end of their respective arrays.
>>> --
>>> Michael
>>
>> Hi Michael.
>>
>> Yes, that is the workaround that I was proposing.
>
> A better option, IMO, is to use the fat comma (=>) between options and
> their values.  This makes it clear both to humans and perltidy that they
> belong together, and we can put all the valueless options first without
> things being rewrapped.

Here's a more thorough patch, that also applies the fat comma treatment
to other pg_createsubscriber invocations in the same file that don't
currently happen to be mangled by perltidy.  It also adds trailing
commas to the last item in multi-line command arrays, which is common
perl style.

- ilmari

From 953d0c8ca8202d6f53af833fd53e3f6b1929fa77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Thu, 12 Dec 2024 11:56:15 +0000
Subject: [PATCH v4] Modify wrapping to make command options easier to read

Use fat comma after options that take values, both to make it clearer
to humans, and to stop perltidy from re-wrapping them.

Also remove pointless quoting of $PostgreSQL::Test::Utils::timeout_default.
---
 .../t/040_pg_createsubscriber.pl              | 255 +++++++++---------
 1 file changed, 135 insertions(+), 120 deletions(-)

diff --git a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
index 0a900edb656..369846db0d0 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -46,69 +46,75 @@ sub generate_db
 command_fails(['pg_createsubscriber'],
     'no subscriber data directory specified');
 command_fails(
-    [ 'pg_createsubscriber', '--pgdata', $datadir ],
+    [ 'pg_createsubscriber', '--pgdata' => $datadir ],
     'no publisher connection string specified');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
     ],
     'no database name specified');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432',
-        '--database', 'pg1',
-        '--database', 'pg1'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
+        '--database' => 'pg1',
+        '--database' => 'pg1',
     ],
     'duplicate database name');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432',
-        '--publication', 'foo1',
-        '--publication', 'foo1',
-        '--database', 'pg1',
-        '--database', 'pg2'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
+        '--publication' => 'foo1',
+        '--publication' => 'foo1',
+        '--database' => 'pg1',
+        '--database' => 'pg2',
     ],
     'duplicate publication name');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432',
-        '--publication', 'foo1',
-        '--database', 'pg1',
-        '--database', 'pg2'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
+        '--publication' => 'foo1',
+        '--database' => 'pg1',
+        '--database' => 'pg2',
     ],
     'wrong number of publication names');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432',
-        '--publication', 'foo1',
-        '--publication', 'foo2',
-        '--subscription', 'bar1',
-        '--database', 'pg1',
-        '--database', 'pg2'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
+        '--publication' => 'foo1',
+        '--publication' => 'foo2',
+        '--subscription' => 'bar1',
+        '--database' => 'pg1',
+        '--database' => 'pg2',
     ],
     'wrong number of subscription names');
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $datadir,
-        '--publisher-server', 'port=5432',
-        '--publication', 'foo1',
-        '--publication', 'foo2',
-        '--subscription', 'bar1',
-        '--subscription', 'bar2',
-        '--replication-slot', 'baz1',
-        '--database', 'pg1',
-        '--database', 'pg2'
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $datadir,
+        '--publisher-server' => 'port=5432',
+        '--publication' => 'foo1',
+        '--publication' => 'foo2',
+        '--subscription' => 'bar1',
+        '--subscription' => 'bar2',
+        '--replication-slot' => 'baz1',
+        '--database' => 'pg1',
+        '--database' => 'pg2',
     ],
     'wrong number of replication slot names');
 
@@ -168,41 +174,44 @@ sub generate_db
 # Run pg_createsubscriber on a promoted server
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_t->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_t->host, '--subscriber-port',
-        $node_t->port, '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_t->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_t->host,
+        '--subscriber-port' => $node_t->port,
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'target server is not in recovery');
 
 # Run pg_createsubscriber when standby is running
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'standby is up and running');
 
 # Run pg_createsubscriber on about-to-fail node F
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--pgdata', $node_f->data_dir,
-        '--publisher-server', $node_p->connstr($db1),
-        '--socketdir', $node_f->host,
-        '--subscriber-port', $node_f->port,
-        '--database', $db1,
-        '--database', $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--pgdata' => $node_f->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_f->host,
+        '--subscriber-port' => $node_f->port,
+        '--database' => $db1,
+        '--database' => $db2
     ],
     'subscriber data directory is not a copy of the source database cluster');
 
@@ -216,14 +225,15 @@ sub generate_db
 # Run pg_createsubscriber on node C (P -> S -> C)
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_c->data_dir, '--publisher-server',
-        $node_s->connstr($db1), '--socketdir',
-        $node_c->host, '--subscriber-port',
-        $node_c->port, '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_c->data_dir,
+        '--publisher-server' => $node_s->connstr($db1),
+        '--socketdir' => $node_c->host,
+        '--subscriber-port' => $node_c->port,
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'primary server is in recovery');
 
@@ -239,14 +249,16 @@ sub generate_db
 $node_s->stop;
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--database' => $db1,
+        '--database' => $db2,
+
     ],
     'primary contains unmet conditions on node P');
 # Restore default settings here but only apply it after testing standby. Some
@@ -268,14 +280,15 @@ sub generate_db
 });
 command_fails(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'standby contains unmet conditions on node S');
 $node_s->append_conf(
@@ -321,19 +334,20 @@ sub generate_db
 # dry run mode on node S
 command_ok(
     [
-        'pg_createsubscriber', '--verbose',
-        '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
-        '--dry-run', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--publication',
-        'pub1', '--publication',
-        'pub2', '--subscription',
-        'sub1', '--subscription',
-        'sub2', '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--publication' => 'pub1',
+        '--publication' => 'pub2',
+        '--subscription' => 'sub1',
+        '--subscription' => 'sub2',
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'run pg_createsubscriber --dry-run on node S');
 
@@ -346,32 +360,33 @@ sub generate_db
 # pg_createsubscriber can run without --databases option
 command_ok(
     [
-        'pg_createsubscriber', '--verbose',
-        '--dry-run', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--replication-slot',
-        'replslot1'
+        'pg_createsubscriber',
+        '--verbose',
+        '--dry-run',
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--replication-slot' => 'replslot1',
     ],
     'run pg_createsubscriber without --databases');
 
 # Run pg_createsubscriber on node S
 command_ok(
     [
-        'pg_createsubscriber', '--verbose',
-        '--recovery-timeout', "$PostgreSQL::Test::Utils::timeout_default",
-        '--verbose', '--pgdata',
-        $node_s->data_dir, '--publisher-server',
-        $node_p->connstr($db1), '--socketdir',
-        $node_s->host, '--subscriber-port',
-        $node_s->port, '--publication',
-        'pub1', '--publication',
-        'Pub2', '--replication-slot',
-        'replslot1', '--replication-slot',
-        'replslot2', '--database',
-        $db1, '--database',
-        $db2
+        'pg_createsubscriber',
+        '--verbose',
+        '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
+        '--pgdata' => $node_s->data_dir,
+        '--publisher-server' => $node_p->connstr($db1),
+        '--socketdir' => $node_s->host,
+        '--subscriber-port' => $node_s->port,
+        '--publication' => 'pub1',
+        '--publication' => 'pub2',
+        '--replication-slot' => 'replslot1',
+        '--replication-slot' => 'replslot2',
+        '--database' => $db1,
+        '--database' => $db2,
     ],
     'run pg_createsubscriber on node S');
 
-- 
2.47.1