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
