On 08/18/2014 10:51 AM, Michael Paquier wrote:
> On Mon, Aug 18, 2014 at 4:12 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Mon, Aug 18, 2014 at 3:19 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Thu, Aug 14, 2014 at 11:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> Attached patch changes \watch so that it displays how long the query takes
>>>> if \timing is enabled.
>>>>
>>>> I didn't refactor PSQLexec and SendQuery into one routine because
>>>> the contents of those functions are not so same. I'm not sure how much
>>>> it's worth doing that refactoring. Anyway this feature is quite useful
>>>> even without that refactoring, I think.
>>>
>>> The patch applies correctly and it does correctly what it is made for:
>>> =# \timing
>>> Timing is on.
>>> =# select 1;
>>> ?column?
>>> ----------
>>> 1
>>> (1 row)
>>> Time: 0.407 ms
>>> =# \watch 1
>>> Watch every 1s Mon Aug 18 15:17:41 2014
>>> ?column?
>>> ----------
>>> 1
>>> (1 row)
>>> Time: 0.397 ms
>>> Watch every 1s Mon Aug 18 15:17:42 2014
>>> ?column?
>>> ----------
>>> 1
>>> (1 row)
>>> Time: 0.615 ms
>>>
>>> Refactoring it would be worth it thinking long-term... And printing
>>> the timing in PSQLexec code path is already done in SendQuery, so
>>> that's doing two times the same thing IMHO.
>>>
>>> Now, looking at the patch, introducing the new function
>>> PSQLexecInternal with an additional parameter to control the timing is
>>> correct choosing the non-refactoring way of doing. But I don't think
>>> that printing the time outside PSQLexecInternal is consistent with
>>> SendQuery. Why not simply control the timing with a boolean flag and
>>> print the timing directly in PSQLexecInternal?
>>
>> Because the timing needs to be printed after the query result.
> Thanks for pointing that. Yes this makes the refactoring a bit more difficult.
Michael reviewed this, so I'm marking this as Ready for Committer. Since
you're a committer yourself, I expect you'll take it over from here.
I agree that refactoring this would be nice in the long-term, and I also
agree that it's probably OK as it is in the short-term. I don't like the
name PSQLexecInternal, though. PSQLexec is used for "internal" commands
anyway. In fact it's backwards, because PSQLexecInternal is used for
non-internal queries, given by \watch, while PSQLexec is used for
internal commands.
- Heikki
