Re: [HACKERS] psql \watch versus \timing
On Thu, Sep 4, 2014 at 10:50:58PM +0900, Michael Paquier wrote: On Thu, Sep 4, 2014 at 1:44 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Aug 28, 2014 at 8:46 PM, Fujii Masao masao.fu...@gmail.com wrote: Good catch. So I will remove start_xact code later. Attached patch removes start_xact from PSQLexec. Nothing negative to say here :) Patch simply removes the second argument of PSQLexec that was set to the same value everywhere, aka false as noticed by Heikki. Comments and code blocks related to this parameter are removed, and the code compiles, passing check-world as well (just kicked the tests in case). Uh, where are we on this? Should I commit it? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Tue, Oct 14, 2014 at 4:49 AM, Bruce Momjian br...@momjian.us wrote: On Thu, Sep 4, 2014 at 10:50:58PM +0900, Michael Paquier wrote: On Thu, Sep 4, 2014 at 1:44 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Aug 28, 2014 at 8:46 PM, Fujii Masao masao.fu...@gmail.com wrote: Good catch. So I will remove start_xact code later. Attached patch removes start_xact from PSQLexec. Nothing negative to say here :) Patch simply removes the second argument of PSQLexec that was set to the same value everywhere, aka false as noticed by Heikki. Comments and code blocks related to this parameter are removed, and the code compiles, passing check-world as well (just kicked the tests in case). Uh, where are we on this? Should I commit it? The patch cleaning up the dead code of psql could be clearly applied now. For the feature itself not sure, it may be better to let Fujii-san manage it. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Thu, Sep 4, 2014 at 1:44 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Aug 28, 2014 at 8:46 PM, Fujii Masao masao.fu...@gmail.com wrote: Good catch. So I will remove start_xact code later. Attached patch removes start_xact from PSQLexec. Nothing negative to say here :) Patch simply removes the second argument of PSQLexec that was set to the same value everywhere, aka false as noticed by Heikki. Comments and code blocks related to this parameter are removed, and the code compiles, passing check-world as well (just kicked the tests in case). Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Mon, Sep 1, 2014 at 11:56 PM, Tom Lane t...@sss.pgh.pa.us wrote:
Michael Paquier michael.paqu...@gmail.com writes:
I just tested the patch and this feature works as expected if timing
is on and it displays the individual run time of each query kicked by
\watch. Note that --echo-hidden does not display the query run during
each loop and that this is contrary to the behavior in HEAD so it
breaks backward compatibility, but are there really people relying in
the existing behavior?
ISTM that's an anti-feature anyway, and changing that behavior is a
good thing.
OK, then as all the comments are basically addressed, here is an
updated patch correcting the comment problems mentioned by Heikki.
This is ready for a committer.
Regards,
--
Michael
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 2687,2693 do_watch(PQExpBuffer query_buf, long sleep)
for (;;)
{
! PGresult *res;
time_t timer;
long i;
--- 2687,2693
for (;;)
{
! int res;
time_t timer;
long i;
***
*** 2700,2764 do_watch(PQExpBuffer query_buf, long sleep)
sleep, asctime(localtime(timer)));
myopt.title = title;
! /*
! * Run the query. We use PSQLexec, which is kind of cheating, but
! * SendQuery doesn't let us suppress autocommit behavior.
! */
! res = PSQLexec(query_buf-data, false);
!
! /* PSQLexec handles failure results and returns NULL */
! if (res == NULL)
! break;
/*
! * If SIGINT is sent while the query is processing, PSQLexec will
! * consume the interrupt. The user's intention, though, is to cancel
! * the entire watch process, so detect a sent cancellation request and
! * exit in this case.
*/
! if (cancel_pressed)
! {
! PQclear(res);
break;
! }
!
! switch (PQresultStatus(res))
! {
! case PGRES_TUPLES_OK:
! printQuery(res, myopt, pset.queryFout, pset.logfile);
! break;
!
! case PGRES_COMMAND_OK:
! fprintf(pset.queryFout, %s\n%s\n\n, title, PQcmdStatus(res));
! break;
!
! case PGRES_EMPTY_QUERY:
! psql_error(_(\\watch cannot be used with an empty query\n));
! PQclear(res);
! return false;
!
! case PGRES_COPY_OUT:
! case PGRES_COPY_IN:
! case PGRES_COPY_BOTH:
! psql_error(_(\\watch cannot be used with COPY\n));
! PQclear(res);
! return false;
!
! default:
! /* other cases should have been handled by PSQLexec */
! psql_error(_(unexpected result status for \\watch\n));
! PQclear(res);
! return false;
! }
!
! PQclear(res);
!
! fflush(pset.queryFout);
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
! * through the loop since it's conceivable something inside PSQLexec
! * could change sigint_interrupt_jmp.
*/
if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
break;
--- 2700,2721
sleep, asctime(localtime(timer)));
myopt.title = title;
! /* Run the query and print out the results */
! res = PSQLexecWatch(query_buf-data, myopt);
/*
! * PSQLexecWatch handles the case where we can no longer
! * repeat the query, and returns 0 or -1.
*/
! if (res == 0)
break;
! if (res == -1)
! return false;
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
! * through the loop since it's conceivable something inside
! * PSQLexecWatch could change sigint_interrupt_jmp.
*/
if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
break;
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 497,502 PSQLexec(const char *query, bool start_xact)
--- 497,598
}
+ /*
+ * PSQLexecWatch
+ *
+ * This function is used for \watch command to send the query to
+ * the server and print out the results.
+ *
+ * Returns 1 if the query executed successfully, 0 if it cannot be repeated,
+ * e.g., because of the interrupt, -1 on error.
+ */
+ int
+ PSQLexecWatch(const char *query, const printQueryOpt *opt)
+ {
+ PGresult *res;
+ double elapsed_msec = 0;
+ instr_time before;
+ instr_time after;
+
+ if (!pset.db)
+ {
+ psql_error(You are currently not connected to a database.\n);
+ return 0;
+ }
+
+ SetCancelConn();
+
+ if (pset.timing)
+ INSTR_TIME_SET_CURRENT(before);
+
+ res = PQexec(pset.db, query);
+
+ ResetCancelConn();
+
+ if (!AcceptResult(res))
+ {
+ PQclear(res);
+ return 0;
+ }
+
+ if (pset.timing)
+ {
+ INSTR_TIME_SET_CURRENT(after);
+ INSTR_TIME_SUBTRACT(after, before);
+ elapsed_msec = INSTR_TIME_GET_MILLISEC(after);
+ }
+
+ /*
+ * If SIGINT is sent while the query is processing, the interrupt
+ * will be consumed. The user's intention, though, is to cancel
+ * the entire watch process, so detect a sent cancellation request and
+ * exit in this case.
+ */
+ if (cancel_pressed)
+ {
+ PQclear(res);
+ return 0;
+ }
+
+ switch (PQresultStatus(res))
+ {
Re: [HACKERS] psql \watch versus \timing
On Wed, Sep 3, 2014 at 12:48 PM, Michael Paquier michael.paqu...@gmail.com wrote: OK, then as all the comments are basically addressed, here is an updated patch correcting the comment problems mentioned by Heikki. I just tried this and found it doesn't cooperate well with AUTOCOMMIT = 'off' and ON_ERROR_ROLLBACK = 'on'. Previously \watch would leave the transaction in a normal state after C-c but now it leaves the transaction in an aborted state. I assume it previously did a savepoint around each execution and now it's not doing that at all. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Wed, Sep 3, 2014 at 10:56 PM, Greg Stark st...@mit.edu wrote: On Wed, Sep 3, 2014 at 12:48 PM, Michael Paquier michael.paqu...@gmail.com wrote: OK, then as all the comments are basically addressed, here is an updated patch correcting the comment problems mentioned by Heikki. Thanks a lot! I just tried this and found it doesn't cooperate well with AUTOCOMMIT = 'off' and ON_ERROR_ROLLBACK = 'on'. Previously \watch would leave the transaction in a normal state after C-c but now it leaves the transaction in an aborted state. I assume it previously did a savepoint around each execution and now it's not doing that at all. No. Previously \watch used PSQLexec and it doesn't use savepoint. If you enter Ctrl-C while \watch is waiting for the query to end, \watch would leave the transaction in an aborted state whether the patch has been applied or not. OTOH, if you enter Ctrl-C while \watch is sleeping, the transaction remains in normal state. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Wed, Sep 3, 2014 at 11:13 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Sep 3, 2014 at 10:56 PM, Greg Stark st...@mit.edu wrote: On Wed, Sep 3, 2014 at 12:48 PM, Michael Paquier michael.paqu...@gmail.com wrote: OK, then as all the comments are basically addressed, here is an updated patch correcting the comment problems mentioned by Heikki. Thanks a lot! Applied. Thanks all! Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Thu, Aug 28, 2014 at 8:46 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Tue, Aug 26, 2014 at 4:55 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 08/25/2014 10:48 PM, Heikki Linnakangas wrote:
On 08/25/2014 09:22 PM, Fujii Masao wrote:
On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
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?
Actually, perhaps it would be better to just copy-paste PSQLexec, and
modify the copy to suite \watch's needs. (PSQLexecWatch?
SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much
overlap between what \watch wants and what other PSQLexec callers want.
\watch wants timing output, others don't. \watch doesn't want
transaction handling.
Agreed. Attached is the revised version of the patch. I implemented
PSQLexecWatch() which sends the query, prints the results and outputs
the query execution time (if \timing is enabled).
This patch was marked as ready for committer, but since I revised
the code very much, I marked this as needs review again.
Do we want --echo-hidden to print the \watch'd
query? Not sure..
Per document, --echo-hidden prints the actual queries generated by
backslash command. But \watch doesn't handle backslash commands.
So I think that PSQLexecWatch doesn't need to handle --echo-hidden.
BTW, I just noticed that none of the callers of PSQLexec pass
start_xact=true. So that part of the function is dead code. We might want
to remove it, and replace with a comment noting that PSQLexec never starts a
new transaction block, even in autocommit-off mode. (I know you're hacking
on this, so I didnn't want to joggle your elbow by doing it right now)
Good catch. So I will remove start_xact code later.
Attached patch removes start_xact from PSQLexec.
Regards,
--
Fujii Masao
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 966,972 exec_command(const char *cmd,
printfPQExpBuffer(buf, ALTER USER %s PASSWORD ,
fmtId(user));
appendStringLiteralConn(buf, encrypted_password, pset.db);
! res = PSQLexec(buf.data, false);
termPQExpBuffer(buf);
if (!res)
success = false;
--- 966,972
printfPQExpBuffer(buf, ALTER USER %s PASSWORD ,
fmtId(user));
appendStringLiteralConn(buf, encrypted_password, pset.db);
! res = PSQLexec(buf.data);
termPQExpBuffer(buf);
if (!res)
success = false;
***
*** 2173,2179 process_file(char *filename, bool single_txn, bool use_relative_path)
if (single_txn)
{
! if ((res = PSQLexec(BEGIN, false)) == NULL)
{
if (pset.on_error_stop)
{
--- 2173,2179
if (single_txn)
{
! if ((res = PSQLexec(BEGIN)) == NULL)
{
if (pset.on_error_stop)
{
***
*** 2189,2195 process_file(char *filename, bool single_txn, bool use_relative_path)
if (single_txn)
{
! if ((res = PSQLexec(COMMIT, false)) == NULL)
{
if (pset.on_error_stop)
{
--- 2189,2195
if (single_txn)
{
! if ((res = PSQLexec(COMMIT)) == NULL)
{
if (pset.on_error_stop)
{
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 426,435 AcceptResult(const PGresult *result)
* This is the way to send backdoor queries (those not directly entered
* by the user). It is subject to -E but not -e.
*
- * In autocommit-off mode, a new transaction block is started if start_xact
- * is true; nothing special is done when start_xact is false. Typically,
- * start_xact = false is used for SELECTs and explicit BEGIN/COMMIT commands.
- *
* Caller is responsible for handling the ensuing processing if a COPY
* command is sent.
*
--- 426,431
***
*** 437,443 AcceptResult(const PGresult *result)
* caller uses this path to issue SET CLIENT_ENCODING.
*/
PGresult *
! PSQLexec(const char *query, bool start_xact)
{
PGresult *res;
--- 433,439
* caller uses this path to issue SET CLIENT_ENCODING.
*/
PGresult *
! PSQLexec(const char *query)
{
PGresult *res;
***
*** 468,488 PSQLexec(const char *query, bool start_xact)
SetCancelConn();
- if (start_xact
- !pset.autocommit
- PQtransactionStatus(pset.db) == PQTRANS_IDLE)
- {
- res = PQexec(pset.db, BEGIN);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- {
- psql_error(%s, PQerrorMessage(pset.db));
- PQclear(res);
-
Re: [HACKERS] psql \watch versus \timing
On Fri, Aug 29, 2014 at 6:33 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/28/2014 02:46 PM, Fujii Masao wrote: On Tue, Aug 26, 2014 at 4:55 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/25/2014 10:48 PM, Heikki Linnakangas wrote: Actually, perhaps it would be better to just copy-paste PSQLexec, and modify the copy to suite \watch's needs. (PSQLexecWatch? SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much overlap between what \watch wants and what other PSQLexec callers want. \watch wants timing output, others don't. \watch doesn't want transaction handling. Agreed. Attached is the revised version of the patch. I implemented PSQLexecWatch() which sends the query, prints the results and outputs the query execution time (if \timing is enabled). This patch was marked as ready for committer, but since I revised the code very much, I marked this as needs review again. This comment: ... We use PSQLexecWatch, !* which is kind of cheating, but SendQuery doesn't let us suppress !* autocommit behavior. is a bit strange now. PSQLexecWatch isn't cheating like reusing PSQLexec was; it's whole purpose is to run \watch queries. /* * Set up cancellation of 'watch' via SIGINT. We redo this each time * through the loop since it's conceivable something inside PSQLexec * could change sigint_interrupt_jmp. */ This should now say PSQLexecWatch. Other than that, looks good to me. I just tested the patch and this feature works as expected if timing is on and it displays the individual run time of each query kicked by \watch. Note that --echo-hidden does not display the query run during each loop and that this is contrary to the behavior in HEAD so it breaks backward compatibility, but are there really people relying in the existing behavior? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
Michael Paquier michael.paqu...@gmail.com writes: I just tested the patch and this feature works as expected if timing is on and it displays the individual run time of each query kicked by \watch. Note that --echo-hidden does not display the query run during each loop and that this is contrary to the behavior in HEAD so it breaks backward compatibility, but are there really people relying in the existing behavior? ISTM that's an anti-feature anyway, and changing that behavior is a good thing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On 08/28/2014 02:46 PM, Fujii Masao wrote: On Tue, Aug 26, 2014 at 4:55 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 08/25/2014 10:48 PM, Heikki Linnakangas wrote: Actually, perhaps it would be better to just copy-paste PSQLexec, and modify the copy to suite \watch's needs. (PSQLexecWatch? SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much overlap between what \watch wants and what other PSQLexec callers want. \watch wants timing output, others don't. \watch doesn't want transaction handling. Agreed. Attached is the revised version of the patch. I implemented PSQLexecWatch() which sends the query, prints the results and outputs the query execution time (if \timing is enabled). This patch was marked as ready for committer, but since I revised the code very much, I marked this as needs review again. This comment: ... We use PSQLexecWatch, !* which is kind of cheating, but SendQuery doesn't let us suppress !* autocommit behavior. is a bit strange now. PSQLexecWatch isn't cheating like reusing PSQLexec was; it's whole purpose is to run \watch queries. /* * Set up cancellation of 'watch' via SIGINT. We redo this each time * through the loop since it's conceivable something inside PSQLexec * could change sigint_interrupt_jmp. */ This should now say PSQLexecWatch. Other than that, looks good to me. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Tue, Aug 26, 2014 at 4:55 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
On 08/25/2014 10:48 PM, Heikki Linnakangas wrote:
On 08/25/2014 09:22 PM, Fujii Masao wrote:
On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas
hlinnakan...@vmware.com wrote:
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?
Actually, perhaps it would be better to just copy-paste PSQLexec, and
modify the copy to suite \watch's needs. (PSQLexecWatch?
SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much
overlap between what \watch wants and what other PSQLexec callers want.
\watch wants timing output, others don't. \watch doesn't want
transaction handling.
Agreed. Attached is the revised version of the patch. I implemented
PSQLexecWatch() which sends the query, prints the results and outputs
the query execution time (if \timing is enabled).
This patch was marked as ready for committer, but since I revised
the code very much, I marked this as needs review again.
Do we want --echo-hidden to print the \watch'd
query? Not sure..
Per document, --echo-hidden prints the actual queries generated by
backslash command. But \watch doesn't handle backslash commands.
So I think that PSQLexecWatch doesn't need to handle --echo-hidden.
BTW, I just noticed that none of the callers of PSQLexec pass
start_xact=true. So that part of the function is dead code. We might want
to remove it, and replace with a comment noting that PSQLexec never starts a
new transaction block, even in autocommit-off mode. (I know you're hacking
on this, so I didnn't want to joggle your elbow by doing it right now)
Good catch. So I will remove start_xact code later.
Regards,
--
Fujii Masao
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 2687,2693 do_watch(PQExpBuffer query_buf, long sleep)
for (;;)
{
! PGresult *res;
time_t timer;
long i;
--- 2687,2693
for (;;)
{
! int res;
time_t timer;
long i;
***
*** 2701,2759 do_watch(PQExpBuffer query_buf, long sleep)
myopt.title = title;
/*
! * Run the query. We use PSQLexec, which is kind of cheating, but
! * SendQuery doesn't let us suppress autocommit behavior.
*/
! res = PSQLexec(query_buf-data, false);
!
! /* PSQLexec handles failure results and returns NULL */
! if (res == NULL)
! break;
/*
! * If SIGINT is sent while the query is processing, PSQLexec will
! * consume the interrupt. The user's intention, though, is to cancel
! * the entire watch process, so detect a sent cancellation request and
! * exit in this case.
*/
! if (cancel_pressed)
! {
! PQclear(res);
break;
! }
!
! switch (PQresultStatus(res))
! {
! case PGRES_TUPLES_OK:
! printQuery(res, myopt, pset.queryFout, pset.logfile);
! break;
!
! case PGRES_COMMAND_OK:
! fprintf(pset.queryFout, %s\n%s\n\n, title, PQcmdStatus(res));
! break;
!
! case PGRES_EMPTY_QUERY:
! psql_error(_(\\watch cannot be used with an empty query\n));
! PQclear(res);
! return false;
!
! case PGRES_COPY_OUT:
! case PGRES_COPY_IN:
! case PGRES_COPY_BOTH:
! psql_error(_(\\watch cannot be used with COPY\n));
! PQclear(res);
! return false;
!
! default:
! /* other cases should have been handled by PSQLexec */
! psql_error(_(unexpected result status for \\watch\n));
! PQclear(res);
! return false;
! }
!
! PQclear(res);
!
! fflush(pset.queryFout);
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
--- 2701,2720
myopt.title = title;
/*
! * Run the query and print out the results. We use PSQLexecWatch,
! * which is kind of cheating, but SendQuery doesn't let us suppress
! * autocommit behavior.
*/
! res = PSQLexecWatch(query_buf-data, myopt);
/*
! * PSQLexecWatch handles the case where we can no longer
! * repeat the query, and returns 0 or -1.
*/
! if (res == 0)
break;
! if (res == -1)
! return false;
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 497,502 PSQLexec(const char *query, bool start_xact)
--- 497,598
}
+ /*
+ * PSQLexecWatch
+ *
+ * This function is used for \watch command to send the query to
+ * the server and print out the results.
+ *
+ * Returns 1 if the query executed successfully, 0
Re: [HACKERS] psql \watch versus \timing
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 1sMon Aug 18 15:17:41 2014 ?column? -- 1 (1 row) Time: 0.397 ms Watch every 1sMon 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
Re: [HACKERS] psql \watch versus \timing
On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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 1sMon Aug 18 15:17:41 2014 ?column? -- 1 (1 row) Time: 0.397 ms Watch every 1sMon 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On 08/25/2014 09:22 PM, Fujii Masao wrote: On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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? Actually, perhaps it would be better to just copy-paste PSQLexec, and modify the copy to suite \watch's needs. (PSQLexecWatch? SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much overlap between what \watch wants and what other PSQLexec callers want. \watch wants timing output, others don't. \watch doesn't want transaction handling. Do we want --echo-hidden to print the \watch'd query? Not sure.. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On 08/25/2014 10:48 PM, Heikki Linnakangas wrote: On 08/25/2014 09:22 PM, Fujii Masao wrote: On Tue, Aug 26, 2014 at 1:34 AM, Heikki Linnakangas hlinnakan...@vmware.com wrote: 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? Actually, perhaps it would be better to just copy-paste PSQLexec, and modify the copy to suite \watch's needs. (PSQLexecWatch? SendWatchQuery?). PSQLexec doesn't do much, and there isn't very much overlap between what \watch wants and what other PSQLexec callers want. \watch wants timing output, others don't. \watch doesn't want transaction handling. Do we want --echo-hidden to print the \watch'd query? Not sure.. BTW, I just noticed that none of the callers of PSQLexec pass start_xact=true. So that part of the function is dead code. We might want to remove it, and replace with a comment noting that PSQLexec never starts a new transaction block, even in autocommit-off mode. (I know you're hacking on this, so I didnn't want to joggle your elbow by doing it right now) - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
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 1sMon Aug 18 15:17:41 2014 ?column? -- 1 (1 row) Time: 0.397 ms Watch every 1sMon 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? Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
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 1sMon Aug 18 15:17:41 2014 ?column? -- 1 (1 row) Time: 0.397 ms Watch every 1sMon 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. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
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 1sMon Aug 18 15:17:41 2014 ?column? -- 1 (1 row) Time: 0.397 ms Watch every 1sMon 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql \watch versus \timing
On Mon, May 20, 2013 at 7:33 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Jeff Janes jeff.ja...@gmail.com writes:
I'd like to run same query repeatedly and see how long it takes each time.
I thought \watch would be excellent for this, but it turns out that using
\watch suppresses the output of \timing.
Is this intentional, or unavoidable?
\watch uses PSQLexec not SendQuery; the latter implements \timing which
I agree is arguably useful here, but also autocommit/auto-savepoint
behavior which probably isn't a good idea.
It might be a good idea to refactor those two routines into one routine
with some sort of bitmap flags argument to control the various add-on
behaviors, but that seems like not 9.3 material anymore.
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.
BTW, I found that \watch doesn't check for async notifications. Is it useful
to allow \watch to do that? ISTM that it's not so bad idea to use \timing
to continuously check for async notifications. No?
Regards,
--
Fujii Masao
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
***
*** 2690,2695 do_watch(PQExpBuffer query_buf, long sleep)
--- 2690,2696
PGresult *res;
time_t timer;
long i;
+ double elapsed_msec = 0;
/*
* Prepare title for output. XXX would it be better to use the time
***
*** 2701,2710 do_watch(PQExpBuffer query_buf, long sleep)
myopt.title = title;
/*
! * Run the query. We use PSQLexec, which is kind of cheating, but
! * SendQuery doesn't let us suppress autocommit behavior.
*/
! res = PSQLexec(query_buf-data, false);
/* PSQLexec handles failure results and returns NULL */
if (res == NULL)
--- 2702,2711
myopt.title = title;
/*
! * Run the query. We use PSQLexecInternal, which is kind of cheating,
! * but SendQuery doesn't let us suppress autocommit behavior.
*/
! res = PSQLexecInternal(query_buf-data, false, elapsed_msec);
/* PSQLexec handles failure results and returns NULL */
if (res == NULL)
***
*** 2755,2760 do_watch(PQExpBuffer query_buf, long sleep)
--- 2756,2765
fflush(pset.queryFout);
+ /* Possible microtiming output */
+ if (pset.timing)
+ printf(_(Time: %.3f ms\n), elapsed_msec);
+
/*
* Set up cancellation of 'watch' via SIGINT. We redo this each time
* through the loop since it's conceivable something inside PSQLexec
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
***
*** 439,445 AcceptResult(const PGresult *result)
--- 439,459
PGresult *
PSQLexec(const char *query, bool start_xact)
{
+ return PSQLexecInternal(query, start_xact, NULL);
+ }
+
+ /*
+ * Send backdoor queries.
+ *
+ * Measure how long the given query takes if elapsed_msec is not NULL and
+ * \timing is enabled.
+ */
+ PGresult *
+ PSQLexecInternal(const char *query, bool start_xact, double *elapsed_msec)
+ {
PGresult *res;
+ instr_time before;
+ instr_time after;
if (!pset.db)
{
***
*** 483,488 PSQLexec(const char *query, bool start_xact)
--- 497,505
PQclear(res);
}
+ if (elapsed_msec != NULL pset.timing)
+ INSTR_TIME_SET_CURRENT(before);
+
res = PQexec(pset.db, query);
ResetCancelConn();
***
*** 493,498 PSQLexec(const char *query, bool start_xact)
--- 510,522
res = NULL;
}
+ if (elapsed_msec != NULL pset.timing)
+ {
+ INSTR_TIME_SET_CURRENT(after);
+ INSTR_TIME_SUBTRACT(after, before);
+ *elapsed_msec = INSTR_TIME_GET_MILLISEC(after);
+ }
+
return res;
}
*** a/src/bin/psql/common.h
--- b/src/bin/psql/common.h
***
*** 37,42 extern void SetCancelConn(void);
--- 37,44
extern void ResetCancelConn(void);
extern PGresult *PSQLexec(const char *query, bool start_xact);
+ extern PGresult *PSQLexecInternal(const char *query, bool start_xact,
+ double *elapsed_msec);
extern bool SendQuery(const char *query);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql \watch versus \timing
I'd like to run same query repeatedly and see how long it takes each time. I thought \watch would be excellent for this, but it turns out that using \watch suppresses the output of \timing. Is this intentional, or unavoidable? Also, is it just or does the inability to watch more frequently than once a second make it a lot less useful than it could be? Cheers, Jeff
Re: [HACKERS] psql \watch versus \timing
Jeff Janes jeff.ja...@gmail.com writes: I'd like to run same query repeatedly and see how long it takes each time. I thought \watch would be excellent for this, but it turns out that using \watch suppresses the output of \timing. Is this intentional, or unavoidable? \watch uses PSQLexec not SendQuery; the latter implements \timing which I agree is arguably useful here, but also autocommit/auto-savepoint behavior which probably isn't a good idea. It might be a good idea to refactor those two routines into one routine with some sort of bitmap flags argument to control the various add-on behaviors, but that seems like not 9.3 material anymore. Also, is it just or does the inability to watch more frequently than once a second make it a lot less useful than it could be? It did not seem that exciting to me. In particular, we've already found out that \watch with zero delay is a pretty bad idea, so you'd have to make a case for what smaller minimum to use if it's not to be 1 second. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
