On 08/18/2014 10:51 AM, Michael Paquier wrote:
On Mon, Aug 18, 2014 at 4:12 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
On Mon, Aug 18, 2014 at 3:19 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
On Thu, Aug 14, 2014 at 11:10 PM, Fujii Masao <masao.fu...@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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to