Thread: pgsql: Close stdin where it's not needed in TestLib.pm procedures
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: https://postgr.es/m/873ebb57-fc98-340d-949d-691b1810bf66@2ndQuadrant.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/9af34f3c6b02779fac6dbb6cd4c5bb225a019f43 Modified Files -------------- src/test/perl/TestLib.pm | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
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: https://postgr.es/m/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
Attachment
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
Attachment
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
Attachment
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