Re: psql \watch versus \timing

On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 08/18/2014 10:51 AM, Michael Paquier wrote:
>>
>> On Mon, Aug 18, 2014 at 4:12 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
>> wrote:
>>>
>>> On Mon, Aug 18, 2014 at 3:19 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>>
>>>> On Thu, Aug 14, 2014 at 11:10 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.

Yep!

> 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.

Agreed. So what about PSQLexecCommon (inspired by
the relation between LWLockAcquireCommon and LWLockAcquire)?
Or any better name?

Regards,

--
Fujii Masao