Thread: 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
Andrew Dunstan <andrew@dunslane.net> 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
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
Attachment
On 5/20/21 5:18 PM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> 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
Andrew Dunstan <andrew@dunslane.net> 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
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)
On 8/10/21 10:40 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> 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
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
Attachment
> On Aug 10, 2021, at 7:10 AM, Andrew Dunstan <andrew@dunslane.net> 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
PostgreSQLnamespace 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
On Wed, Aug 11, 2021 at 9:37 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > On Aug 10, 2021, at 7:10 AM, Andrew Dunstan <andrew@dunslane.net> 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 PostgreSQLnamespace 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?
On 8/10/21 9:37 PM, Mark Dilger wrote:
>
>> On Aug 10, 2021, at 7:10 AM, Andrew Dunstan <andrew@dunslane.net> 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
PostgreSQLnamespace 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
> On Aug 10, 2021, at 7:11 PM, Andrew Dunstan <andrew@dunslane.net> 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 somethingthat 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
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
On 8/10/21 10:13 PM, Mark Dilger wrote: > >> On Aug 10, 2021, at 7:11 PM, Andrew Dunstan <andrew@dunslane.net> 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 somethingthat 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
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 <andrew@dunslane.net> 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 somethingthat 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
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)
Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On 8/11/21 9:30 AM, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> 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
On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan <andrew@dunslane.net> 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
On Mon, Aug 23, 2021 at 03:39:15PM -0400, Robert Haas wrote: > On Mon, Aug 23, 2021 at 3:03 PM Andrew Dunstan <andrew@dunslane.net> 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
Attachment
On Wed, Aug 25, 2021 at 1:48 AM Michael Paquier <michael@paquier.xyz> 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 <andrew@dunslane.net> 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::<whatever> 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
On 8/25/21 10:08 AM, Robert Haas wrote:
> On Wed, Aug 25, 2021 at 1:48 AM Michael Paquier <michael@paquier.xyz> 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 <andrew@dunslane.net> 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::<whatever> 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);
and I think that's not so bad.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment
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 <michael@paquier.xyz> 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 <andrew@dunslane.net> 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::<whatever> 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::.
On 9/4/21 2:19 AM, Noah Misch wrote:
> 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 <michael@paquier.xyz> 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 <andrew@dunslane.net> 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::<whatever> 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::.
Good point. Here's the same thing using PostgreSQL::Test
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment
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
Attachment
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
Attachment
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
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.)
> On Sep 7, 2021, at 9:00 PM, Noah Misch <noah@leadboat.com> 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
On 9/7/21 7:43 AM, 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? > > There's nothing very test-specific about this module - it simply > encapsulates a Postgres version string. So maybe it should just be > PostgreSQL::Version. > > Discussion has gone quiet and the tree is now relatively quiet, so now seems like a good time to do this. See attached patches. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Attachment
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
Attachment
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
Attachment
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
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
On Sun, Oct 24, 2021 at 10:46:30AM -0400, Andrew Dunstan wrote: > ... and pushed. Thanks! -- Michael
Attachment
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
Andres Freund <andres@anarazel.de> 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
Hi, On 2022-04-18 10:26:15 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
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.
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
Andrew Dunstan <andrew@dunslane.net> 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
On 2022-04-18 Mo 13:43, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> 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
Andrew Dunstan <andrew@dunslane.net> 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
On 2022-04-18 Mo 14:07, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> 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
> On Apr 18, 2022, at 10:59 AM, Andrew Dunstan <andrew@dunslane.net> 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 alreadyassigned to the old stopped node, except when port assignment wraps around. Without sharing the global, wrap-aroundneed not happen for port collisions. Or am I reading the code wrong? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2022-04-18 Mo 15:46, Mark Dilger wrote: > >> On Apr 18, 2022, at 10:59 AM, Andrew Dunstan <andrew@dunslane.net> 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 alreadyassigned to the old stopped node, except when port assignment wraps around. Without sharing the global, wrap-aroundneed 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
> On Apr 18, 2022, at 1:19 PM, Andrew Dunstan <andrew@dunslane.net> 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
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
Attachment
> On 18 Apr 2022, at 19:59, Andrew Dunstan <andrew@dunslane.net> 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/
On 2022-04-18 Mo 14:07, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> 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
Attachment
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
On 2022-04-19 Tu 11:36, Andrew Dunstan wrote:
> On 2022-04-18 Mo 14:07, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> 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
Attachment
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
Attachment
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
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
Attachment
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
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
Attachment
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
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!
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.
Attachment
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
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.
Attachment
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.