pgsql: Close stdin where it's not needed in TestLib.pm procedures

Hi Andrew,

On Mon, Nov 25, 2019 at 09:01:46PM +0000, Andrew Dunstan wrote:

Close stdin where it's not needed in TestLib.pm procedures

Where possible, do this using a pseudoterminal, so that things like
openssl that want to open /dev/tty if stdin isn't a tty won't.
Elsewhere, i.e. Windows, just close by providing an empty string using
the standard IPC::Run pipe mechanism.

Patch by Andrew Dunstan, based on an idea from Craig Ringer.

Reviewed by Mark Dilger.

Discussion: /messages/by-id/873ebb57-fc98-340d-949d-691b1810bf66@2ndQuadrant.com

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level
[...]
Unexpected SCALAR(0x1987690) in harness() parameter 7 at
../../../src/test/perl//TestLib.pm line 692
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Nov 25, 2019 at 09:01:46PM +0000, Andrew Dunstan wrote:

Close stdin where it's not needed in TestLib.pm procedures

Where possible, do this using a pseudoterminal, so that things like
openssl that want to open /dev/tty if stdin isn't a tty won't.

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level

Perhaps the "where possible" caveat needs to include a test whether
IO::Pty is installed? It's evidently not there by default everywhere.

It's possible that we should just move the goalposts and say IO::Pty
is required for TAP testing. I can foresee that we'll have to do
that if we ever want automated tests of psql's tab completion,
for example.

regards, tom lane

On Tue, Nov 26, 2019 at 02:13:50AM -0500, Tom Lane wrote:

Perhaps the "where possible" caveat needs to include a test whether
IO::Pty is installed? It's evidently not there by default everywhere.

It's possible that we should just move the goalposts and say IO::Pty
is required for TAP testing. I can foresee that we'll have to do
that if we ever want automated tests of psql's tab completion,
for example.

Hmm. I am not sure that we are at that point yet (in which case we'd
need to check after it in configure). Any other modules we require to
be installed are directly imported to our code, but here we visibly
have a dependency forced by the loaded code. My point is that it does
not impact platforms that do not need it, so it seems like a waste to
require for everybody something which is actually necessary only for a
portion of users running the TAP tests.
--
Michael

On 2019-11-26 05:43, Michael Paquier wrote:

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level
[...]
Unexpected SCALAR(0x1987690) in harness() parameter 7 at
../../../src/test/perl//TestLib.pm line 692

I'm also seeing this problem on macOS in
src/test/recovery/t/016_min_consistency.pl .

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/26/19 3:47 AM, Michael Paquier wrote:

On Tue, Nov 26, 2019 at 02:13:50AM -0500, Tom Lane wrote:

Perhaps the "where possible" caveat needs to include a test whether
IO::Pty is installed? It's evidently not there by default everywhere.

It's possible that we should just move the goalposts and say IO::Pty
is required for TAP testing. I can foresee that we'll have to do
that if we ever want automated tests of psql's tab completion,
for example.

Hmm. I am not sure that we are at that point yet (in which case we'd
need to check after it in configure). Any other modules we require to
be installed are directly imported to our code, but here we visibly
have a dependency forced by the loaded code. My point is that it does
not impact platforms that do not need it, so it seems like a waste to
require for everybody something which is actually necessary only for a
portion of users running the TAP tests.

Yeah, I don't want to go that far at this stage at least.

here's my proposed fix.

I will adjust the code that wants to use it to have a block containing
the tests that need the pty code something like this:

SKIP:
{
� skip 3, "no Pty support" unless $have_io_pty;
� ...
}

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

here's my proposed fix.

Looks sane by eyeball (didn't test it).

regards, tom lane

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-11-26 05:43, Michael Paquier wrote:

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level
[...]
Unexpected SCALAR(0x1987690) in harness() parameter 7 at
../../../src/test/perl//TestLib.pm line 692

I'm also seeing this problem on macOS in
src/test/recovery/t/016_min_consistency.pl .

I just noticed that my buildfarm animal longfin has been stuck for
the past 24 hours in t/001_initdb.pl, and I can replicate that
in a manual build. ktrace says it's in some sort of loop.

Apparently, macOS has got IO::Pty but it doesn't actually work
the way this patch expects. Therefore, the proposed patch will
leave things still broken for macOS :-(

I tried this:

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 18b4ce35c2..d67ffc4368 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -192,8 +192,7 @@ INIT
    }
    else
    {
-       use charnames ':full';
-       @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");
+       @no_stdin = ('<pty<', \"");
    }
 }

and that seems to fix it. I wonder what's the point of sending
a nonempty string to the pty, exactly?

regards, tom lane

On 11/26/19 3:30 PM, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-11-26 05:43, Michael Paquier wrote:

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level
[...]
Unexpected SCALAR(0x1987690) in harness() parameter 7 at
../../../src/test/perl//TestLib.pm line 692

I'm also seeing this problem on macOS in
src/test/recovery/t/016_min_consistency.pl .

I just noticed that my buildfarm animal longfin has been stuck for
the past 24 hours in t/001_initdb.pl, and I can replicate that
in a manual build. ktrace says it's in some sort of loop.

Apparently, macOS has got IO::Pty but it doesn't actually work
the way this patch expects. Therefore, the proposed patch will
leave things still broken for macOS :-(

I tried this:

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 18b4ce35c2..d67ffc4368 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -192,8 +192,7 @@ INIT
}
else
{
-       use charnames ':full';
-       @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");
+       @no_stdin = ('<pty<', \"");
}
}

and that seems to fix it. I wonder what's the point of sending
a nonempty string to the pty, exactly?

Oh, I missed seeing this.

The short answer is I didn't question it because it worked for me. I'll
test out your change on Linux with my test case.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11/26/19 5:08 PM, Andrew Dunstan wrote:

On 11/26/19 3:30 PM, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

On 2019-11-26 05:43, Michael Paquier wrote:

This one is causing failures with the TAP tests of initdb on all AIX
animals and prairiedog:
# Running: initdb --help
Can't locate IO/Pty.pm in @INC (@INC contains: ../../../src/test/perl/
. /usr/local/perl5.8.3/lib/5.8.3/darwin-2level
[...]
Unexpected SCALAR(0x1987690) in harness() parameter 7 at
../../../src/test/perl//TestLib.pm line 692

I'm also seeing this problem on macOS in
src/test/recovery/t/016_min_consistency.pl .

I just noticed that my buildfarm animal longfin has been stuck for
the past 24 hours in t/001_initdb.pl, and I can replicate that
in a manual build. ktrace says it's in some sort of loop.

Apparently, macOS has got IO::Pty but it doesn't actually work
the way this patch expects. Therefore, the proposed patch will
leave things still broken for macOS :-(

I tried this:

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 18b4ce35c2..d67ffc4368 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -192,8 +192,7 @@ INIT
}
else
{
-       use charnames ':full';
-       @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");
+       @no_stdin = ('<pty<', \"");
}
}

and that seems to fix it. I wonder what's the point of sending
a nonempty string to the pty, exactly?

Oh, I missed seeing this.

The short answer is I didn't question it because it worked for me. I'll
test out your change on Linux with my test case.

This causes some of the new tests I want to run to hang on Linux,
presumably waiting for the pty to close :-(

So I'm a bit stuck. Probably the best thing to do for now is revert the
patch and mark the tests that need it in the libpq-sslpassword patch as
TODO (with todo_skip). If somebody whose perl-fu is greater than mine
can disentangle this mess that would be nice :-)

Thoughts?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 26, 2019 at 09:36:50PM -0500, Andrew Dunstan wrote:

So I'm a bit stuck. Probably the best thing to do for now is revert the
patch and mark the tests that need it in the libpq-sslpassword patch as
TODO (with todo_skip). If somebody whose perl-fu is greater than mine
can disentangle this mess that would be nice :-)

A revert sounds fine to me at this point. Some buildfarm animals are
not reporting back :(
--
Michael