Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Peter Smith <smithpb2250@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,  Tom Lane <tgl@sss.pgh.pa.us>,
  PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_createsubscriber TAP test wrapping makes command options
 hard to read.
References: <CAHut+PvfaUZQR3rEnj-Dcg_6713Wu4yRdd-yZkkC9NVsARht8A@mail.gmail.com>
	<556792.1733967676@sss.pgh.pa.us>
	<CAHut+Ps6Q+i_3xS6H7ybw3-hx9+2QOuneTJwHArxqxFnKX5+4g@mail.gmail.com>
	<564915.1733971586@sss.pgh.pa.us>
	<CAHut+PvFGytgJT+K_e2x+uWKuVYCBR3HNaNtPKYEkO3YxAwZvw@mail.gmail.com>
	<Z1peKSb8roiDNgRj@paquier.xyz>
	<CAHut+PvYsVhF5k-mYHVEFdpijdgkYRtoeWVxdspxio0ePRpkug@mail.gmail.com>
	<87pllx5cqn.fsf@wibble.ilmari.org>
Date: Thu, 12 Dec 2024 13:17:48 +0000
In-Reply-To: <87pllx5cqn.fsf@wibble.ilmari.org> ("Dagfinn Ilmari
 =?utf-8?Q?Manns=C3=A5ker=22's?=
	message of "Thu, 12 Dec 2024 12:04:16 +0000")
Message-ID: <87msh159c3.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/87msh159c3.fsf%40wibble.ilmari.org>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit

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


--=-=-=
Content-Type: text/x-diff
Content-Disposition: inline;
 filename=v4-0001-Modify-wrapping-to-make-command-options-easier-to.patch

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


--=-=-=--