Horton

Thread

Commits

  1. Fix PostgreSQL::Test aliasing for Perl v5.10.1.

  2. For PostgreSQL::Test compatibility, alias entire package symbol tables.

  3. Make PostgresNode easily subclassable

  1. Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-05-20T19:47:43Z

    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

  2. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-20T21:18:29Z

    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

  3. Re: Postgres perl module namespace

    Devrim GÜNDÜZ <devrim@gunduz.org> — 2021-05-21T13:55:23Z

    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

  4. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-10T14:10:56Z

    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

  5. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-10T14:40:43Z

    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

  6. Re: Postgres perl module namespace

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-10T14:41:35Z

    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)

  7. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-10T15:02:13Z

    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

  8. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-08-11T01:25:02Z

    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

  9. Re: Postgres perl module namespace

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-08-11T01:37:12Z

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

  10. Re: Postgres perl module namespace

    Julien Rouhaud <rjuju123@gmail.com> — 2021-08-11T02:09:29Z

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

  11. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-11T02:11:22Z

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

  12. Re: Postgres perl module namespace

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-08-11T02:13:17Z

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

  13. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-11T02:22:48Z

    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

  14. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-11T02:26:42Z

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

  15. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-11T02:33:50Z

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

  16. Re: Postgres perl module namespace

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-08-11T13:22:45Z

    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)

  17. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-11T13:30:08Z

    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

  18. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-08-23T19:03:37Z

    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

  19. Re: Postgres perl module namespace

    Robert Haas <robertmhaas@gmail.com> — 2021-08-23T19:39:15Z

    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

  20. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-08-25T05:48:39Z

    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

  21. Re: Postgres perl module namespace

    Robert Haas <robertmhaas@gmail.com> — 2021-08-25T14:08:54Z

    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

  22. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-09-03T19:34:24Z

    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

  23. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2021-09-04T06:19:49Z

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

  24. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-09-04T13:58:08Z

    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

  25. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-09-06T05:08:45Z

    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

  26. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-09-07T02:30:59Z

    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

  27. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-09-07T11:43:47Z

    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

  28. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2021-09-08T04:00:11Z

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

  29. Re: Postgres perl module namespace

    Mark Dilger <mark.dilger@enterprisedb.com> — 2021-09-08T14:15:24Z

    
> 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

  30. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-10-19T18:54:58Z

    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

  31. Re: Postgres perl module namespace

    Erik Rijkers <er@xs4all.nl> — 2021-10-19T20:16:06Z

    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

  32. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-10-20T03:22:58Z

    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

  33. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-10-20T12:40:04Z

    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

  34. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2021-10-24T14:46:30Z

    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

  35. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2021-10-25T08:12:08Z

    On Sun, Oct 24, 2021 at 10:46:30AM -0400, Andrew Dunstan wrote:
> ... and pushed.

Thanks!
--
Michael

  36. Re: Postgres perl module namespace

    Andres Freund <andres@anarazel.de> — 2022-04-18T14:15:30Z

    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

  37. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-18T14:26:15Z

    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

  38. Re: Postgres perl module namespace

    Andres Freund <andres@anarazel.de> — 2022-04-18T14:44:29Z

    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

  39. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2022-04-18T15:52:24Z

    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.

  40. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-18T17:28:40Z

    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

  41. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-18T17:43:52Z

    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

  42. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-18T17:59:23Z

    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

  43. Re: Postgres perl module namespace

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-18T18:07:43Z

    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

  44. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-18T19:29:18Z

    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

  45. Re: Postgres perl module namespace

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-18T19:46:09Z

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

  46. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-18T20:19:31Z

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

  47. Re: Postgres perl module namespace

    Mark Dilger <mark.dilger@enterprisedb.com> — 2022-04-18T20:22:17Z

    
> 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

  48. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2022-04-19T02:43:13Z

    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

  49. Re: Postgres perl module namespace

    Daniel Gustafsson <daniel@yesql.se> — 2022-04-19T07:43:03Z

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

  50. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-19T15:36:44Z

    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

  51. Re: Postgres perl module namespace

    Andres Freund <andres@anarazel.de> — 2022-04-19T17:15:28Z

    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

  52. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-19T20:06:28Z

    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

  53. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2022-04-19T22:39:35Z

    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

  54. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-19T23:24:58Z

    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

  55. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2022-04-20T00:30:14Z

    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

  56. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-20T19:56:17Z

    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

  57. Re: Postgres perl module namespace

    Michael Paquier <michael@paquier.xyz> — 2022-04-21T04:11:49Z

    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

  58. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-21T13:42:44Z

    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

  59. Re: Postgres perl module namespace

    Andres Freund <andres@anarazel.de> — 2022-04-22T18:36:56Z

    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!

  60. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2022-06-22T07:21:44Z

    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.

  61. Re: Postgres perl module namespace

    Andrew Dunstan <andrew@dunslane.net> — 2022-06-22T15:03:22Z

    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

  62. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2022-06-24T05:45:40Z

    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.

  63. Re: Postgres perl module namespace

    Noah Misch <noah@leadboat.com> — 2022-06-25T17:15:33Z

    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.