Thread: TestLib::command_fails_like enhancement
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
Attachment
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
Attachment
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
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.
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
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
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
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
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
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
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
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
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
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
Attachment
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
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