Re: Postgres perl module namespace
On Thu, Jun 23, 2022 at 10:45:40PM -0700, Noah Misch wrote: > On Wed, Jun 22, 2022 at 11:03:22AM -0400, Andrew Dunstan wrote: > > On 2022-06-22 We 03:21, Noah Misch wrote: > > > On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote: > > >> On 2022-04-19 Tu 18:39, Michael Paquier wrote: > > >>> +*generate_ascii_string = *TestLib::generate_ascii_string; > > >>> +*slurp_dir = *TestLib::slurp_dir; > > >>> +*slurp_file = *TestLib::slurp_file; > > >>> > > >>> I am not sure if it is possible and my perl-fu is limited in this > > >>> area, but could a failure be enforced when loading this path if a new > > >>> routine added in TestLib.pm is forgotten in this list? > > >> Not very easily that I'm aware of, but maybe some superior perl wizard > > >> will know better. > > > One can alias the symbol table, like > > > https://metacpan.org/pod/Package::Alias > > > does. I'm attaching what I plan to use. Today, check-world fails after > > > > > > sed -i 's/TestLib/PostgreSQL::Test::Utils/g; > > > s/PostgresNode/PostgreSQL::Test::Cluster/g' **/*.pl > > > > > > on REL_14_STABLE, because today's alias list is incomplete. With this > > > change, > > > the same check-world passes. > > The patch wasn't sufficient to make that experiment pass for REL_10_STABLE, > where 017_shm.pl uses the %params argument of get_new_node(). The problem > call stack had PostgreSQL::Test::Cluster->get_new_code calling > PostgreSQL::Test::Cluster->new, which needs v14- semantics. Here's a fixed > version, just changing the new() hack. I pushed this, but it broke lapwing and wrasse. I will investigate.
Re: Postgres perl module namespace
On Wed, Jun 22, 2022 at 11:03:22AM -0400, Andrew Dunstan wrote:
> On 2022-06-22 We 03:21, Noah Misch wrote:
> > On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote:
> >> On 2022-04-19 Tu 18:39, Michael Paquier wrote:
> >>> +*generate_ascii_string = *TestLib::generate_ascii_string;
> >>> +*slurp_dir = *TestLib::slurp_dir;
> >>> +*slurp_file = *TestLib::slurp_file;
> >>>
> >>> I am not sure if it is possible and my perl-fu is limited in this
> >>> area, but could a failure be enforced when loading this path if a new
> >>> routine added in TestLib.pm is forgotten in this list?
> >> Not very easily that I'm aware of, but maybe some superior perl wizard
> >> will know better.
> > One can alias the symbol table, like https://metacpan.org/pod/Package::Alias
> > does. I'm attaching what I plan to use. Today, check-world fails after
> >
> > sed -i 's/TestLib/PostgreSQL::Test::Utils/g;
> > s/PostgresNode/PostgreSQL::Test::Cluster/g' **/*.pl
> >
> > on REL_14_STABLE, because today's alias list is incomplete. With this
> > change,
> > the same check-world passes.
The patch wasn't sufficient to make that experiment pass for REL_10_STABLE,
where 017_shm.pl uses the %params argument of get_new_node(). The problem
call stack had PostgreSQL::Test::Cluster->get_new_code calling
PostgreSQL::Test::Cluster->new, which needs v14- semantics. Here's a fixed
version, just changing the new() hack.
I suspect v1 also misbehaved for non-core tests that subclass PostgresNode
(via the approach from commit 54dacc7) or PostgreSQL::Test::Cluster. I expect
this version will work with subclasses written for v14- and with subclasses
written for v15+. I didn't actually write dummy subclasses to test, and the
relevant permutations are numerous (e.g. whether or not the subclass overrides
new(), whether or not the subclass overrides get_new_node()).
> Nice. 30 years of writing perl and I'm still learning of nifty features.
Thanks for reviewing.
commit 5155e0f
Author: Noah Misch
AuthorDate: Thu Jun 23 15:31:41 2022 -0700
Commit: Noah Misch
CommitDate: Thu Jun 23 15:31:41 2022 -0700
For PostgreSQL::Test compatibility, alias entire package symbol tables.
Remove the need to edit back-branch-specific code sites when
back-patching the addition of a PostgreSQL::Test::Utils symbol. Replace
per-symbol, incomplete alias lists. Give old and new package names the
same EXPORT and EXPORT_OK semantics. Back-patch to v10 (all supported
versions).
Reviewed by Andrew Dunstan.
Discussion: https://postgr.es/m/20220622072144.gd4167...@rfd.leadboat.com
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 9 ---
src/test/perl/PostgreSQL/Test/Utils.pm | 40 +++---
src/test/perl/PostgresNode.pm| 23 +++--
src/test/perl/TestLib.pm | 42
4 files changed, 19 insertions(+), 95 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 12339c2..a855fbc 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1,9 +1,9 @@
# Copyright (c) 2022, PostgreSQL Global Development Group
-# allow use of release 15+ perl namespace in older branches
-# just 'use' the older module name.
-# See PostgresNode.pm for function implementations
+# Allow use of release 15+ Perl package name in older branches, by giving that
+# package the same symbol table as the older package. See PostgresNode::new
+# for behavior reacting to the class name.
package PostgreSQL::Test::Cluster;
@@ -11,5 +11,8 @@ use strict;
use warnings;
use PostgresNode;
+BEGIN { *PostgreSQL::Test::Cluster:: = \*PostgresNode::; }
+
+use Exporter 'import';
1;
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index bdbbd6e..e743bdf 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1,48 +1,16 @@
# Copyright (c) 2022, PostgreSQL Global Development Group
-# allow use of release 15+ perl namespace in older branches
-# just 'use' the older module name.
-# We export the same names as the v15 module.
-# See TestLib.pm for alias assignment that makes this all work.
+# Allow use of release 15+ Perl package name in older branches, by giving that
+# package the same symbol table as the older package.
package PostgreSQL::Test::Utils;
use strict;
use warnings;
-use Exporter 'import';
-
use TestLib;
+BEGIN { *PostgreSQL::Test::Utils:: = \*TestLib::; }
-our @EXPORT = qw(
- generate_ascii_string
- slurp_dir
- slurp_file
- append_to_file
- check_mode_recursive
- chmod_recursive
- check_pg_config
- dir_symlink
- system_or_bail
- system_log
- run_log
- run_command
- pump_until
-
- command_ok
- command_fails
- command_exit_is
- program_help_ok
- program_version_ok
- program_options_handling_ok
- command_like
-
Re: Postgres perl module namespace
On 2022-06-22 We 03:21, Noah Misch wrote: > On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote: >> On 2022-04-19 Tu 18:39, Michael Paquier wrote: >>> +*generate_ascii_string = *TestLib::generate_ascii_string; >>> +*slurp_dir = *TestLib::slurp_dir; >>> +*slurp_file = *TestLib::slurp_file; >>> >>> I am not sure if it is possible and my perl-fu is limited in this >>> area, but could a failure be enforced when loading this path if a new >>> routine added in TestLib.pm is forgotten in this list? >> Not very easily that I'm aware of, but maybe some superior perl wizard >> will know better. > One can alias the symbol table, like https://metacpan.org/pod/Package::Alias > does. I'm attaching what I plan to use. Today, check-world fails after > > sed -i 's/TestLib/PostgreSQL::Test::Utils/g; > s/PostgresNode/PostgreSQL::Test::Cluster/g' **/*.pl > > on REL_14_STABLE, because today's alias list is incomplete. With this change, > the same check-world passes. Nice. 30 years of writing perl and I'm still learning of nifty features. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote:
> On 2022-04-19 Tu 18:39, Michael Paquier wrote:
> > +*generate_ascii_string = *TestLib::generate_ascii_string;
> > +*slurp_dir = *TestLib::slurp_dir;
> > +*slurp_file = *TestLib::slurp_file;
> >
> > I am not sure if it is possible and my perl-fu is limited in this
> > area, but could a failure be enforced when loading this path if a new
> > routine added in TestLib.pm is forgotten in this list?
>
> Not very easily that I'm aware of, but maybe some superior perl wizard
> will know better.
One can alias the symbol table, like https://metacpan.org/pod/Package::Alias
does. I'm attaching what I plan to use. Today, check-world fails after
sed -i 's/TestLib/PostgreSQL::Test::Utils/g;
s/PostgresNode/PostgreSQL::Test::Cluster/g' **/*.pl
on REL_14_STABLE, because today's alias list is incomplete. With this change,
the same check-world passes.
Author: Noah Misch
Commit: Noah Misch
For PostgreSQL::Test compatibility, alias entire package symbol tables.
Remove the need to edit back-branch-specific code sites when
back-patching the addition of a PostgreSQL::Test::Utils symbol. Replace
per-symbol, incomplete alias lists. Give old and new package names the
same EXPORT and EXPORT_OK semantics. Back-patch to v10 (all supported
versions).
Reviewed by FIXME.
Discussion: https://postgr.es/m/FIXME
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 12339c2..a855fbc 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1,9 +1,9 @@
# Copyright (c) 2022, PostgreSQL Global Development Group
-# allow use of release 15+ perl namespace in older branches
-# just 'use' the older module name.
-# See PostgresNode.pm for function implementations
+# Allow use of release 15+ Perl package name in older branches, by giving that
+# package the same symbol table as the older package. See PostgresNode::new
+# for behavior reacting to the class name.
package PostgreSQL::Test::Cluster;
@@ -11,5 +11,8 @@ use strict;
use warnings;
use PostgresNode;
+BEGIN { *PostgreSQL::Test::Cluster:: = \*PostgresNode::; }
+
+use Exporter 'import';
1;
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index bdbbd6e..e743bdf 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1,48 +1,16 @@
# Copyright (c) 2022, PostgreSQL Global Development Group
-# allow use of release 15+ perl namespace in older branches
-# just 'use' the older module name.
-# We export the same names as the v15 module.
-# See TestLib.pm for alias assignment that makes this all work.
+# Allow use of release 15+ Perl package name in older branches, by giving that
+# package the same symbol table as the older package.
package PostgreSQL::Test::Utils;
use strict;
use warnings;
-use Exporter 'import';
-
use TestLib;
+BEGIN { *PostgreSQL::Test::Utils:: = \*TestLib::; }
-our @EXPORT = qw(
- generate_ascii_string
- slurp_dir
- slurp_file
- append_to_file
- check_mode_recursive
- chmod_recursive
- check_pg_config
- dir_symlink
- system_or_bail
- system_log
- run_log
- run_command
- pump_until
-
- command_ok
- command_fails
- command_exit_is
- program_help_ok
- program_version_ok
- program_options_handling_ok
- command_like
- command_like_safe
- command_fails_like
- command_checks_all
-
- $windows_os
- $is_msys2
- $use_unix_sockets
-);
+use Exporter 'import';
1;
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index be90963..5b8c7a9 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -149,6 +149,11 @@ of finding port numbers, registering instances for
cleanup, etc.
sub new
{
my ($class, $name, $pghost, $pgport) = @_;
+
+ # Use release 15+ semantics when called under a release 15+ name.
+ return PostgresNode->get_new_node(@_[ 1 .. $#_ ])
+ if $class ne 'PostgresNode';
+
my $testname = basename($0);
$testname =~ s/\.[^.]+$//;
my $self = {
@@ -2796,18 +2801,4 @@ sub corrupt_page_checksum
=cut
-# support release 15+ perl module namespace
-
-package PostgreSQL::Test::Cluster; ## no critic (ProhibitMultiplePackages)
-
-sub new
-{
- shift; # remove class param from args
- return PostgresNode->get_new_node(@_);
-}
-
-no warnings 'once';
-
-*get_free_port = *PostgresNode::get_free_port;
-
1;
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index f3ee20a..610050e 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -979,46 +979,4 @@ sub command_checks_all
=cut
-# support release 15+ perl module namespace
-
-package PostgreSQL::Test::Utils; ## no critic (ProhibitMultiplePackages)
-
-# we don't want to export anything here, but we want to support things called
-#
Re: Postgres perl module namespace
On 2022-04-21 09:42:44 -0400, Andrew Dunstan wrote: > On 2022-04-21 Th 00:11, Michael Paquier wrote: > > On Wed, Apr 20, 2022 at 03:56:17PM -0400, Andrew Dunstan wrote: > >> Basically I propose just to remove any mention of the Testlib items and > >> get_free_port from the export and alias lists for versions where they > >> are absent. If backpatchers need a function they can backport it if > >> necessary. > > Agreed. I am fine to stick to that (I may have done that only once or > > twice in the past years, so that does not happen a lot either IMO). > > The patch in itself looks like an improvement in the right direction, > > so +1 from me. > Thanks, pushed. Thanks for working on this!
Re: Postgres perl module namespace
On 2022-04-21 Th 00:11, Michael Paquier wrote: > On Wed, Apr 20, 2022 at 03:56:17PM -0400, Andrew Dunstan wrote: >> Basically I propose just to remove any mention of the Testlib items and >> get_free_port from the export and alias lists for versions where they >> are absent. If backpatchers need a function they can backport it if >> necessary. > Agreed. I am fine to stick to that (I may have done that only once or > twice in the past years, so that does not happen a lot either IMO). > The patch in itself looks like an improvement in the right direction, > so +1 from me. Thanks, pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Wed, Apr 20, 2022 at 03:56:17PM -0400, Andrew Dunstan wrote: > Basically I propose just to remove any mention of the Testlib items and > get_free_port from the export and alias lists for versions where they > are absent. If backpatchers need a function they can backport it if > necessary. Agreed. I am fine to stick to that (I may have done that only once or twice in the past years, so that does not happen a lot either IMO). The patch in itself looks like an improvement in the right direction, so +1 from me. -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On 2022-04-19 Tu 20:30, Michael Paquier wrote: > On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote: >> On 2022-04-19 Tu 18:39, Michael Paquier wrote: >>> +*generate_ascii_string = *TestLib::generate_ascii_string; >>> +*slurp_dir = *TestLib::slurp_dir; >>> +*slurp_file = *TestLib::slurp_file; >>> >>> I am not sure if it is possible and my perl-fu is limited in this >>> area, but could a failure be enforced when loading this path if a new >>> routine added in TestLib.pm is forgotten in this list? >> Not very easily that I'm aware of, but maybe some superior perl wizard >> will know better. > Okay. Please do not consider this as a blocker. I was just wondering > about ways to ease more the error reports when it comes to > back-patching, and this would move the error stack a bit earlier. There are a few other things that could make backpatching harder, and while they are not related to the namespace issue they do affect a bit how that is managed. The following variables are missing in various versions of TestLib: in version 13 and earlier: $is_msys2, $timeout_default in version 12 and earlier: $use_unix_sockets and the following functions are missing: in version 14 and earlier: pump_until in version 13 and earlier: dir_symlink in version 11 and earlier: run_command in version 10: check_mode_recursive, chmod_recursive, check_pg_config (Also in version 10 command_checks_all exists but isn't exported. I'm inclined just to remedy that along the way) Turning to PostgresNode, the class-wide function get_free_port is absent from version 10, and the following instance methods are absent from some or all of the back branches: adjust_conf, clean_node, command_fails_like, config_data, connect_fails, connect_ok, corrupt_page_checksum, group_access, installed_command, install_path, interactive_psql, logrotate, set_recovery_mode, set_standby_mode, wait_for_log We don't export or provide aliases for any of these instance methods in these patches, but attempts to use them in backpatched code will fail where they are absent, so I thought it worth mentioning. Basically I propose just to remove any mention of the Testlib items and get_free_port from the export and alias lists for versions where they are absent. If backpatchers need a function they can backport it if necessary. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Tue, Apr 19, 2022 at 07:24:58PM -0400, Andrew Dunstan wrote: > On 2022-04-19 Tu 18:39, Michael Paquier wrote: >> +*generate_ascii_string = *TestLib::generate_ascii_string; >> +*slurp_dir = *TestLib::slurp_dir; >> +*slurp_file = *TestLib::slurp_file; >> >> I am not sure if it is possible and my perl-fu is limited in this >> area, but could a failure be enforced when loading this path if a new >> routine added in TestLib.pm is forgotten in this list? > > Not very easily that I'm aware of, but maybe some superior perl wizard > will know better. Okay. Please do not consider this as a blocker. I was just wondering about ways to ease more the error reports when it comes to back-patching, and this would move the error stack a bit earlier. -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On 2022-04-19 Tu 18:39, Michael Paquier wrote: > On Tue, Apr 19, 2022 at 04:06:28PM -0400, Andrew Dunstan wrote: >> Here's a version with a fixed third patch that corrects a file misnaming >> and fixes the export issue referred to above. Passes my testing so far. > Wow. That's really cool. You are combining the best of both worlds > here to ease backpatching, as far as I understand what you wrote. Thanks. > > +*generate_ascii_string = *TestLib::generate_ascii_string; > +*slurp_dir = *TestLib::slurp_dir; > +*slurp_file = *TestLib::slurp_file; > > I am not sure if it is possible and my perl-fu is limited in this > area, but could a failure be enforced when loading this path if a new > routine added in TestLib.pm is forgotten in this list? Not very easily that I'm aware of, but maybe some superior perl wizard will know better. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Tue, Apr 19, 2022 at 04:06:28PM -0400, Andrew Dunstan wrote: > Here's a version with a fixed third patch that corrects a file misnaming > and fixes the export issue referred to above. Passes my testing so far. Wow. That's really cool. You are combining the best of both worlds here to ease backpatching, as far as I understand what you wrote. +*generate_ascii_string = *TestLib::generate_ascii_string; +*slurp_dir = *TestLib::slurp_dir; +*slurp_file = *TestLib::slurp_file; I am not sure if it is possible and my perl-fu is limited in this area, but could a failure be enforced when loading this path if a new routine added in TestLib.pm is forgotten in this list? -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On 2022-04-19 Tu 11:36, Andrew Dunstan wrote:
> On 2022-04-18 Mo 14:07, Tom Lane wrote:
>> Andrew Dunstan writes:
>>> No, I think we could probably just port the whole of src/test/PostreSQL
>>> back if required, and have it live alongside the old modules. Each TAP
>>> test is a separate miracle - see comments elsewhere about port
>>> assignment in parallel TAP tests.
>>> But that would mean we have some tests in the old flavor and some in the
>>> new flavor in the back branches, which might get confusing.
>> That works for back-patching entire new test scripts, but not for adding
>> some cases to an existing script, which I think is more common.
>>
>>
>
> I think I've come up with a better scheme that I hope will fix all or
> almost all of the pain complained of in this thread. I should note that
> we deliberately delayed making these changes until fairly early in the
> release 15 development cycle, and that was clearly a good decision.
>
> The attached three patches basically implement the new naming scheme for
> the back branches without doing away with the old scheme or doing a
> wholesale copy of the new modules.
>
> The first simply implements a proper "new" constructor for PostgresNode,
> just like we have in PostgreSQL:Test::Cluster. It's not really essential
> but it seems like a good idea. The second adds all the visible
> functionality of the PostgresNode and TestLib modules to the
> PostgreSQL::Test::Cluster and PostgreSQL::Test::Utils namespaces.. The
> third adds dummy packages so that any code doing 'use
> PostgreSQL::Test::Utils;' or 'use PostgreSQL::Test::Cluster;' will
> actually import the old modules. This last piece is where there might be
> some extra work needed, to export the names so that using an unqualified
> function or variable, say, 'slurp_file("foo");' will work. But in
> general, modulo that issue, I believe things should Just Work (tm). You
> should basically just be able to backpatch any new or modified TAP test
> without difficulty, sed script usage, etc.
>
> Comments welcome.
>
>
Here's a version with a fixed third patch that corrects a file misnaming
and fixes the export issue referred to above. Passes my testing so far.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 92d9303e23..94b39341ce 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -137,16 +137,22 @@ INIT
=over
-=item PostgresNode::new($class, $name, $pghost, $pgport)
+=item PostgresNode->new(node_name, %params)
-Create a new PostgresNode instance. Does not initdb or start it.
-
-You should generally prefer to use get_new_node() instead since it takes care
-of finding port numbers, registering instances for cleanup, etc.
+Class oriented alias for get_new_node()
=cut
sub new
+{
+ my $class = $_[0];
+ $class->isa(__PACKAGE__) || die "new() not called as a class method";
+ return get_new_node(@_);
+}
+
+# internal subroutine used by get_new_node(). SHould not be called by any
+# external client of the module.
+sub _new
{
my ($class, $name, $pghost, $pgport) = @_;
my $testname = basename($0);
@@ -1229,7 +1235,7 @@ sub get_new_node
}
# Lock port number found by creating a new node
- my $node = $class->new($name, $host, $port);
+ my $node = _new($class, $name, $host, $port);
if ($params{install_path})
{
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 92d9303e23..f5d2ba72e6 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2761,4 +2761,16 @@ sub corrupt_page_checksum
=cut
+# support release 15+ perl module namespace
+
+package PostgreSQL::Test::Cluster;
+
+sub new
+{
+ shift; # remove class param from args
+ return PostgresNode->get_new_node(@_);
+}
+
+sub get_free_port { return PostgresNode::get_free_port(); }
+
1;
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 0dfc414b07..92583f84b4 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -948,4 +948,46 @@ sub command_checks_all
=cut
+# support release 15+ perl module namespace
+
+package PostgreSQL::Test::Utils;
+
+# we don't want to export anything, but we want to support things called
+# via this package name explicitly.
+
+# use typeglobs to alias these functions and variables
+
+*generate_ascii_string = *TestLib::generate_ascii_string;
+*slurp_dir = *TestLib::slurp_dir;
+*slurp_file = *TestLib::slurp_file;
+*append_to_file = *TestLib::append_to_file;
+*check_mode_recursive = *TestLib::check_mode_recursive;
+*chmod_recursive = *TestLib::chmod_recursive;
+*check_pg_config = *TestLib::check_pg_config;
+*dir_symlink = *TestLib::dir_symlink;
+*system_or_bail = *TestLib::system_or_bail;
+*system_log = *TestLib::system_log;
+*run_log = *TestLib::run_log;
+*run_command = *TestLib::run_command;
+sub pump_until { die "pump_until not implemented in TestLib"; }
Re: Postgres perl module namespace
Hi, On 2022-04-19 11:36:44 -0400, Andrew Dunstan wrote: > The attached three patches basically implement the new naming scheme for > the back branches without doing away with the old scheme or doing a > wholesale copy of the new modules. That sounds like good plan! I don't know perl enough to comment on the details, but it looks roughly sane to me. Greetings, Andres Freund
Re: Postgres perl module namespace
On 2022-04-18 Mo 14:07, Tom Lane wrote:
> Andrew Dunstan writes:
>> No, I think we could probably just port the whole of src/test/PostreSQL
>> back if required, and have it live alongside the old modules. Each TAP
>> test is a separate miracle - see comments elsewhere about port
>> assignment in parallel TAP tests.
>> But that would mean we have some tests in the old flavor and some in the
>> new flavor in the back branches, which might get confusing.
> That works for back-patching entire new test scripts, but not for adding
> some cases to an existing script, which I think is more common.
>
>
I think I've come up with a better scheme that I hope will fix all or
almost all of the pain complained of in this thread. I should note that
we deliberately delayed making these changes until fairly early in the
release 15 development cycle, and that was clearly a good decision.
The attached three patches basically implement the new naming scheme for
the back branches without doing away with the old scheme or doing a
wholesale copy of the new modules.
The first simply implements a proper "new" constructor for PostgresNode,
just like we have in PostgreSQL:Test::Cluster. It's not really essential
but it seems like a good idea. The second adds all the visible
functionality of the PostgresNode and TestLib modules to the
PostgreSQL::Test::Cluster and PostgreSQL::Test::Utils namespaces.. The
third adds dummy packages so that any code doing 'use
PostgreSQL::Test::Utils;' or 'use PostgreSQL::Test::Cluster;' will
actually import the old modules. This last piece is where there might be
some extra work needed, to export the names so that using an unqualified
function or variable, say, 'slurp_file("foo");' will work. But in
general, modulo that issue, I believe things should Just Work (tm). You
should basically just be able to backpatch any new or modified TAP test
without difficulty, sed script usage, etc.
Comments welcome.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 92d9303e23..94b39341ce 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -137,16 +137,22 @@ INIT
=over
-=item PostgresNode::new($class, $name, $pghost, $pgport)
+=item PostgresNode->new(node_name, %params)
-Create a new PostgresNode instance. Does not initdb or start it.
-
-You should generally prefer to use get_new_node() instead since it takes care
-of finding port numbers, registering instances for cleanup, etc.
+Class oriented alias for get_new_node()
=cut
sub new
+{
+ my $class = $_[0];
+ $class->isa(__PACKAGE__) || die "new() not called as a class method";
+ return get_new_node(@_);
+}
+
+# internal subroutine used by get_new_node(). SHould not be called by any
+# external client of the module.
+sub _new
{
my ($class, $name, $pghost, $pgport) = @_;
my $testname = basename($0);
@@ -1229,7 +1235,7 @@ sub get_new_node
}
# Lock port number found by creating a new node
- my $node = $class->new($name, $host, $port);
+ my $node = _new($class, $name, $host, $port);
if ($params{install_path})
{
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 92d9303e23..f5d2ba72e6 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -2761,4 +2761,16 @@ sub corrupt_page_checksum
=cut
+# support release 15+ perl module namespace
+
+package PostgreSQL::Test::Cluster;
+
+sub new
+{
+ shift; # remove class param from args
+ return PostgresNode->get_new_node(@_);
+}
+
+sub get_free_port { return PostgresNode::get_free_port(); }
+
1;
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 0dfc414b07..92583f84b4 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -948,4 +948,46 @@ sub command_checks_all
=cut
+# support release 15+ perl module namespace
+
+package PostgreSQL::Test::Utils;
+
+# we don't want to export anything, but we want to support things called
+# via this package name explicitly.
+
+# use typeglobs to alias these functions and variables
+
+*generate_ascii_string = *TestLib::generate_ascii_string;
+*slurp_dir = *TestLib::slurp_dir;
+*slurp_file = *TestLib::slurp_file;
+*append_to_file = *TestLib::append_to_file;
+*check_mode_recursive = *TestLib::check_mode_recursive;
+*chmod_recursive = *TestLib::chmod_recursive;
+*check_pg_config = *TestLib::check_pg_config;
+*dir_symlink = *TestLib::dir_symlink;
+*system_or_bail = *TestLib::system_or_bail;
+*system_log = *TestLib::system_log;
+*run_log = *TestLib::run_log;
+*run_command = *TestLib::run_command;
+sub pump_until { die "pump_until not implemented in TestLib"; }
+*command_ok = *TestLib::command_ok;
+*command_fails = *TestLib::command_fails;
+*command_exit_is = *TestLib::command_exit_is;
+*program_help_ok = *TestLib::program_help_ok;
+*program_version_ok = *TestLib::program_version_ok;
+*program_options_handling_ok =
Re: Postgres perl module namespace
> On 18 Apr 2022, at 19:59, Andrew Dunstan wrote: > On 2022-04-18 Mo 13:43, Tom Lane wrote: >> Another thing that ought to be on the table is back-patching >> 549ec201d (Replace Test::More plans with done_testing). Those >> test counts are an even huger pain for back-patching than the >> renaming, because the count is often different in each branch. >> > > +1 for doing that TI'll get to work on that then. -- Daniel Gustafsson https://vmware.com/
Re: Postgres perl module namespace
On Mon, Apr 18, 2022 at 01:59:23PM -0400, Andrew Dunstan wrote: > On 2022-04-18 Mo 13:43, Tom Lane wrote: >> I doubt that just plopping the new Cluster.pm in alongside the old >> file could work --- wouldn't the two modules need to share state >> somehow? > > No, I think we could probably just port the whole of src/test/PostreSQL > back if required, and have it live alongside the old modules. Each TAP > test is a separate miracle - see comments elsewhere about port > assignment in parallel TAP tests. Doesn't that mean doubling the maintenance cost if any of the internal module routines are changed? If the existing in-core TAP tests use one module or the other exclusively, how do we make easily sure that one and the other base modules are not broken? There are also out-of-tree TAP tests relying on those modules, though having everything in parallel would work. >> Another thing that ought to be on the table is back-patching >> 549ec201d (Replace Test::More plans with done_testing). Those >> test counts are an even huger pain for back-patching than the >> renaming, because the count is often different in each branch. > > +1 for doing that The harcoded number of tests has been the most annoying part for me, to be honest, while the namespace change just requires a few seds and it is a matter of getting used to it. FWIW, I have a git script that does the same thing as Noah, but only for files part of the code tree, as of: for file in $(git grep -l "$OLDSTR") do sed -i "s/$OLDSTR/$NEWSTR/g" "$file" done -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
> On Apr 18, 2022, at 1:19 PM, Andrew Dunstan wrote: > > that seems quite separate from the present issue. Thanks for the clarification. I agree, given your comments, that it is unrelated to this thread. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Postgres perl module namespace
On 2022-04-18 Mo 15:46, Mark Dilger wrote: > >> On Apr 18, 2022, at 10:59 AM, Andrew Dunstan wrote: >> >> No, I think we could probably just port the whole of src/test/PostreSQL >> back if required, and have it live alongside the old modules. Each TAP >> test is a separate miracle - see comments elsewhere about port >> assignment in parallel TAP tests. > I think $last_port_assigned would need to be shared between the two modules. > This global safeguard is already a bit buggy, but not sharing it between > modules would be far worse. Currently, if a node which has a port assigned > is stopped, and a parallel test creates a new node, this global variable > prevents the new node from getting the port already assigned to the old > stopped node, except when port assignment wraps around. Without sharing the > global, wrap-around need not happen for port collisions. > > Or am I reading the code wrong? > I don't see anything at all in the current code that involves sharing $last_port_assigned (or anything else) between parallel tests. The only reason we don't get lots of collisions there is because each one starts off at a random port. If you want it shared to guarantee non-collision we will need far more infrastructure, AFAICS, but that seems quite separate from the present issue. I have some experience of managing that - the buildfarm code has some shared state, protected by bunch of locks. To the best of my knowledge. each TAP test runs in its own process, a child of prove. And that's just as well because we certainly wouldn't want other package globals (like the node list) shared. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
> On Apr 18, 2022, at 10:59 AM, Andrew Dunstan wrote: > > No, I think we could probably just port the whole of src/test/PostreSQL > back if required, and have it live alongside the old modules. Each TAP > test is a separate miracle - see comments elsewhere about port > assignment in parallel TAP tests. I think $last_port_assigned would need to be shared between the two modules. This global safeguard is already a bit buggy, but not sharing it between modules would be far worse. Currently, if a node which has a port assigned is stopped, and a parallel test creates a new node, this global variable prevents the new node from getting the port already assigned to the old stopped node, except when port assignment wraps around. Without sharing the global, wrap-around need not happen for port collisions. Or am I reading the code wrong? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Postgres perl module namespace
On 2022-04-18 Mo 14:07, Tom Lane wrote: > Andrew Dunstan writes: >> No, I think we could probably just port the whole of src/test/PostreSQL >> back if required, and have it live alongside the old modules. Each TAP >> test is a separate miracle - see comments elsewhere about port >> assignment in parallel TAP tests. >> But that would mean we have some tests in the old flavor and some in the >> new flavor in the back branches, which might get confusing. > That works for back-patching entire new test scripts, but not for adding > some cases to an existing script, which I think is more common. > > I think the only thing that should trip people up in those cases is the the new/get_new_node thing. That's complicated by the fact that the old PostgresNode module has both new() and get_new_node(), although it advises people not to use its new(). Probably the best way around that is a) rename it's new() and deal with any callers, and b) add a new new(), which would be a wrapper around get_new_node(). I'll have a play with that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
Andrew Dunstan writes: > No, I think we could probably just port the whole of src/test/PostreSQL > back if required, and have it live alongside the old modules. Each TAP > test is a separate miracle - see comments elsewhere about port > assignment in parallel TAP tests. > But that would mean we have some tests in the old flavor and some in the > new flavor in the back branches, which might get confusing. That works for back-patching entire new test scripts, but not for adding some cases to an existing script, which I think is more common. regards, tom lane
Re: Postgres perl module namespace
On 2022-04-18 Mo 13:43, Tom Lane wrote: > Andrew Dunstan writes: >> On 2022-04-18 Mo 11:52, Noah Misch wrote: >>> On Mon, Apr 18, 2022 at 07:15:30AM -0700, Andres Freund wrote: I just, again, tried to backport a test as part of a bugfix. The renaming between 14 and 15 makes that task almost comically harder. The only way I see of dealing with that for the next 5 years is to just never backpatch tests to < 15. Which seems like a bad outcome. >> I'm not sure how often we do things like that. But I don't agree it's >> impossibly hard, although I can see it might be a bit annoying. > I think we back-patch test cases *all the time*. So I think Andres > is quite right to be concerned about making that harder, although I'm > not sure that his estimate of the conversion difficulty is accurate. > I plan to keep a copy of Noah's script and see if applying that to > the patch files alleviates the pain. In a few months we should have > a better idea of whether that's sufficient, or we want to go to the > work of back-patching the renaming. > > I doubt that just plopping the new Cluster.pm in alongside the old > file could work --- wouldn't the two modules need to share state > somehow? No, I think we could probably just port the whole of src/test/PostreSQL back if required, and have it live alongside the old modules. Each TAP test is a separate miracle - see comments elsewhere about port assignment in parallel TAP tests. But that would mean we have some tests in the old flavor and some in the new flavor in the back branches, which might get confusing. > > Another thing that ought to be on the table is back-patching > 549ec201d (Replace Test::More plans with done_testing). Those > test counts are an even huger pain for back-patching than the > renaming, because the count is often different in each branch. > > +1 for doing that cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
Andrew Dunstan writes: > On 2022-04-18 Mo 11:52, Noah Misch wrote: >> On Mon, Apr 18, 2022 at 07:15:30AM -0700, Andres Freund wrote: >>> I just, again, tried to backport a test as part of a bugfix. The >>> renaming between 14 and 15 makes that task almost comically harder. The >>> only way I see of dealing with that for the next 5 years is to just >>> never backpatch tests to < 15. Which seems like a bad outcome. > I'm not sure how often we do things like that. But I don't agree it's > impossibly hard, although I can see it might be a bit annoying. I think we back-patch test cases *all the time*. So I think Andres is quite right to be concerned about making that harder, although I'm not sure that his estimate of the conversion difficulty is accurate. I plan to keep a copy of Noah's script and see if applying that to the patch files alleviates the pain. In a few months we should have a better idea of whether that's sufficient, or we want to go to the work of back-patching the renaming. I doubt that just plopping the new Cluster.pm in alongside the old file could work --- wouldn't the two modules need to share state somehow? Another thing that ought to be on the table is back-patching 549ec201d (Replace Test::More plans with done_testing). Those test counts are an even huger pain for back-patching than the renaming, because the count is often different in each branch. regards, tom lane
Re: Postgres perl module namespace
On 2022-04-18 Mo 11:52, Noah Misch wrote: > On Mon, Apr 18, 2022 at 07:15:30AM -0700, Andres Freund wrote: >> I just, again, tried to backport a test as part of a bugfix. The >> renaming between 14 and 15 makes that task almost comically harder. The >> only way I see of dealing with that for the next 5 years is to just >> never backpatch tests to < 15. Which seems like a bad outcome. I'm not sure how often we do things like that. But I don't agree it's impossibly hard, although I can see it might be a bit annoying. > For what it's worth, to back-patch TAP suite changes, I've been using this > script (works on a .p[lm] file or on a patch file): > > bin/tap15to14 > #! /bin/sh > > # This translates a PostgreSQL 15 TAP test into a PostgreSQL 14 TAP test > > sed -i~ ' > s/PostgreSQL::Test::Cluster/PostgresNode/g > s/PostgreSQL::Test::Utils/TestLib/g > s/PostgresNode->new/get_new_node/g > ' -- "$@" > > grep -w subtest -- "$@" > > Yeah, that should take care of most of it. >> Except that it's *way* too late I would argue that this should just >> straight up be reverted until that aspect is addressed. It's a >> maintenance nightmare. > I do feel PostgreSQL has been over-eager to do cosmetic refactoring. For me, > this particular one has been sort-of-tolerable. There were reasons beyond being purely cosmetic for all the changes. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Mon, Apr 18, 2022 at 07:15:30AM -0700, Andres Freund wrote: > I just, again, tried to backport a test as part of a bugfix. The > renaming between 14 and 15 makes that task almost comically harder. The > only way I see of dealing with that for the next 5 years is to just > never backpatch tests to < 15. Which seems like a bad outcome. For what it's worth, to back-patch TAP suite changes, I've been using this script (works on a .p[lm] file or on a patch file): bin/tap15to14 #! /bin/sh # This translates a PostgreSQL 15 TAP test into a PostgreSQL 14 TAP test sed -i~ ' s/PostgreSQL::Test::Cluster/PostgresNode/g s/PostgreSQL::Test::Utils/TestLib/g s/PostgresNode->new/get_new_node/g ' -- "$@" grep -w subtest -- "$@" > Except that it's *way* too late I would argue that this should just > straight up be reverted until that aspect is addressed. It's a > maintenance nightmare. I do feel PostgreSQL has been over-eager to do cosmetic refactoring. For me, this particular one has been sort-of-tolerable.
Re: Postgres perl module namespace
Hi, On 2022-04-18 10:26:15 -0400, Tom Lane wrote: > Andres Freund writes: > > I just, again, tried to backport a test as part of a bugfix. The > > renaming between 14 and 15 makes that task almost comically harder. The > > only way I see of dealing with that for the next 5 years is to just > > never backpatch tests to < 15. Which seems like a bad outcome. > > Yeah ... > > > Except that it's *way* too late I would argue that this should just > > straight up be reverted until that aspect is addressed. It's a > > maintenance nightmare. > > I'm not for that I'm not either, at this point... > but could it be sane to back-patch the renaming? That might be the best. But it might not even suffice. There've been other global refactorings between 14 and 15. E.g. 201a76183e2. I wonder if we should just backpatch the current PostgreSQL module, but leave the old stuff around :/. Greetings, Andres Freund
Re: Postgres perl module namespace
Andres Freund writes: > I just, again, tried to backport a test as part of a bugfix. The > renaming between 14 and 15 makes that task almost comically harder. The > only way I see of dealing with that for the next 5 years is to just > never backpatch tests to < 15. Which seems like a bad outcome. Yeah ... > Except that it's *way* too late I would argue that this should just > straight up be reverted until that aspect is addressed. It's a > maintenance nightmare. I'm not for that, but could it be sane to back-patch the renaming? regards, tom lane
Re: Postgres perl module namespace
Hi, On 2021-10-25 17:12:08 +0900, Michael Paquier wrote: > On Sun, Oct 24, 2021 at 10:46:30AM -0400, Andrew Dunstan wrote: > > ... and pushed. > > Thanks! I just, again, tried to backport a test as part of a bugfix. The renaming between 14 and 15 makes that task almost comically harder. The only way I see of dealing with that for the next 5 years is to just never backpatch tests to < 15. Which seems like a bad outcome. I just read through the thread and didn't really see this aspect discussed - which I find surprising. Except that it's *way* too late I would argue that this should just straight up be reverted until that aspect is addressed. It's a maintenance nightmare. - Andres
Re: Postgres perl module namespace
On Sun, Oct 24, 2021 at 10:46:30AM -0400, Andrew Dunstan wrote: > ... and pushed. Thanks! -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On 10/20/21 08:40, Andrew Dunstan wrote:
> On 10/19/21 11:22 PM, Michael Paquier wrote:
>> On Tue, Oct 19, 2021 at 10:16:06PM +0200, Erik Rijkers wrote:
[0001-move-perl-test-modules-to-PostgreSQL-Test-namespace.patch ]
[0002-move-PostgreSQL-Test-PostgresVersion-up-in-the-names.patch]
>> It seems to me that the hardest part is sorted out with the naming and
>> pathing of the modules, so better to apply them sooner than later.
>
> Yeah, my plan is to apply it today or tomorrow
>
>
>>> Those patches gave some complains about PostgreSQL/Test/PostgresVersion.pm
>>> being absent so I added this deletion. I'm not sure that's correct but it
>>> enabled the build and check-world ran without errors.
>> Your change is incorrect, as we want to install PostgresVersion.pm.
>> What's needed here is the following:
>> {PostgresVersion.pm => PostgreSQL/Version.pm}
>>
>> And so the patch needs to be changed like that:
>> - $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/PostgresVersion.pm
>> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
>> + $(INSTALL_DATA) $(srcdir)/PostgreSQL/Version.pm
>> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
>> [...]
>> - rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
>> + rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
> right. There are one or two other cosmetic changes too.
>
>
... and pushed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On 10/19/21 11:22 PM, Michael Paquier wrote:
> On Tue, Oct 19, 2021 at 10:16:06PM +0200, Erik Rijkers wrote:
>>> [0001-move-perl-test-modules-to-PostgreSQL-Test-namespace.patch ]
>>> [0002-move-PostgreSQL-Test-PostgresVersion-up-in-the-names.patch]
> It seems to me that the hardest part is sorted out with the naming and
> pathing of the modules, so better to apply them sooner than later.
Yeah, my plan is to apply it today or tomorrow
>
>> Those patches gave some complains about PostgreSQL/Test/PostgresVersion.pm
>> being absent so I added this deletion. I'm not sure that's correct but it
>> enabled the build and check-world ran without errors.
> Your change is incorrect, as we want to install PostgresVersion.pm.
> What's needed here is the following:
> {PostgresVersion.pm => PostgreSQL/Version.pm}
>
> And so the patch needs to be changed like that:
> - $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/PostgresVersion.pm
> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
> + $(INSTALL_DATA) $(srcdir)/PostgreSQL/Version.pm
> '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
> [...]
> - rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
> + rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
right. There are one or two other cosmetic changes too.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Tue, Oct 19, 2021 at 10:16:06PM +0200, Erik Rijkers wrote:
>> [0001-move-perl-test-modules-to-PostgreSQL-Test-namespace.patch ]
>> [0002-move-PostgreSQL-Test-PostgresVersion-up-in-the-names.patch]
It seems to me that the hardest part is sorted out with the naming and
pathing of the modules, so better to apply them sooner than later.
> Those patches gave some complains about PostgreSQL/Test/PostgresVersion.pm
> being absent so I added this deletion. I'm not sure that's correct but it
> enabled the build and check-world ran without errors.
Your change is incorrect, as we want to install PostgresVersion.pm.
What's needed here is the following:
{PostgresVersion.pm => PostgreSQL/Version.pm}
And so the patch needs to be changed like that:
- $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/PostgresVersion.pm
'$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
+ $(INSTALL_DATA) $(srcdir)/PostgreSQL/Version.pm
'$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
[...]
- rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm'
+ rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Version.pm'
--
Michael
signature.asc
Description: PGP signature
Re: Postgres perl module namespace
Op 19-10-2021 om 20:54 schreef Andrew Dunstan: Discussion has gone quiet and the tree is now relatively quiet, so now seems like a good time to do this. See attached patches. > [0001-move-perl-test-modules-to-PostgreSQL-Test-namespace.patch ] > [0002-move-PostgreSQL-Test-PostgresVersion-up-in-the-names.patch] Those patches gave some complains about PostgreSQL/Test/PostgresVersion.pm being absent so I added this deletion. I'm not sure that's correct but it enabled the build and check-world ran without errors. Erik Rijkers --- src/test/perl/Makefile.orig2 2021-10-19 21:40:38.388116778 +0200 +++ src/test/perl/Makefile 2021-10-19 21:40:52.208346619 +0200 @@ -23,13 +23,11 @@ $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/SimpleTee.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/SimpleTee.pm' $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/RecursiveCopy.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/RecursiveCopy.pm' $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/Cluster.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/Cluster.pm' - $(INSTALL_DATA) $(srcdir)/PostgreSQL/Test/PostgresVersion.pm '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm' uninstall: rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/Utils.pm' rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/SimpleTee.pm' rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/RecursiveCopy.pm' rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/Cluster.pm' - rm -f '$(DESTDIR)$(pgxsdir)/$(subdir)/PostgreSQL/Test/PostgresVersion.pm' endif
Re: Postgres perl module namespace
> On Sep 7, 2021, at 9:00 PM, Noah Misch wrote: > > I wondered about using PGXS:: as the namespace for all these modules That immediately suggests perl modules wrapping C code, which is misleading for these. See `man perlxstut` — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Postgres perl module namespace
On Tue, Sep 07, 2021 at 07:43:47AM -0400, Andrew Dunstan wrote: > On 9/6/21 1:08 AM, Michael Paquier wrote: > > On Sat, Sep 04, 2021 at 09:58:08AM -0400, Andrew Dunstan wrote: > >> On 9/4/21 2:19 AM, Noah Misch wrote: > >>> plperl uses PostgreSQL:: as the first component of its Perl module > >>> namespace. > >>> We shouldn't use both PostgreSQL:: and Postgres:: in the same source > >>> tree, so > >>> this change should not use Postgres::. > >> Good point. Here's the same thing using PostgreSQL::Test > > A minor point: this introduces PostgreSQL::Test::PostgresVersion. > > Could be this stripped down to PostgreSQL::Test::Version instead? > > That name isn't very clear - what is it the version of, PostgreSQL or > the test? Fair. > There's nothing very test-specific about this module - it simply > encapsulates a Postgres version string. So maybe it should just be > PostgreSQL::Version. Could be fine, but that name could be useful as a CPAN module. These modules don't belong on CPAN, so I'd keep PostgreSQL::Test::PostgresVersion. There's only one reference in the tree, so optimizing that particular name is less exciting. (I wondered about using PGXS:: as the namespace for all these modules, since it's short and "pgxs" is the closest thing to a name for the PostgreSQL build system. Overall, I didn't convince myself about it being an improvement.)
Re: Postgres perl module namespace
On 9/6/21 1:08 AM, Michael Paquier wrote: > On Sat, Sep 04, 2021 at 09:58:08AM -0400, Andrew Dunstan wrote: >> On 9/4/21 2:19 AM, Noah Misch wrote: >>> plperl uses PostgreSQL:: as the first component of its Perl module >>> namespace. >>> We shouldn't use both PostgreSQL:: and Postgres:: in the same source tree, >>> so >>> this change should not use Postgres::. >> Good point. Here's the same thing using PostgreSQL::Test > A minor point: this introduces PostgreSQL::Test::PostgresVersion. > Could be this stripped down to PostgreSQL::Test::Version instead? That name isn't very clear - what is it the version of, PostgreSQL or the test? There's nothing very test-specific about this module - it simply encapsulates a Postgres version string. So maybe it should just be PostgreSQL::Version. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Mon, Sep 06, 2021 at 02:08:45PM +0900, Michael Paquier wrote: > A minor point: this introduces PostgreSQL::Test::PostgresVersion. > Could be this stripped down to PostgreSQL::Test::Version instead? This fails to apply since 5fcb23c, but the conflicts are simple enough to solve. Sorry about that :/ -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On Sat, Sep 04, 2021 at 09:58:08AM -0400, Andrew Dunstan wrote: > On 9/4/21 2:19 AM, Noah Misch wrote: >> plperl uses PostgreSQL:: as the first component of its Perl module namespace. >> We shouldn't use both PostgreSQL:: and Postgres:: in the same source tree, so >> this change should not use Postgres::. > > Good point. Here's the same thing using PostgreSQL::Test A minor point: this introduces PostgreSQL::Test::PostgresVersion. Could be this stripped down to PostgreSQL::Test::Version instead? -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On Fri, Sep 03, 2021 at 03:34:24PM -0400, Andrew Dunstan wrote:
> On 8/25/21 10:08 AM, Robert Haas wrote:
> > On Wed, Aug 25, 2021 at 1:48 AM Michael Paquier wrote:
> >> On Mon, Aug 23, 2021 at 03:39:15PM -0400, Robert Haas wrote:
> >>> On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan
> >>> wrote:
> OK, I count 3 in favor of changing to PgTest::Cluster, 1 against,
> remainder don't care.
> >>> I'd have gone with something starting with Postgres:: ... but I don't
> >>> care much.
> >> PgTest seems like a better choice to me, as "Postgres" could be used
> >> for other purposes than a testing framework, and the argument that
> >> multiple paths makes things annoying for hackers is sensible.
> > I mean, it's a hierarchical namespace. The idea is you do
> > Postgres::Test or Postgres:: and other people using the
> > Postgres database can use other parts of it. But again, not really
> > worth arguing about.
>
> I think I have come around to this POV. Here's a patch. The worst of it
> is changes like this:
>
> - my $node2 = PostgresNode->new('replica');
> + my $node2 = Postgres::Test::Cluster->new('replica');
> ...
> - TestLib::system_or_bail($tar, 'xf', $tblspc_tars[0], '-C', $repTsDir);
> + Postgres::Test::Utils::system_or_bail($tar, 'xf', $tblspc_tars[0], '-C',
> $repTsDir);
plperl uses PostgreSQL:: as the first component of its Perl module namespace.
We shouldn't use both PostgreSQL:: and Postgres:: in the same source tree, so
this change should not use Postgres::.
Re: Postgres perl module namespace
On Wed, Aug 25, 2021 at 1:48 AM Michael Paquier wrote: > On Mon, Aug 23, 2021 at 03:39:15PM -0400, Robert Haas wrote: > > On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan wrote: > >> OK, I count 3 in favor of changing to PgTest::Cluster, 1 against, > >> remainder don't care. > > > > I'd have gone with something starting with Postgres:: ... but I don't care > > much. > > PgTest seems like a better choice to me, as "Postgres" could be used > for other purposes than a testing framework, and the argument that > multiple paths makes things annoying for hackers is sensible. I mean, it's a hierarchical namespace. The idea is you do Postgres::Test or Postgres:: and other people using the Postgres database can use other parts of it. But again, not really worth arguing about. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Postgres perl module namespace
On Mon, Aug 23, 2021 at 03:39:15PM -0400, Robert Haas wrote: > On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan wrote: >> OK, I count 3 in favor of changing to PgTest::Cluster, 1 against, >> remainder don't care. > > I'd have gone with something starting with Postgres:: ... but I don't care > much. PgTest seems like a better choice to me, as "Postgres" could be used for other purposes than a testing framework, and the argument that multiple paths makes things annoying for hackers is sensible. -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan wrote: > OK, I count 3 in favor of changing to PgTest::Cluster, 1 against, > remainder don't care. I'd have gone with something starting with Postgres:: ... but I don't care much. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Postgres perl module namespace
On 8/11/21 9:30 AM, Tom Lane wrote:
> Alvaro Herrera writes:
>> I'll recast my vote to make this line be
>> my $node = PgTest::Cluster->new('nodename');
>> which seems succint enough. I could get behind PgTest::PgCluster too,
>> but having a second Pg there seems unnecessary.
> Either of those WFM.
>
>
OK, I count 3 in favor of changing to PgTest::Cluster, 1 against,
remainder don't care.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
Alvaro Herrera writes:
> I'll recast my vote to make this line be
> my $node = PgTest::Cluster->new('nodename');
> which seems succint enough. I could get behind PgTest::PgCluster too,
> but having a second Pg there seems unnecessary.
Either of those WFM.
regards, tom lane
Re: Postgres perl module namespace
On 2021-Aug-10, Andrew Dunstan wrote:
> If we were publishing them on CPAN that would be reasonable. But we're
> not, nor are we likely to, I believe. I'd rather not have to add two
> level of directory hierarchy for this, and this also seems a bit
> long-winded:
>
> my $node = PostgreSQL::Test::Cluster->new('nodename');
I'll recast my vote to make this line be
my $node = PgTest::Cluster->new('nodename');
which seems succint enough. I could get behind PgTest::PgCluster too,
but having a second Pg there seems unnecessary.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"Cada quien es cada cual y baja las escaleras como quiere" (JMSerrat)
Re: Postgres perl module namespace
On 8/10/21 10:26 PM, Andrew Dunstan wrote: > On 8/10/21 10:13 PM, Mark Dilger wrote: >>> On Aug 10, 2021, at 7:11 PM, Andrew Dunstan wrote: >>> >>> If we were publishing them on CPAN that would be reasonable. But we're >>> not, nor are we likely to, I believe. >> I'm now trying to understand the purpose of the renaming. I thought the >> problem was that RPM packagers wanted something that was unlikely to >> collide. Publishing on CPAN would be the way to claim the namespace. >> >> What's the purpose of this idea then? If there isn't one, I'd rather just >> keep the current names. > > > Yes we want them to be in a namespace where they are unlikely to collide > with anything else. No, you don't have to publish on CPAN to achieve that. > Incidentally, not publishing on CPAN was a major reason given a few years ago for using fairly lax perlcritic policies. If we publish these on CPAN now some people at least might want to revisit that decision. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On 8/10/21 10:13 PM, Mark Dilger wrote: > >> On Aug 10, 2021, at 7:11 PM, Andrew Dunstan wrote: >> >> If we were publishing them on CPAN that would be reasonable. But we're >> not, nor are we likely to, I believe. > I'm now trying to understand the purpose of the renaming. I thought the > problem was that RPM packagers wanted something that was unlikely to collide. > Publishing on CPAN would be the way to claim the namespace. > > What's the purpose of this idea then? If there isn't one, I'd rather just > keep the current names. Yes we want them to be in a namespace where they are unlikely to collide with anything else. No, you don't have to publish on CPAN to achieve that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On 8/10/21 9:25 PM, Michael Paquier wrote: > On Tue, Aug 10, 2021 at 11:02:13AM -0400, Andrew Dunstan wrote: >> I can live with that. I guess to be consistent we would also rename >> PostgresVersion to PgVersion > Are RewindTest.pm and SSLServer.pm things you are considering for a > renaming as well? It may be more consistent to put everything in the > same namespace if this move is done. It could be very easily done. But I doubt these will make their way into packages, which is how this discussion started. There's good reason to package src/test/perl, so you can use those modules to write TAP tests for extensions. The same reasoning doesn't apply to SSLServer.pm and RewindTest.pm. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
> On Aug 10, 2021, at 7:11 PM, Andrew Dunstan wrote: > > If we were publishing them on CPAN that would be reasonable. But we're > not, nor are we likely to, I believe. I'm now trying to understand the purpose of the renaming. I thought the problem was that RPM packagers wanted something that was unlikely to collide. Publishing on CPAN would be the way to claim the namespace. What's the purpose of this idea then? If there isn't one, I'd rather just keep the current names. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Postgres perl module namespace
On 8/10/21 9:37 PM, Mark Dilger wrote:
>
>> On Aug 10, 2021, at 7:10 AM, Andrew Dunstan wrote:
>>
>> use PgTest::Utils;
>> use PgTest::PostgresNode;
> Checking CPAN, it seems there are three older modules with names starting
> with "Postgres":
>
> Postgres
> Postgres::Handler
> Postgres::Handler::HTML
>
> It would be confusing to combine official PostgreSQL modules with those third
> party ones, so perhaps we can claim the PostgreSQL namespace for official
> project modules. How about:
>
> PostgreSQL::Test::Cluster
> PostgreSQL::Test::Lib
> PostgreSQL::Test::Utils
>
> and then if we ever wanted to have official packages for non-test purposes,
> we could start another namespace under PostgreSQL.
>
If we were publishing them on CPAN that would be reasonable. But we're
not, nor are we likely to, I believe. I'd rather not have to add two
level of directory hierarchy for this, and this also seems a bit
long-winded:
my $node = PostgreSQL::Test::Cluster->new('nodename');
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On Wed, Aug 11, 2021 at 9:37 AM Mark Dilger wrote: > > > On Aug 10, 2021, at 7:10 AM, Andrew Dunstan wrote: > > > > use PgTest::Utils; > > use PgTest::PostgresNode; > > Checking CPAN, it seems there are three older modules with names starting > with "Postgres": > > Postgres > Postgres::Handler > Postgres::Handler::HTML > > It would be confusing to combine official PostgreSQL modules with those third > party ones, so perhaps we can claim the PostgreSQL namespace for official > project modules. How about: > > PostgreSQL::Test::Cluster > PostgreSQL::Test::Lib > PostgreSQL::Test::Utils > > and then if we ever wanted to have official packages for non-test purposes, > we could start another namespace under PostgreSQL. Maybe it's me but I would find that more confusing. Also, to actually claim PostgreSQL namespace, we would have to actually publish them on CPAN right?
Re: Postgres perl module namespace
> On Aug 10, 2021, at 7:10 AM, Andrew Dunstan wrote: > > use PgTest::Utils; > use PgTest::PostgresNode; Checking CPAN, it seems there are three older modules with names starting with "Postgres": Postgres Postgres::Handler Postgres::Handler::HTML It would be confusing to combine official PostgreSQL modules with those third party ones, so perhaps we can claim the PostgreSQL namespace for official project modules. How about: PostgreSQL::Test::Cluster PostgreSQL::Test::Lib PostgreSQL::Test::Utils and then if we ever wanted to have official packages for non-test purposes, we could start another namespace under PostgreSQL. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Postgres perl module namespace
On Tue, Aug 10, 2021 at 11:02:13AM -0400, Andrew Dunstan wrote: > I can live with that. I guess to be consistent we would also rename > PostgresVersion to PgVersion Are RewindTest.pm and SSLServer.pm things you are considering for a renaming as well? It may be more consistent to put everything in the same namespace if this move is done. -- Michael signature.asc Description: PGP signature
Re: Postgres perl module namespace
On 8/10/21 10:40 AM, Tom Lane wrote: > Andrew Dunstan writes: >> I will undertake to do the work, once we get the bike-shedding part done. >> I'll kick that off by suggesting we move all of these to the namespace >> PgTest, and rename TestLib to Utils, so instead of >> use TestLib; >> use PostgresNode; >> you would say >> use PgTest::Utils; >> use PgTest::PostgresNode; > Using both "Pg" and "Postgres" seems a bit inconsistent. > Maybe "PgTest::PgNode"? > > More generally, I've never thought that "Node" was a great name > here; it's a very vague term. While we're renaming, maybe we > could change it to "PgTest::PgCluster" or the like? > > I can live with that. I guess to be consistent we would also rename PostgresVersion to PgVersion cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
On 2021-Aug-10, Andrew Dunstan wrote: > I'll kick that off by suggesting we move all of these to the namespace > PgTest, and rename TestLib to Utils, so [...] you would say > > use PgTest::Utils; > use PgTest::PostgresNode; WFM. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Hay que recordar que la existencia en el cosmos, y particularmente la elaboración de civilizaciones dentro de él no son, por desgracia, nada idílicas" (Ijon Tichy)
Re: Postgres perl module namespace
Andrew Dunstan writes: > I will undertake to do the work, once we get the bike-shedding part done. > I'll kick that off by suggesting we move all of these to the namespace > PgTest, and rename TestLib to Utils, so instead of > use TestLib; > use PostgresNode; > you would say > use PgTest::Utils; > use PgTest::PostgresNode; Using both "Pg" and "Postgres" seems a bit inconsistent. Maybe "PgTest::PgNode"? More generally, I've never thought that "Node" was a great name here; it's a very vague term. While we're renaming, maybe we could change it to "PgTest::PgCluster" or the like? regards, tom lane
Re: Postgres perl module namespace
On 5/20/21 5:18 PM, Tom Lane wrote: > Andrew Dunstan writes: >> While solving a problem with the Beta RPMs, I noticed that they export >> our perl test modules as capabilities like this: >> [andrew@f34 x86_64]$ rpm -q --provides -p >> postgresql14-devel-14-beta1_PGDG.fc34.x86_64.rpm | grep ^perl >> perl(PostgresNode) >> perl(PostgresVersion) >> perl(RecursiveCopy) >> perl(SimpleTee) >> perl(TestLib) >> I don't think we should be putting this stuff in a global namespace like >> that. We should invent a namespace that's not likely to conflict with >> other people, like, say, 'PostgreSQL::Test' to put these modules. That >> would require moving some code around and adjusting a bunch of scripts, >> but it would not be difficult. Maybe something to be done post-14? > Agreed that we ought to namespace these better. It looks to me like most > of these are several versions old. Given the lack of field complaints, > I'm content to wait for v15 for a fix, rather than treating it as an open > item for v14. So now the dev tree is open for v15 it's time to get back to this item. I will undertake to do the work, once we get the bike-shedding part done. I'll kick that off by suggesting we move all of these to the namespace PgTest, and rename TestLib to Utils, so instead of use TestLib; use PostgresNode; you would say use PgTest::Utils; use PgTest::PostgresNode; cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Postgres perl module namespace
Hi, On Thu, 2021-05-20 at 15:47 -0400, Andrew Dunstan wrote: > Meanwhile I would suggest that RPM maintainers exclude both requires > and provides for these five names. Done, thanks. Will appear in next beta build. Regards, -- Devrim Gündüz Open Source Solution Architect, Red Hat Certified Engineer Twitter: @DevrimGunduz , @DevrimGunduzTR signature.asc Description: This is a digitally signed message part
Re: Postgres perl module namespace
Andrew Dunstan writes: > While solving a problem with the Beta RPMs, I noticed that they export > our perl test modules as capabilities like this: > [andrew@f34 x86_64]$ rpm -q --provides -p > postgresql14-devel-14-beta1_PGDG.fc34.x86_64.rpm | grep ^perl > perl(PostgresNode) > perl(PostgresVersion) > perl(RecursiveCopy) > perl(SimpleTee) > perl(TestLib) > I don't think we should be putting this stuff in a global namespace like > that. We should invent a namespace that's not likely to conflict with > other people, like, say, 'PostgreSQL::Test' to put these modules. That > would require moving some code around and adjusting a bunch of scripts, > but it would not be difficult. Maybe something to be done post-14? Agreed that we ought to namespace these better. It looks to me like most of these are several versions old. Given the lack of field complaints, I'm content to wait for v15 for a fix, rather than treating it as an open item for v14. > Meanwhile I would suggest that RPM maintainers exclude both requires and > provides for these five names. +1 regards, tom lane
Postgres perl module namespace
While solving a problem with the Beta RPMs, I noticed that they export our perl test modules as capabilities like this: [andrew@f34 x86_64]$ rpm -q --provides -p postgresql14-devel-14-beta1_PGDG.fc34.x86_64.rpm | grep ^perl perl(PostgresNode) perl(PostgresVersion) perl(RecursiveCopy) perl(SimpleTee) perl(TestLib) I don't think we should be putting this stuff in a global namespace like that. We should invent a namespace that's not likely to conflict with other people, like, say, 'PostgreSQL::Test' to put these modules. That would require moving some code around and adjusting a bunch of scripts, but it would not be difficult. Maybe something to be done post-14? Meanwhile I would suggest that RPM maintainers exclude both requires and provides for these five names. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
