Re: TestLib::command_fails_like enhancement - Mailing list pgsql-hackers

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