Horton

Thread

Commits

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

  1. TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-10-31T17:02:58Z

    This small patch authored by my colleague Craig Ringer enhances
Testlib's command_fails_like by allowing the passing of extra keyword
type arguments. The keyword initially recognized is 'extra_ipcrun_opts'.
The value for this keyword needs to be an array, and is passed through
to the call to IPC::Run.

Some patches I will be submitting shortly rely on this enhancement.


cheers


andrew


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

  2. Re: TestLib::command_fails_like enhancement

    Michael Paquier <michael@paquier.xyz> — 2019-11-02T07:44:40Z

    On Thu, Oct 31, 2019 at 01:02:58PM -0400, Andrew Dunstan wrote:
> This small patch authored by my colleague Craig Ringer enhances
> Testlib's command_fails_like by allowing the passing of extra keyword
> type arguments. The keyword initially recognized is 'extra_ipcrun_opts'.
> The value for this keyword needs to be an array, and is passed through
> to the call to IPC::Run.

Why not.

> Some patches I will be submitting shortly rely on this enhancement.

Anything submitted yet or any examples?  I was just wondering in which
case it mattered.
--
Michael

  3. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-07T22:28:24Z

    
On 10/31/19 10:02 AM, Andrew Dunstan wrote:
> 
> This small patch authored by my colleague Craig Ringer enhances
> Testlib's command_fails_like by allowing the passing of extra keyword
> type arguments. The keyword initially recognized is 'extra_ipcrun_opts'.
> The value for this keyword needs to be an array, and is passed through
> to the call to IPC::Run.

Hi Andrew, a few code review comments:

The POD documentation for this function should be updated to include a 
description of the %kwargs argument list.

Since command_fails_like is patterned on command_like, perhaps you 
should make this change to both of them, even if you only originally 
intend to use the new functionality in command_fails_like.  I'm not sure 
of this, though, since I haven't seen any example usage yet.

I'm vaguely bothered by having %kwargs gobble up the remaining function 
arguments, not because it isn't a perl-ish thing to do, but because none 
of the other functions in this module do anything similar.  The function 
check_mode_recursive takes an optional $ignore_list array reference as 
its last argument.  Perhaps command_fails_like could follow that pattern 
by taking an optional $kwargs hash reference.

-- 
Mark Dilger

  4. Re: TestLib::command_fails_like enhancement

    Craig Ringer <craig@2ndquadrant.com> — 2019-11-08T06:16:28Z

    On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter@gmail.com> wrote:

>
>
> On 10/31/19 10:02 AM, Andrew Dunstan wrote:
> >
> > This small patch authored by my colleague Craig Ringer enhances
> > Testlib's command_fails_like by allowing the passing of extra keyword
> > type arguments. The keyword initially recognized is 'extra_ipcrun_opts'.
> > The value for this keyword needs to be an array, and is passed through
> > to the call to IPC::Run.
>
> Hi Andrew, a few code review comments:
>
> The POD documentation for this function should be updated to include a
> description of the %kwargs argument list.
>
> Since command_fails_like is patterned on command_like, perhaps you
> should make this change to both of them, even if you only originally
> intend to use the new functionality in command_fails_like.  I'm not sure
> of this, though, since I haven't seen any example usage yet.
>
> I'm vaguely bothered by having %kwargs gobble up the remaining function
> arguments, not because it isn't a perl-ish thing to do, but because none
> of the other functions in this module do anything similar.  The function
> check_mode_recursive takes an optional $ignore_list array reference as
> its last argument.  Perhaps command_fails_like could follow that pattern
> by taking an optional $kwargs hash reference.
>

Yeah, that's probably sensible.


-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 2ndQuadrant - PostgreSQL Solutions for the Enterprise

  5. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-08T14:33:20Z

    On 11/8/19 1:16 AM, Craig Ringer wrote:
> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter@gmail.com
> <mailto:hornschnorter@gmail.com>> wrote:
>
>
>
>     On 10/31/19 10:02 AM, Andrew Dunstan wrote:
>     >
>     > This small patch authored by my colleague Craig Ringer enhances
>     > Testlib's command_fails_like by allowing the passing of extra
>     keyword
>     > type arguments. The keyword initially recognized is
>     'extra_ipcrun_opts'.
>     > The value for this keyword needs to be an array, and is passed
>     through
>     > to the call to IPC::Run.
>
>     Hi Andrew, a few code review comments:
>
>     The POD documentation for this function should be updated to
>     include a
>     description of the %kwargs argument list.
>
>     Since command_fails_like is patterned on command_like, perhaps you
>     should make this change to both of them, even if you only originally
>     intend to use the new functionality in command_fails_like.  I'm
>     not sure
>     of this, though, since I haven't seen any example usage yet.
>
>     I'm vaguely bothered by having %kwargs gobble up the remaining
>     function
>     arguments, not because it isn't a perl-ish thing to do, but
>     because none
>     of the other functions in this module do anything similar.  The
>     function
>     check_mode_recursive takes an optional $ignore_list array
>     reference as
>     its last argument.  Perhaps command_fails_like could follow that
>     pattern
>     by taking an optional $kwargs hash reference.
>
>
> Yeah, that's probably sensible. 
>
>
>


OK, I will rework it taking these comments into account. Thanks for the
comments Mark.


cheers


andrew


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

  6. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-08T16:25:50Z

    
On 11/8/19 6:33 AM, Andrew Dunstan wrote:
> 
> On 11/8/19 1:16 AM, Craig Ringer wrote:
>> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter@gmail.com
>> <mailto:hornschnorter@gmail.com>> wrote:
>>
>>
>>
>>      On 10/31/19 10:02 AM, Andrew Dunstan wrote:
>>      >
>>      > This small patch authored by my colleague Craig Ringer enhances
>>      > Testlib's command_fails_like by allowing the passing of extra
>>      keyword
>>      > type arguments. The keyword initially recognized is
>>      'extra_ipcrun_opts'.
>>      > The value for this keyword needs to be an array, and is passed
>>      through
>>      > to the call to IPC::Run.
>>
>>      Hi Andrew, a few code review comments:
>>
>>      The POD documentation for this function should be updated to
>>      include a
>>      description of the %kwargs argument list.
>>
>>      Since command_fails_like is patterned on command_like, perhaps you
>>      should make this change to both of them, even if you only originally
>>      intend to use the new functionality in command_fails_like.  I'm
>>      not sure
>>      of this, though, since I haven't seen any example usage yet.
>>
>>      I'm vaguely bothered by having %kwargs gobble up the remaining
>>      function
>>      arguments, not because it isn't a perl-ish thing to do, but
>>      because none
>>      of the other functions in this module do anything similar.  The
>>      function
>>      check_mode_recursive takes an optional $ignore_list array
>>      reference as
>>      its last argument.  Perhaps command_fails_like could follow that
>>      pattern
>>      by taking an optional $kwargs hash reference.
>>
>>
>> Yeah, that's probably sensible.
>>
>>
>>
> 
> 
> OK, I will rework it taking these comments into account. Thanks for the
> comments Mark.

I'd be happy to see the regression tests you are writing sooner than 
that, if you don't mind posting them.  It's hard to do a proper review 
for you without a better sense of where you are going with these changes.

-- 
Mark Dilger

  7. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-08T17:22:05Z

    On 11/8/19 11:25 AM, Mark Dilger wrote:
>
>
> On 11/8/19 6:33 AM, Andrew Dunstan wrote:
>>
>> On 11/8/19 1:16 AM, Craig Ringer wrote:
>>> On Fri, 8 Nov 2019 at 06:28, Mark Dilger <hornschnorter@gmail.com
>>> <mailto:hornschnorter@gmail.com>> wrote:
>>>
>>>
>>>
>>>      On 10/31/19 10:02 AM, Andrew Dunstan wrote:
>>>      >
>>>      > This small patch authored by my colleague Craig Ringer enhances
>>>      > Testlib's command_fails_like by allowing the passing of extra
>>>      keyword
>>>      > type arguments. The keyword initially recognized is
>>>      'extra_ipcrun_opts'.
>>>      > The value for this keyword needs to be an array, and is passed
>>>      through
>>>      > to the call to IPC::Run.
>>>
>>>      Hi Andrew, a few code review comments:
>>>
>>>      The POD documentation for this function should be updated to
>>>      include a
>>>      description of the %kwargs argument list.
>>>
>>>      Since command_fails_like is patterned on command_like, perhaps you
>>>      should make this change to both of them, even if you only
>>> originally
>>>      intend to use the new functionality in command_fails_like.  I'm
>>>      not sure
>>>      of this, though, since I haven't seen any example usage yet.
>>>
>>>      I'm vaguely bothered by having %kwargs gobble up the remaining
>>>      function
>>>      arguments, not because it isn't a perl-ish thing to do, but
>>>      because none
>>>      of the other functions in this module do anything similar.  The
>>>      function
>>>      check_mode_recursive takes an optional $ignore_list array
>>>      reference as
>>>      its last argument.  Perhaps command_fails_like could follow that
>>>      pattern
>>>      by taking an optional $kwargs hash reference.
>>>
>>>
>>> Yeah, that's probably sensible.
>>>
>>>
>>>
>>
>>
>> OK, I will rework it taking these comments into account. Thanks for the
>> comments Mark.
>
> I'd be happy to see the regression tests you are writing sooner than
> that, if you don't mind posting them.  It's hard to do a proper review
> for you without a better sense of where you are going with these changes.


This will need to be rewritten in light of the above, but see
<https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@2ndQuadrant.com>


cheers


andrew


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

  8. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-08T21:40:06Z

    
On 11/8/19 9:22 AM, Andrew Dunstan wrote:
...
> This will need to be rewritten in light of the above, but see
> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@2ndQuadrant.com>

Thanks for the reference.  Having read your motivating example, this new 
review reverses some of what I suggested in the prior review.


In the existing TestLib.pm, there are eight occurrences of nearly 
identical usages of IPC::Run scattered through similar functions:

run_command:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

check_pg_config:
     my $result = IPC::Run::run [ 'pg_config', '--includedir' ], '>',
       \$stdout, '2>', \$stderr
       or die "could not execute pg_config";

program_help_ok:
     my $result = IPC::Run::run [ $cmd, '--help' ], '>', \$stdout, '2>',
       \$stderr;

program_version_ok:
     my $result = IPC::Run::run [ $cmd, '--version' ], '>', \$stdout, '2>',
       \$stderr;

program_options_handling_ok:
     my $result = IPC::Run::run [ $cmd, '--not-a-valid-option' ], '>',
       \$stdout,
       '2>', \$stderr;

command_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr;

command_like_safe:
     my $result = IPC::Run::run $cmd, '>', $stdoutfile, '2>', $stderrfile;

command_fails_like:
     my $result = IPC::Run::run $cmd, '>', \$stdout, '2>', \$stderr,
       @extra_ipcrun_opts;

One rational motive for designing TestLib with so much code duplication 
is to make the tests that use the library easier to read:

   command_like_safe(foo);
   command_like(bar);
   command_fails_like(baz);

which is easier to understand than:

   command_like(foo, failure_mode => safe);
   command_like(bar);
   command_like(baz, failure => expected);

and so forth.

In line with that thinking, perhaps you should just create:

   command_fails_without_tty_like(foo)

and be done, or perhaps:

   command_fails_like(foo, tty => 'closed')

and still preserve some of the test readability.  Will anyone like the 
readability of your tests if you have:

   command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ?

Admittedly, "foo", "bar", and "baz" above are shorthand notation for 
things in practice that are already somewhat hard to read, as in:

   command_fails_like(
       [ 'pg_dump', 'qqq', 'abc' ],
       qr/\Qpg_dump: error: too many command-line arguments (first is 
"abc")\E/,
       'pg_dump: too many command-line arguments');

but adding more to that cruft just makes it worse.  Right?

-- 
Mark Dilger

  9. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-09T13:25:10Z

    On 11/8/19 4:40 PM, Mark Dilger wrote:
>
>
> On 11/8/19 9:22 AM, Andrew Dunstan wrote:
> ...
>> This will need to be rewritten in light of the above, but see
>> <https://www.postgresql.org/message-id/87b1e36b-e36a-add5-1a9b-9fa34914a256@2ndQuadrant.com>
>>
>
> Thanks for the reference.  Having read your motivating example, this
> new review reverses some of what I suggested in the prior review.
>
>
> In the existing TestLib.pm, there are eight occurrences of nearly
> identical usages of IPC::Run scattered through similar functions:
>
>
[snip]


>
> One rational motive for designing TestLib with so much code
> duplication is to make the tests that use the library easier to read:
>
>   command_like_safe(foo);
>   command_like(bar);
>   command_fails_like(baz);
>
> which is easier to understand than:
>
>   command_like(foo, failure_mode => safe);
>   command_like(bar);
>   command_like(baz, failure => expected);
>
> and so forth.
>
> In line with that thinking, perhaps you should just create:
>
>   command_fails_without_tty_like(foo)
>
> and be done, or perhaps:
>
>   command_fails_like(foo, tty => 'closed')
>
> and still preserve some of the test readability.  Will anyone like the
> readability of your tests if you have:
>
>   command_fails_like(foo, extra_ipcrun_opts => ['<pty<', \$eof_in]) ?
>
> Admittedly, "foo", "bar", and "baz" above are shorthand notation for
> things in practice that are already somewhat hard to read, as in:
>
>   command_fails_like(
>       [ 'pg_dump', 'qqq', 'abc' ],
>       qr/\Qpg_dump: error: too many command-line arguments (first is
> "abc")\E/,
>       'pg_dump: too many command-line arguments');
>
> but adding more to that cruft just makes it worse.  Right?
>

OK, I agree that we're getting rather baroque here. I could go with your
suggestion of YA function, or possibly a solution that simple passes any
extra arguments straight through to IPC::Run::run(), e.g.

command_fails_like(
      [ 'pg_dump', 'qqq', 'abc' ],
      qr/\Qpg_dump: error: too many command-line arguments (first is
"abc")\E/,
      'pg_dump: too many command-line arguments',
      '<pty<', \$eof_in);

That means we're not future-proofing the function - we'll never be able
to add more arguments to it, but I'm not really certain that matters
anyway. I should note that perlcritic whines about subroutines with too
many arguments, so making provision for more seems unnecessary anyway.

I don't think this is worth spending a huge amount of time on, we've
already spent more time discussing it than it would take to implement
either solution.


cheers


andrew

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

  10. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-11T16:48:25Z

    On 11/9/19 8:25 AM, Andrew Dunstan wrote:
> OK, I agree that we're getting rather baroque here. I could go with your
> suggestion of YA function, or possibly a solution that simple passes any
> extra arguments straight through to IPC::Run::run(), e.g.
>
> command_fails_like(
>       [ 'pg_dump', 'qqq', 'abc' ],
>       qr/\Qpg_dump: error: too many command-line arguments (first is
> "abc")\E/,
>       'pg_dump: too many command-line arguments',
>       '<pty<', \$eof_in);
>
> That means we're not future-proofing the function - we'll never be able
> to add more arguments to it, but I'm not really certain that matters
> anyway. I should note that perlcritic whines about subroutines with too
> many arguments, so making provision for more seems unnecessary anyway.
>
> I don't think this is worth spending a huge amount of time on, we've
> already spent more time discussing it than it would take to implement
> either solution.
>
>

On further consideration, I'm wondering why we don't just
unconditionally use a closed input pty for all these functions (except
run_log). None of them use any input, and making the client worry about
whether or not to close it seems something of an abstraction break.
There would be no API change at all involved in this case, just a bit of
extra cleanliness. Would need testing on Windows, I'll go and do that now.


Thoughts?


cheers


andrew


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

  11. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-11T18:27:35Z

    
On 11/11/19 8:48 AM, Andrew Dunstan wrote:
> 
> On 11/9/19 8:25 AM, Andrew Dunstan wrote:
>> OK, I agree that we're getting rather baroque here. I could go with your
>> suggestion of YA function, or possibly a solution that simple passes any
>> extra arguments straight through to IPC::Run::run(), e.g.
>>
>> command_fails_like(
>>        [ 'pg_dump', 'qqq', 'abc' ],
>>        qr/\Qpg_dump: error: too many command-line arguments (first is
>> "abc")\E/,
>>        'pg_dump: too many command-line arguments',
>>        '<pty<', \$eof_in);
>>
>> That means we're not future-proofing the function - we'll never be able
>> to add more arguments to it, but I'm not really certain that matters
>> anyway. I should note that perlcritic whines about subroutines with too
>> many arguments, so making provision for more seems unnecessary anyway.
>>
>> I don't think this is worth spending a huge amount of time on, we've
>> already spent more time discussing it than it would take to implement
>> either solution.
>>
>>
> 
> On further consideration, I'm wondering why we don't just
> unconditionally use a closed input pty for all these functions (except
> run_log). None of them use any input, and making the client worry about
> whether or not to close it seems something of an abstraction break.
> There would be no API change at all involved in this case, just a bit of
> extra cleanliness. Would need testing on Windows, I'll go and do that now.
> 
> 
> Thoughts?

That sounds a lot better than your previous patch.

PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you change 
all the invocations in TestLib to close input pty, should you do the 
same for PostgresNode?  I don't have a strong argument for doing so, but 
it seems cleaner to have both libraries invoking commands under 
identical conditions, such that if commands were borrowed from one 
library and called from the other they would behave the same.

PostgresNode already uses TestLib, so perhaps setting up the environment 
can be abstracted into something, perhaps TestLib::run, and then used 
everywhere that IPC::Run::run currently is used.


-- 
Mark Dilger

  12. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-11T19:28:23Z

    On 11/11/19 1:27 PM, Mark Dilger wrote:
>
>
> On 11/11/19 8:48 AM, Andrew Dunstan wrote:
>>
>> On 11/9/19 8:25 AM, Andrew Dunstan wrote:
>>> OK, I agree that we're getting rather baroque here. I could go with
>>> your
>>> suggestion of YA function, or possibly a solution that simple passes
>>> any
>>> extra arguments straight through to IPC::Run::run(), e.g.
>>>
>>> command_fails_like(
>>>        [ 'pg_dump', 'qqq', 'abc' ],
>>>        qr/\Qpg_dump: error: too many command-line arguments (first is
>>> "abc")\E/,
>>>        'pg_dump: too many command-line arguments',
>>>        '<pty<', \$eof_in);
>>>
>>> That means we're not future-proofing the function - we'll never be able
>>> to add more arguments to it, but I'm not really certain that matters
>>> anyway. I should note that perlcritic whines about subroutines with too
>>> many arguments, so making provision for more seems unnecessary anyway.
>>>
>>> I don't think this is worth spending a huge amount of time on, we've
>>> already spent more time discussing it than it would take to implement
>>> either solution.
>>>
>>>
>>
>> On further consideration, I'm wondering why we don't just
>> unconditionally use a closed input pty for all these functions (except
>> run_log). None of them use any input, and making the client worry about
>> whether or not to close it seems something of an abstraction break.
>> There would be no API change at all involved in this case, just a bit of
>> extra cleanliness. Would need testing on Windows, I'll go and do that
>> now.
>>
>>
>> Thoughts?
>
> That sounds a lot better than your previous patch.
>
> PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you
> change all the invocations in TestLib to close input pty, should you
> do the same for PostgresNode?  I don't have a strong argument for
> doing so, but it seems cleaner to have both libraries invoking
> commands under identical conditions, such that if commands were
> borrowed from one library and called from the other they would behave
> the same.
>
> PostgresNode already uses TestLib, so perhaps setting up the
> environment can be abstracted into something, perhaps TestLib::run,
> and then used everywhere that IPC::Run::run currently is used.



I don't think we need to do that. In the case of the PostgresNode.pm
uses we know what the executable is, unlike the the TestLib.pm cases.
They are our own executables and we don't expect them to be doing
anything funky with /dev/tty.


cheers


andrew


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

  13. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-11T21:28:37Z

    
On 11/11/19 11:28 AM, Andrew Dunstan wrote:
> 
> On 11/11/19 1:27 PM, Mark Dilger wrote:
>>
>>
>> On 11/11/19 8:48 AM, Andrew Dunstan wrote:
>>>
>>> On 11/9/19 8:25 AM, Andrew Dunstan wrote:
>>>> OK, I agree that we're getting rather baroque here. I could go with
>>>> your
>>>> suggestion of YA function, or possibly a solution that simple passes
>>>> any
>>>> extra arguments straight through to IPC::Run::run(), e.g.
>>>>
>>>> command_fails_like(
>>>>         [ 'pg_dump', 'qqq', 'abc' ],
>>>>         qr/\Qpg_dump: error: too many command-line arguments (first is
>>>> "abc")\E/,
>>>>         'pg_dump: too many command-line arguments',
>>>>         '<pty<', \$eof_in);
>>>>
>>>> That means we're not future-proofing the function - we'll never be able
>>>> to add more arguments to it, but I'm not really certain that matters
>>>> anyway. I should note that perlcritic whines about subroutines with too
>>>> many arguments, so making provision for more seems unnecessary anyway.
>>>>
>>>> I don't think this is worth spending a huge amount of time on, we've
>>>> already spent more time discussing it than it would take to implement
>>>> either solution.
>>>>
>>>>
>>>
>>> On further consideration, I'm wondering why we don't just
>>> unconditionally use a closed input pty for all these functions (except
>>> run_log). None of them use any input, and making the client worry about
>>> whether or not to close it seems something of an abstraction break.
>>> There would be no API change at all involved in this case, just a bit of
>>> extra cleanliness. Would need testing on Windows, I'll go and do that
>>> now.
>>>
>>>
>>> Thoughts?
>>
>> That sounds a lot better than your previous patch.
>>
>> PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you
>> change all the invocations in TestLib to close input pty, should you
>> do the same for PostgresNode?  I don't have a strong argument for
>> doing so, but it seems cleaner to have both libraries invoking
>> commands under identical conditions, such that if commands were
>> borrowed from one library and called from the other they would behave
>> the same.
>>
>> PostgresNode already uses TestLib, so perhaps setting up the
>> environment can be abstracted into something, perhaps TestLib::run,
>> and then used everywhere that IPC::Run::run currently is used.
> 
> 
> 
> I don't think we need to do that. In the case of the PostgresNode.pm
> uses we know what the executable is, unlike the the TestLib.pm cases.
> They are our own executables and we don't expect them to be doing
> anything funky with /dev/tty.

Ok.  I think your proposal sounds fine.

-- 
Mark Dilger

  14. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-25T13:08:56Z

    On 11/11/19 4:28 PM, Mark Dilger wrote:
>
>
>>>>>
>>>>
>>>> On further consideration, I'm wondering why we don't just
>>>> unconditionally use a closed input pty for all these functions (except
>>>> run_log). None of them use any input, and making the client worry
>>>> about
>>>> whether or not to close it seems something of an abstraction break.
>>>> There would be no API change at all involved in this case, just a
>>>> bit of
>>>> extra cleanliness. Would need testing on Windows, I'll go and do that
>>>> now.
>>>>
>>>>
>>>> Thoughts?
>>>
>>> That sounds a lot better than your previous patch.
>>>
>>> PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you
>>> change all the invocations in TestLib to close input pty, should you
>>> do the same for PostgresNode?  I don't have a strong argument for
>>> doing so, but it seems cleaner to have both libraries invoking
>>> commands under identical conditions, such that if commands were
>>> borrowed from one library and called from the other they would behave
>>> the same.
>>>
>>> PostgresNode already uses TestLib, so perhaps setting up the
>>> environment can be abstracted into something, perhaps TestLib::run,
>>> and then used everywhere that IPC::Run::run currently is used.
>>
>>
>>
>> I don't think we need to do that. In the case of the PostgresNode.pm
>> uses we know what the executable is, unlike the the TestLib.pm cases.
>> They are our own executables and we don't expect them to be doing
>> anything funky with /dev/tty.
>
> Ok.  I think your proposal sounds fine.



Here's a patch for that. The pty stuff crashes and burns on my Windows
test box, so there I just set stdin to an empty string via the usual
pipe mechanism.


cheers


andrew


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

  15. Re: TestLib::command_fails_like enhancement

    Mark Dilger <hornschnorter@gmail.com> — 2019-11-25T18:56:38Z

    
On 11/25/19 5:08 AM, Andrew Dunstan wrote:
> 
> On 11/11/19 4:28 PM, Mark Dilger wrote:
>>
>>
>>>>>>
>>>>>
>>>>> On further consideration, I'm wondering why we don't just
>>>>> unconditionally use a closed input pty for all these functions (except
>>>>> run_log). None of them use any input, and making the client worry
>>>>> about
>>>>> whether or not to close it seems something of an abstraction break.
>>>>> There would be no API change at all involved in this case, just a
>>>>> bit of
>>>>> extra cleanliness. Would need testing on Windows, I'll go and do that
>>>>> now.
>>>>>
>>>>>
>>>>> Thoughts?
>>>>
>>>> That sounds a lot better than your previous patch.
>>>>
>>>> PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you
>>>> change all the invocations in TestLib to close input pty, should you
>>>> do the same for PostgresNode?  I don't have a strong argument for
>>>> doing so, but it seems cleaner to have both libraries invoking
>>>> commands under identical conditions, such that if commands were
>>>> borrowed from one library and called from the other they would behave
>>>> the same.
>>>>
>>>> PostgresNode already uses TestLib, so perhaps setting up the
>>>> environment can be abstracted into something, perhaps TestLib::run,
>>>> and then used everywhere that IPC::Run::run currently is used.
>>>
>>>
>>>
>>> I don't think we need to do that. In the case of the PostgresNode.pm
>>> uses we know what the executable is, unlike the the TestLib.pm cases.
>>> They are our own executables and we don't expect them to be doing
>>> anything funky with /dev/tty.
>>
>> Ok.  I think your proposal sounds fine.
> 
> 
> 
> Here's a patch for that. The pty stuff crashes and burns on my Windows
> test box, so there I just set stdin to an empty string via the usual
> pipe mechanism.

Ok, I've reviewed and tested this.  It works fine for me on Linux.  I
am not set up to test it on Windows.  I think it is ready to commit.

I have one remaining comment about the code, and this is just FYI.  I
won't quibble with you committing your patch as it currently stands.

You might consider changing the '\x04' literal to use a named control
character, both for readability and portability, as here:

+               use charnames ':full';
+               @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");

The only character set I can find where this matters is EBCDIC, in
which the EOT character is 55 rather than 4.  Since EBCDIC does not
occur in the list of supported character sets for postgres, per the
docs section 23.3.1, I don't suppose it matters too much.  Nor can
I test how this works on EBCDIC, so I'm mostly guessing that perl
would do the right thing there.  But, at least to my eyes, it is
more immediately clear what the code is doing when the control
character name is spelled out.


-- 
Mark Dilger

  16. Re: TestLib::command_fails_like enhancement

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2019-11-25T20:07:10Z

    On 11/25/19 1:56 PM, Mark Dilger wrote:
>
>
> On 11/25/19 5:08 AM, Andrew Dunstan wrote:
>>
>> On 11/11/19 4:28 PM, Mark Dilger wrote:
>>>
>>>
>>>>>>>
>>>>>>
>>>>>> On further consideration, I'm wondering why we don't just
>>>>>> unconditionally use a closed input pty for all these functions
>>>>>> (except
>>>>>> run_log). None of them use any input, and making the client worry
>>>>>> about
>>>>>> whether or not to close it seems something of an abstraction break.
>>>>>> There would be no API change at all involved in this case, just a
>>>>>> bit of
>>>>>> extra cleanliness. Would need testing on Windows, I'll go and do
>>>>>> that
>>>>>> now.
>>>>>>
>>>>>>
>>>>>> Thoughts?
>>>>>
>>>>> That sounds a lot better than your previous patch.
>>>>>
>>>>> PostgresNode.pm and TestLib.pm both invoke IPC::Run::run.  If you
>>>>> change all the invocations in TestLib to close input pty, should you
>>>>> do the same for PostgresNode?  I don't have a strong argument for
>>>>> doing so, but it seems cleaner to have both libraries invoking
>>>>> commands under identical conditions, such that if commands were
>>>>> borrowed from one library and called from the other they would behave
>>>>> the same.
>>>>>
>>>>> PostgresNode already uses TestLib, so perhaps setting up the
>>>>> environment can be abstracted into something, perhaps TestLib::run,
>>>>> and then used everywhere that IPC::Run::run currently is used.
>>>>
>>>>
>>>>
>>>> I don't think we need to do that. In the case of the PostgresNode.pm
>>>> uses we know what the executable is, unlike the the TestLib.pm cases.
>>>> They are our own executables and we don't expect them to be doing
>>>> anything funky with /dev/tty.
>>>
>>> Ok.  I think your proposal sounds fine.
>>
>>
>>
>> Here's a patch for that. The pty stuff crashes and burns on my Windows
>> test box, so there I just set stdin to an empty string via the usual
>> pipe mechanism.
>
> Ok, I've reviewed and tested this.  It works fine for me on Linux.  I
> am not set up to test it on Windows.  I think it is ready to commit.
>
> I have one remaining comment about the code, and this is just FYI.  I
> won't quibble with you committing your patch as it currently stands.
>
> You might consider changing the '\x04' literal to use a named control
> character, both for readability and portability, as here:
>
> +               use charnames ':full';
> +               @no_stdin = ('<pty<', \"\N{END OF TRANSMISSION}");
>
> The only character set I can find where this matters is EBCDIC, in
> which the EOT character is 55 rather than 4.  Since EBCDIC does not
> occur in the list of supported character sets for postgres, per the
> docs section 23.3.1, I don't suppose it matters too much.  Nor can
> I test how this works on EBCDIC, so I'm mostly guessing that perl
> would do the right thing there.  But, at least to my eyes, it is
> more immediately clear what the code is doing when the control
> character name is spelled out.
>
>


Agreed, I'll do it that way. This is quite timely, as I just finished
reworking the patch that relies on it. Thanks for the review.


cheers


andrew


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