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

Hi Peter,

Peter Smith <smithpb2250@gmail.com> writes:

> 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.

I was going to say that if it were deliberate, I would have expected the
two `--verbose` swithces to be together, but then I noticed that the
--recovery-timeout switch was added later (commit 04c8634c0c4d), rather
haphazardly between them.  Still, I would have expected a comment as to
why this command was being invoked extra-verbosely, but I'll restore it
in the next version.

I've Cc-ed both Peter (the committer) and Euler (the author) in case
they have any insight.

> ~~
>
>  # 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

Peter Smith <smithpb2250@gmail.com> writes:

>

> 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.

I was going to say that if it were deliberate, I would have expected the

two `--verbose` swithces to be together, but then I noticed that the

--recovery-timeout switch was added later (commit 04c8634c0c4d), rather

haphazardly between them.  Still, I would have expected a comment as to

why this command was being invoked extra-verbosely, but I'll restore it

in the next version.

"Euler Taveira" <euler@eulerto.com> writes:
> It is kind of annoying to keep version 20230309 around to run perltidy. Do we
> have any other alternatives?

As it says in src/tools/pgindent/README:

2) Install perltidy.  Please be sure it is version 20230309 (older and newer
   versions make different formatting choices, and we want consistency).

I don't think anyone is especially wedded to 20230309 in particular,
but we don't want different developers using different versions and
coming out with different results.  The whole point of this effort
is to standardize code layout as best we can, so that would be
counterproductive.

Do you have a strong argument for switching to some other specific
version of perltidy?

            regards, tom lane

"Euler Taveira" <euler@eulerto.com> writes:

> It is kind of annoying to keep version 20230309 around to run perltidy. Do we

> have any other alternatives?

As it says in src/tools/pgindent/README:

2) Install perltidy.  Please be sure it is version 20230309 (older and newer

   versions make different formatting choices, and we want consistency).

I don't think anyone is especially wedded to 20230309 in particular,

but we don't want different developers using different versions and

coming out with different results.  The whole point of this effort

is to standardize code layout as best we can, so that would be

counterproductive.

Do you have a strong argument for switching to some other specific

version of perltidy?

"Euler Taveira" <euler@eulerto.com> writes:

> On Mon, Jan 20, 2025, at 7:49 PM, Tom Lane wrote:
>> "Euler Taveira" <euler@eulerto.com> writes:
>> > It is kind of annoying to keep version 20230309 around to run perltidy. Do we
>> > have any other alternatives?
>> 
>> As it says in src/tools/pgindent/README:
>> 
>> 2) Install perltidy.  Please be sure it is version 20230309 (older and newer
>>    versions make different formatting choices, and we want consistency).
>> 
>> I don't think anyone is especially wedded to 20230309 in particular,
>> but we don't want different developers using different versions and
>> coming out with different results.  The whole point of this effort
>> is to standardize code layout as best we can, so that would be
>> counterproductive.
>
> I can live with that.
>
>> Do you have a strong argument for switching to some other specific
>> version of perltidy?
>> 
>
> I don't. Let's see if the Dagfinn's patch will maintain more stable formatting
> over the years. I tested in a previous version (20220613) after applying the
> patch and it didn't suggest changes.

More interesting than running older versions of perltidy would be
running more recent ones.  On top of the perltidy patch upthread, the
current version (20250105) only wants to make a couple of changes to
PostgreSQL::Test::Cluster (attached):

  1. A slight change to the indentation of a multi-line ternary
     statement. I slightly prefer the old version, but the new one
     doesn't bother me either.

  2. Ignoring the length of ## no critic comments when breaking lines,
     which is a definite improvement for the secondary `package`
     statements in the file.

These changes came with Perl::Tidy 20230701, which is the very next
version after the one we're currently using.  Subsequent versions have
caused no other changes, with or without my fat comma patch.

All in all, this makes me +0.5 to bumping the required Perl::Tidy
version, and +0.5 on (at least considering) bumping it to the latest
version before the pre-release-branch pgperltidy run.

- ilmari

From f6eb43ed13d3138c4cbde6bbdc2e17fd583aefa9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 21 Jan 2025 00:54:37 +0000
Subject: [PATCH] Run pgperltidy with Perl::Tidy-20250105

---
 src/test/perl/PostgreSQL/Test/Cluster.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index f521ad0b12f..fbddc85c554 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1826,8 +1826,8 @@ sub get_free_port
         {
             foreach my $addr (qw(127.0.0.1),
                 ($use_tcp && $PostgreSQL::Test::Utils::windows_os)
-                  ? qw(127.0.0.2 127.0.0.3 0.0.0.0)
-                  : ())
+                ? qw(127.0.0.2 127.0.0.3 0.0.0.0)
+                : ())
             {
                 if (!can_bind($addr, $port))
                 {
@@ -3738,8 +3738,7 @@ sub advance_wal
 
 ##########################################################################
 
-package PostgreSQL::Test::Cluster::V_11
-  ;    ## no critic (ProhibitMultiplePackages)
+package PostgreSQL::Test::Cluster::V_11;    ## no critic (ProhibitMultiplePackages)
 
 use parent -norequire, qw(PostgreSQL::Test::Cluster);
 
@@ -3766,8 +3765,7 @@ sub init
 
 ##########################################################################
 
-package PostgreSQL::Test::Cluster::V_10
-  ;    ## no critic (ProhibitMultiplePackages)
+package PostgreSQL::Test::Cluster::V_10;    ## no critic (ProhibitMultiplePackages)
 
 use parent -norequire, qw(PostgreSQL::Test::Cluster::V_11);
 
-- 
2.39.5

=?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org> writes:
> All in all, this makes me +0.5 to bumping the required Perl::Tidy
> version, and +0.5 on (at least considering) bumping it to the latest
> version before the pre-release-branch pgperltidy run.

Nah, I'm pretty much -1 on bumping our perltidy version frequently.
That imposes costs on every developer who wants to track it.
It's unlikely that anyone will be on a platform that updates it
exactly when we decide to change, so most of us are going to be
using hand-installed copies that will have to be hand-updated
whenever we change versions.

As a data point, we were using 20170521 for five years before
adopting 20230309, and 20090616 for seven years before that,
which is as far back as I can trace any definitive info about
which version was being used.  Every five years or so sounds
like a sane cadence to me, in terms of developer overhead
versus likely formatting improvements.

(Of course, if a new version comes out that is way better than
what we're using, I could be persuaded that it's worth changing.
But from what you're showing here, that hasn't happened yet.)

            regards, tom lane