Обсуждение: Re: pg_createsubscriber TAP test wrapping makes command options hard to read.

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.

- ilmari

From e972f1a5f87499390f401e7db2c079fb87533553 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 v3] 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.
---
 .../t/040_pg_createsubscriber.pl              | 169 +++++++++---------
 1 file changed, 89 insertions(+), 80 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..0f7bb103177 100644
--- a/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
+++ b/src/bin/pg_basebackup/t/040_pg_createsubscriber.pl
@@ -168,41 +168,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 +219,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 +243,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 +274,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 +328,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 +354,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

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

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.

+1 for this approach.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>> 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.

> +1 for this approach.

Indeed, this is much nicer if it's something perltidy knows about.

However, I know we have the same issue in many other places.
Anyone feel like running through all the TAP scripts?

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Andrew Dunstan <andrew@dunslane.net> writes:
>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>> 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.
>
>> +1 for this approach.
>
> Indeed, this is much nicer if it's something perltidy knows about.
>
> However, I know we have the same issue in many other places.
> Anyone feel like running through all the TAP scripts?

I can have a go in the next few days.  A quick grep spotted another
workaround in 027_stream_regress.pl: using paretheses around the option
and its value:

command_ok(
    [
        'pg_dump',
        ('--schema', 'pg_catalog'),
        ('-f', $outputdir . '/catalogs_primary.dump'),
        '--no-sync',
        ('-p', $node_primary->port),
        '--no-unlogged-table-data',
        'regression'
    ],
    'dump catalogs of primary server');

I think the fat comma is much nicer than this, so I'd like to convert
these too (and replace some of the concatenations with interpolation).

Technically the quotes aren't necessary around single-dash options
before => since unary minus works on strings as well as numbers, but
I'll leave them in for consistency.

- ilmari

On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker wrote:
>>>> 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.
>>> +1 for this approach.
>> Indeed, this is much nicer if it's something perltidy knows about.
>>
>> However, I know we have the same issue in many other places.
>> Anyone feel like running through all the TAP scripts?
> I can have a go in the next few days.  A quick grep spotted another
> workaround in 027_stream_regress.pl: using paretheses around the option
> and its value:
>
> command_ok(
>     [
>         'pg_dump',
>         ('--schema', 'pg_catalog'),
>         ('-f', $outputdir . '/catalogs_primary.dump'),
>         '--no-sync',
>         ('-p', $node_primary->port),
>         '--no-unlogged-table-data',
>         'regression'
>     ],
>     'dump catalogs of primary server');
>
> I think the fat comma is much nicer than this, so I'd like to convert
> these too (and replace some of the concatenations with interpolation).
>
> Technically the quotes aren't necessary around single-dash options
> before => since unary minus works on strings as well as numbers, but
> I'll leave them in for consistency.
>

I'd rather get rid of those and just use the long options.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
>>
>> command_ok(
>>     [
>>         'pg_dump',
>>         ('--schema', 'pg_catalog'),
>>         ('-f', $outputdir . '/catalogs_primary.dump'),
>>         '--no-sync',
>>         ('-p', $node_primary->port),
>>         '--no-unlogged-table-data',
>>         'regression'
>>     ],
>>     'dump catalogs of primary server');
>>
>> I think the fat comma is much nicer than this, so I'd like to convert
>> these too (and replace some of the concatenations with interpolation).
>>
>> Technically the quotes aren't necessary around single-dash options
>> before => since unary minus works on strings as well as numbers, but
>> I'll leave them in for consistency.
>
> I'd rather get rid of those and just use the long options.

Yeah, that is more self-documenting, so I'll do that while I'm at it.

--
- ilmari

On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> > On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> >>
> >> command_ok(
> >>      [
> >>              'pg_dump',
> >>              ('--schema', 'pg_catalog'),
> >>              ('-f', $outputdir . '/catalogs_primary.dump'),
> >>              '--no-sync',
> >>              ('-p', $node_primary->port),
> >>              '--no-unlogged-table-data',
> >>              'regression'
> >>      ],
> >>      'dump catalogs of primary server');
> >>
> >> I think the fat comma is much nicer than this, so I'd like to convert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leave them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>
> --

Your fat-comma solution is much better than something I could have
come up with. Thanks for taking this on.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
> > On 2024-12-12 Th 12:08 PM, Dagfinn Ilmari Mannsåker wrote:
> >>
> >> command_ok(
> >>      [
> >>              'pg_dump',
> >>              ('--schema', 'pg_catalog'),
> >>              ('-f', $outputdir . '/catalogs_primary.dump'),
> >>              '--no-sync',
> >>              ('-p', $node_primary->port),
> >>              '--no-unlogged-table-data',
> >>              'regression'
> >>      ],
> >>      'dump catalogs of primary server');
> >>
> >> I think the fat comma is much nicer than this, so I'd like to convert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leave them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>

Hi. This thread has been silent for 1 month. Just wondering, has any
progress been made on these changes?

======
Kind Regards,
Peter Smith.
Fujitsu Australia

On Fri, Dec 13, 2024 at 12:17 AM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
>
> 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
>

Hi,

In your v4 patch, there is a fragment (below) that replaces a  double
'--verbose' switch with just a single '--verbose'.

As I have only recently learned, the '--verbose'' switch has a
cumulative effect [1], so the original double '--verbose' was probably
deliberate so it should be kept that way.

~~

 # 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');


======
[1] https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html

Kind Regards,
Peter Smith.
Fujitsu Australia

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jan 23, 2025 at 08:25:45PM +0000, Dagfinn Ilmari Mannsåker wrote:
>> Here's a patch for that.

> Thanks.  I had a bit of time today and applied it.

BF member drongo doesn't like the new test for amcheck.
Looks like it has to do with SSPI authentication producing
a different error than expected:

stderr:
#   Failed test 'checking with a non-existent user stderr /(?^:role "no_such_user" does not exist)/'
#   at C:/prog/bf/root/HEAD/pgsql/src/bin/pg_amcheck/t/002_nonesuch.pl line 86.
#                   'pg_amcheck: error: connection to server at "127.0.0.1", port 19928 failed: FATAL:  SSPI
authenticationfailed for user "no_such_user" 
# '
#     doesn't match '(?^:role "no_such_user" does not exist)'
# Looks like you failed 1 test of 107.

You might be able to work around this with auth_extra,
a la 1e3f461e8 and other past fixes.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Michael Paquier <michael@paquier.xyz> writes:
>> On Thu, Jan 23, 2025 at 08:25:45PM +0000, Dagfinn Ilmari Mannsåker wrote:
>>> Here's a patch for that.
>
>> Thanks.  I had a bit of time today and applied it.
>
> BF member drongo doesn't like the new test for amcheck.
> Looks like it has to do with SSPI authentication producing
> a different error than expected:

Ah yes, I always forget about that quirk on Windows.

> stderr:
> #   Failed test 'checking with a non-existent user stderr /(?^:role "no_such_user" does not exist)/'
> #   at C:/prog/bf/root/HEAD/pgsql/src/bin/pg_amcheck/t/002_nonesuch.pl line 86.
> # 'pg_amcheck: error: connection to server at "127.0.0.1", port 19928
> failed: FATAL: SSPI authentication failed for user "no_such_user"
> # '
> #     doesn't match '(?^:role "no_such_user" does not exist)'
> # Looks like you failed 1 test of 107.
>
> You might be able to work around this with auth_extra,
> a la 1e3f461e8 and other past fixes.

Here's a (blind, but at least doesn't break on Linux) patch for that.

- ilmari

From eb168096c3e88cbd849907f7b6398d87a7844544 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Fri, 24 Jan 2025 18:50:05 +0000
Subject: [PATCH] Fix Windows pg_amcheck test failure

For SSPI auth extra users need to be explicitly allowed, or we get
"SSPI authentication failed" instead of the expected "role does not
exist" error.
---
 src/bin/pg_amcheck/t/002_nonesuch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_amcheck/t/002_nonesuch.pl b/src/bin/pg_amcheck/t/002_nonesuch.pl
index 2697f1c1b1a..f23368abeab 100644
--- a/src/bin/pg_amcheck/t/002_nonesuch.pl
+++ b/src/bin/pg_amcheck/t/002_nonesuch.pl
@@ -11,7 +11,7 @@
 # Test set-up
 my ($node, $port);
 $node = PostgreSQL::Test::Cluster->new('test');
-$node->init;
+$node->init(auth_extra => [ '--create-role' => 'no_such_user' ]);
 $node->start;
 $port = $node->port;
 
-- 
2.39.5

On 2025-01-26 Su 9:02 AM, Andrew Dunstan wrote:
>
>
>
>
> This seems to me like the wrong fix. We don't want to create 
> "no_such_user" I think, we just want to catch the Windows error 
> message, as in this patch.
>
>
>

Ignore this ... I'm clearly insufficiently caffeinated this morning. 
Despite the name, this doesn't actually create the role, so the proposed 
fix would be fine, I think.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com