From: | Mark Dilger <hornschnorter(at)gmail(dot)com> |
---|---|
To: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: TestLib::command_fails_like enhancement |
Date: | 2019-11-11 21:28:37 |
Message-ID: | efc657ab-d1e4-3b36-7ffa-806c7ab3485e@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2019-11-11 21:41:41 | Missing dependency tracking for TableFunc nodes |
Previous Message | Alvaro Herrera | 2019-11-11 20:26:16 | Re: FETCH FIRST clause WITH TIES option |