More Perl cleanups

On Fri, Mar 14, 2025 at 05:39:30PM +0000, Dagfinn Ilmari Mannsåker wrote:

In some recent-ish commits (ce1b0f9d, fd4c4ede and cc2c9fa6) we
converted a lot of the TAP tests to use long command line options and
fat commas (=>) to separate command line options from their arguments,
so that perltidy does't break lines between them.

However, those patches were nowhere near complete, so here's a follow-up
to fix all the cases I could find. While I was there I failed to resist
the urge to do some other minor tidy-ups that I think make things
neater, but perltidy has no opinion on. I also eliminated some variables
that were only initialised and then used once.

Thanks for digging into all that. Agreed that it would be nice to
apply a more consistent style for this release. I'll look more into
what you have here.

The second patch might be more controversial: it eliminates unnecessary
quoting from hash keys, both inside curly braces and before fat commas
(except where a hash has a mix of keys that need quoting and not).

Hmm. I can partially get behind this one, seeing things like that in
what you have sent:

@@ -24,8 +24,8 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
 # Make sure pg_hba.conf is set up to allow connections from backupuser.
 # This is only needed on Windows machines that don't use UNIX sockets.
 $node->init(
-	'allows_streaming' => 1,
-	'auth_extra' => [ '--create-role' => 'backupuser' ]);
+	allows_streaming => 1,
+	auth_extra => [ '--create-role' => 'backupuser' ]);

This option handling style is inconsistent in the tree. Most of the
time these keywords are not quoted when it comes to our internal test
modules..
--
Michael

On Sat, Mar 15, 2025 at 03:26:10PM +0900, Michael Paquier wrote:

Thanks for digging into all that. Agreed that it would be nice to
apply a more consistent style for this release. I'll look more into
what you have here.

The changes in pg_dump's 010_dump_connstr.pl read the same.

extension_schema in test_pg_dump adds silently a --no-sync.

Hmm. I can partially get behind this one, seeing things like that in
what you have sent:

@@ -24,8 +24,8 @@ my $node = PostgreSQL::Test::Cluster->new('primary');
# Make sure pg_hba.conf is set up to allow connections from backupuser.
# This is only needed on Windows machines that don't use UNIX sockets.
$node->init(
-	'allows_streaming' => 1,
-	'auth_extra' => [ '--create-role' => 'backupuser' ]);
+	allows_streaming => 1,
+	auth_extra => [ '--create-role' => 'backupuser' ]);

This option handling style is inconsistent in the tree. Most of the
time these keywords are not quoted when it comes to our internal test
modules..

So even for the option handling, what we have here is a mixed bad of
inconsistencies and rather consistent-ish behaviors.

Some notes:
src/bin/pg_basebackup/t/010_pg_basebackup.pl quotes tablespace_map.
src/bin/pg_basebackup/t/011_in_place_tablespace.pl quotes allows_streaming.
slot_type and restart_lsn are quoted everywhere now.
src/bin/pg_combinebackup/t/008_promote.pl quotes has_streaming.
ENV is a mixed bag of various things, single, double or no quotes.
pg_verifybackup/t/003_corruption.pl, 008, 009, 010 are a set of local
changes.
001_uri.pl was inconsistent, still local.
The parts about auth_extra are inconsistent (like in
002_connection_limits.pl).
Inconsistency in BackgroundPsql.pm, but it's always quoted now.
021_row_visibility.pl and 032_relfilenode_reuse.pl are local. Hm, not
planning to bother here.
Okay for 003_sslinfo.pl and 002_scram.pl, that mix both styles,
unquoting looks fine.
Not sure about the build scripts as a whole.

One point that applies for sure is that some existing options use an
inconsistent style across the tree in the TAP tests, mainly for init()
and init_from_backup(). I've extracted these from 0002, and applied
the subset. For the rest, if there are more opinions, feel free..
--
Michael