pg_createsubscriber TAP test wrapping makes command options hard to read.

Peter Smith <smithpb2250@gmail.com> writes:

While reviewing a patch for another pg_createsubscriber thread I found
that multiple TAP tests have a terrible wrapping where the command
options and their associated oparg are separated on different lines
instead of paired together nicely. This makes it unnecessarily
difficult to see what the test is doing.

I think that is mostly the fault of pgperltidy. We did change
the options we use with it awhile back, so maybe now it will honor
your manual changes to its line-wrapping choices. But I wouldn't
bet on that. Did you check what happens if you run the modified
code through pgperltidy?

(If the answer is bad, we could look into making further changes so
that pgperltidy won't override these decisions. But there's no point
in manually patching this if it'll just get undone.)

regards, tom lane

On Thu, Dec 12, 2024 at 12:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

While reviewing a patch for another pg_createsubscriber thread I found
that multiple TAP tests have a terrible wrapping where the command
options and their associated oparg are separated on different lines
instead of paired together nicely. This makes it unnecessarily
difficult to see what the test is doing.

I think that is mostly the fault of pgperltidy. We did change
the options we use with it awhile back, so maybe now it will honor
your manual changes to its line-wrapping choices. But I wouldn't
bet on that. Did you check what happens if you run the modified
code through pgperltidy?

(If the answer is bad, we could look into making further changes so
that pgperltidy won't override these decisions. But there's no point
in manually patching this if it'll just get undone.)

Thanks for your suggestion. As you probably suspected, the answer is bad.

I ran pgperltidy on the "fixed" file:
[postgres@CentOS7-x64 oss_postgres_misc]$
src/tools/pgindent/pgperltidy
src/bin/pg_basebackup/t/040_pg_createsubscriber.pl

This reverted it to how it currently looks on master.

The strange thing is there are other commands in that file very
similar to the ones I had changed but those already looked good, yet
they remained unaffected by the pgperltidy. Why?

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

Peter Smith <smithpb2250@gmail.com> writes:

The strange thing is there are other commands in that file very
similar to the ones I had changed but those already looked good, yet
they remained unaffected by the pgperltidy. Why?

You sure it's not just luck-of-the-draw? I think that perltidy
is just splitting the lines based on length, so sometimes related
options would be kept together and sometimes not.

regards, tom lane

On Thu, Dec 12, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Smith <smithpb2250@gmail.com> writes:

The strange thing is there are other commands in that file very
similar to the ones I had changed but those already looked good, yet
they remained unaffected by the pgperltidy. Why?

You sure it's not just luck-of-the-draw? I think that perltidy
is just splitting the lines based on length, so sometimes related
options would be kept together and sometimes not.

TBH, I have no idea what logic perltidy uses. I did find some
configurations here [1]https://github.com/postgres/postgres/blob/master/src/tools/pgindent/perltidyrc (are those what it pgperltidy uses?) but those
claim max line length is 78 which I didn;t come anywhere near
exceeding.

After some more experimentation, I've noticed that it is trying to
keep only 2 items on each line. So whether it looks good or not seems
to depend if there is an even or odd number of options without
arguments up-front. Maybe those perltidy "tightness" switches?

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.

======
[1]: https://github.com/postgres/postgres/blob/master/src/tools/pgindent/perltidyrc

Kind Regards,
Peter Smith.
Fujitsu Australia

On Thu, Dec 12, 2024 at 02:13:40PM +1100, Peter Smith wrote:

TBH, I have no idea what logic perltidy uses. I did find some
configurations here [1] (are those what it pgperltidy uses?) but those
claim max line length is 78 which I didn't come anywhere near
exceeding.

Gave up trying to understand its internals and its rules years ago.
It's complicated enough that we require a specific version of the tool
for the tree with a custom PATH :D

After some more experimentation, I've noticed that it is trying to
keep only 2 items on each line. So whether it looks good or not seems
to depend if there is an even or odd number of options without
arguments up-front. Maybe those perltidy "tightness" switches?

Don't recall so even under Perl-Tidy-20230309, because it comes down
to the number of elements in these arrays and the length of their
values, not the specific values in each element of the array.

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

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.

PSA v2-0001. This time it can survive pgperltidy unchanged.

In passing I also removed the duplicated '--verbose' and changed the
'Pub2' case mentioned in the original post.

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

On Thu, Dec 12, 2024 at 03:24:42PM +1100, Peter Smith wrote:

PSA v2-0001. This time it can survive pgperltidy unchanged.

Confirmed. It looks to apply cleanly to v17 as well. Better to
backpatch to avoid conflict frictions, even if it's cosmetic.

In passing I also removed the duplicated '--verbose' and changed the
'Pub2' case mentioned in the original post.

Right. I didn't notice this dup in "run pg_createsubscriber on node
S". All the commands of the test seem to be in order with what you've
attached, so LGTM.
--
Michael

On 2024-Dec-12, Michael Paquier wrote:

On Thu, Dec 12, 2024 at 03:24:42PM +1100, Peter Smith wrote:

PSA v2-0001. This time it can survive pgperltidy unchanged.

Confirmed. It looks to apply cleanly to v17 as well. Better to
backpatch to avoid conflict frictions, even if it's cosmetic.

I'd rather use #<<< and #>>> markers.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

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

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

On 2024-12-12 Th 8:17 AM, Dagfinn Ilmari Mannsåker 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.

+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 Thu, Dec 12, 2024 at 06:03:33PM +0000, Dagfinn Ilmari Mannsåker wrote:

On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:

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.

Agreed. I tend to prefer long options too. That's less mapping to
think about the intentions behind some tests when reading their code.

The idea of using fat commas is neat, Ilmari. I didn't suspect that
this one was possible to trick perltidy.
--
Michael

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]https://www.postgresql.org/docs/devel/app-pgcreatesubscriber.html, 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