Re: [HACKERS] trailing comment ghost-timing

2014-04-10 Thread Bruce Momjian
On Mon, Mar 31, 2014 at 02:06:28PM -0400, Bruce Momjian wrote:
 Where are we on this?  It seem odd that psql sends /* */ comments to the
 server, but not -- comments.  Should this be documented or changed?
 
 I am confused why changing the behavior would affect the regression test
 output as -- and /* */ comments already appear, and it was stated that
 -- comments are already not sent to the server.
 
 Everyone agreed that suppressing \timing output for a PGRES_EMPTY_QUERY
 return result was not a good idea.

I have applied the attached document patch to document that '--'
comments are not passed to the server, while C-style block comments are.
We can call this a feature.  ;-)

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index 5dce06a..85899d7
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** testdb=gt;
*** 679,684 
--- 679,690 
  xref linkend=SQL-LISTEN and
  xref linkend=SQL-NOTIFY.
  /para
+ 
+ para
+ While C-style block comments are passed to the server for
+ processing and removal, SQL-standard comments are removed by
+ applicationpsql/application.
+ /para
/refsect2
  
refsect2 id=APP-PSQL-meta-commands

-- 
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] trailing comment ghost-timing

2014-03-31 Thread Bruce Momjian
On Sat, Dec 28, 2013 at 08:20:59PM -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  On 2013-12-24 12:27:59 -0500, Tom Lane wrote:
  What I was proposing was that we do include comments in what we send,
  as long as those comments are embedded in the statement text, not
  on lines before it.
 
  The common way I've seen what I've described above done as is something
  like:
  /* File:path/to/some/file.whatever Function:foo::something()
   * Comment: Expensive, but that's ok!
   */
  SELECT here_comes FROM my WHERE some_sql;
 
  If I unerstood what you propose correctly, we'd now drop that comment,
  where we sent it before?
 
 Yeah.  But I just noticed that this would cause the regression test
 results to change dramatically --- right now, most -- comment comments
 get echoed to the output, and that would stop.  So maybe it's not such
 a great idea after all.

Where are we on this?  It seem odd that psql sends /* */ comments to the
server, but not -- comments.  Should this be documented or changed?

I am confused why changing the behavior would affect the regression test
output as -- and /* */ comments already appear, and it was stated that
-- comments are already not sent to the server.

Everyone agreed that suppressing \timing output for a PGRES_EMPTY_QUERY
return result was not a good idea.

-- 
  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] trailing comment ghost-timing

2013-12-28 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 On 2013-12-24 12:27:59 -0500, Tom Lane wrote:
 What I was proposing was that we do include comments in what we send,
 as long as those comments are embedded in the statement text, not
 on lines before it.

 The common way I've seen what I've described above done as is something
 like:
 /* File:path/to/some/file.whatever Function:foo::something()
  * Comment: Expensive, but that's ok!
  */
 SELECT here_comes FROM my WHERE some_sql;

 If I unerstood what you propose correctly, we'd now drop that comment,
 where we sent it before?

Yeah.  But I just noticed that this would cause the regression test
results to change dramatically --- right now, most -- comment comments
get echoed to the output, and that would stop.  So maybe it's not such
a great idea after all.

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] trailing comment ghost-timing

2013-12-27 Thread Andres Freund
On 2013-12-24 12:27:59 -0500, Tom Lane wrote:
 Andres Freund and...@2ndquadrant.com writes:
  This is inconsistent, IMO.  I think if we were to fix things so that
  leading block comments were dropped the same way -- comments are, that
  would also take care of the behavior complained of in this thread.
  There's been some previous discussion of this point, I think.
 
  FWIW, I find dropping comments a rather annoying behaviour. I'd rather
  include dash comments in the statements sent to the server than start
  dropping block comments.
 
 What I was proposing was that we do include comments in what we send,
 as long as those comments are embedded in the statement text, not
 on lines before it.

The common way I've seen what I've described above done as is something
like:
/* File:path/to/some/file.whatever Function:foo::something()
 * Comment: Expensive, but that's ok!
 */
SELECT here_comes FROM my WHERE some_sql;

If I unerstood what you propose correctly, we'd now drop that comment,
where we sent it before?

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] trailing comment ghost-timing

2013-12-24 Thread Fabrízio de Royes Mello
On Tue, Dec 24, 2013 at 5:53 AM, Martijn van Oosterhout klep...@svana.org
wrote:

 On Tue, Dec 24, 2013 at 03:40:58AM +0100, Andreas Karlsson wrote:
  On 12/24/2013 03:17 AM, David Johnston wrote:
  It is not sent to the server as a trailing comment. The following
  file is sent to the server like this.
 
  File:
  /**/;
  /**/
 
  Commands:
  PQexec(..., /**/;);
  PQexec(..., /**/);
 
  If this has to be fixed it should be in the client. I think people
  would complain if we broke the API by starting to throw an exception
  on PQexec with a string containing no actual query.

 (Untested). Isn't this just a case of psql not printing out a timing if
 the server responds with PGRES_EMPTY_QUERY?


Works... look to the attached patch!

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
 Timbira: http://www.timbira.com.br
 Blog sobre TI: http://fabriziomello.blogspot.com
 Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
 Twitter: http://twitter.com/fabriziomello
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index bbdafab..7320f31 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1023,12 +1023,12 @@ SendQuery(const char *query)
 		}
 	}
 
-	PQclear(results);
-
 	/* Possible microtiming output */
-	if (pset.timing)
+	if (pset.timing  PQresultStatus(results) != PGRES_EMPTY_QUERY)
 		printf(_(Time: %.3f ms\n), elapsed_msec);
 
+	PQclear(results);
+
 	/* check for events that may occur during query execution */
 
 	if (pset.encoding != PQclientEncoding(pset.db) 

-- 
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] trailing comment ghost-timing

2013-12-24 Thread Peter Eisentraut
On 12/24/13, 5:40 AM, Fabrízio de Royes Mello wrote:
 (Untested). Isn't this just a case of psql not printing out a timing if
 the server responds with PGRES_EMPTY_QUERY?

 
 Works... look to the attached patch!

That looks reasonable.


-- 
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] trailing comment ghost-timing

2013-12-24 Thread Andres Freund
Hi,

On 2013-12-24 02:05:23 +0100, Erik Rijkers wrote:
 With \timing on, a trailing comment yields a timing.
 
 # test.sql
 select 1;
 
 /*
 select 2
 */
 
 $ psql -f test.sql
  ?column?
 --
 1
 (1 row)
 
 Time: 0.651 ms
 Time: 0.089 ms
 
 I assume it is timing something about that comment (right?).
 
 Confusing and annoying, IMHO.  Is there any chance such trailing 
 ghost-timings can be removed?

Maybe I am thinking to technical here, but why would that be a good
idea? After all, the comment will have triggered sending a statement to
the server and waiting for the result. The user might want to know about
that.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] trailing comment ghost-timing

2013-12-24 Thread Erik Rijkers
On Tue, December 24, 2013 15:19, Andres Freund wrote:
 Hi,

 On 2013-12-24 02:05:23 +0100, Erik Rijkers wrote:
 With \timing on, a trailing comment yields a timing.

 # test.sql
 select 1;

 /*
 select 2
 */

 $ psql -f test.sql
  ?column?
 --
 1
 (1 row)

 Time: 0.651 ms
 Time: 0.089 ms

 I assume it is timing something about that comment (right?).

 Confusing and annoying, IMHO.  Is there any chance such trailing 
 ghost-timings can be removed?

 Maybe I am thinking to technical here, but why would that be a good
 idea? After all, the comment will have triggered sending a statement to
 the server and waiting for the result. The user might want to know about
 that.

Technical or non-technical; it's at least pretty inconsistent:
- it only times with the last comment.
- it only times with the /**/ comment; to time your trailing -- comments you'll 
have to find another solution :)

Obviously it's a minor thing, and I don't care if it does not get removed, but 
I don't think you can argue that it serves
any useful purpose in the current state.


Erik Rijkers





-- 
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] trailing comment ghost-timing

2013-12-24 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 Maybe I am thinking to technical here, but why would that be a good
 idea? After all, the comment will have triggered sending a statement to
 the server and waiting for the result. The user might want to know about
 that.

I agree; if we triggered a server operation, we should report a timing.
Pretending we did not isn't a good solution.

The real question is whether we shouldn't suppress the whole PQexec.
I believe this is very closely related to the question of what we do
with a comment preceding the next command.  Try this experiment:

regression=# set log_statement = 'all';
SET
regression=# /* block comment here */
regression-# select 2+2;
 ?column? 
--
4
(1 row)

regression=# -- dash comment here
regression=# select 3+3;
 ?column? 
--
6
(1 row)

and then look in the postmaster log:

LOG:  statement: /* block comment here */
select 2+2;
LOG:  statement: select 3+3;

This is inconsistent, IMO.  I think if we were to fix things so that
leading block comments were dropped the same way -- comments are, that
would also take care of the behavior complained of in this thread.
There's been some previous discussion of this point, I think.

A related issue is that psql suppresses -- comments after the start of
the SQL statement as well as before it.  I think that this is probably not
such a good idea anymore; it has the potential at least to screw up
psql's reporting of error locations.  I think what would likely be the
ideal behavior is to drop all leading lines containing only
whitespace/comments, but once we identify the first line of the
statement, transmit verbatim up till the closing semicolon.

Fun corner case:

/* block comment
   here */ SELECT ...

If we try to strip this comment we're definitely going to have issues with
error cursor positions.  So probably the way it will have to work is to
make a check at each end-of-line about whether we are outside any block
comment and haven't seen any SQL token yet, and flush the query collection
buffer if so.

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] trailing comment ghost-timing

2013-12-24 Thread Andres Freund
 The real question is whether we shouldn't suppress the whole PQexec.
 I believe this is very closely related to the question of what we do
 with a comment preceding the next command.  Try this experiment:

 regression=# /* block comment here */
 regression-# select 2+2;

 regression=# -- dash comment here
 regression=# select 3+3;

 and then look in the postmaster log:
 
 LOG:  statement: /* block comment here */
 select 2+2;
 LOG:  statement: select 3+3;
 
 This is inconsistent, IMO.  I think if we were to fix things so that
 leading block comments were dropped the same way -- comments are, that
 would also take care of the behavior complained of in this thread.
 There's been some previous discussion of this point, I think.

FWIW, I find dropping comments a rather annoying behaviour. I'd rather
include dash comments in the statements sent to the server than start
dropping block comments.
It's not uncommon to annotate statements with additional information
using comments for log analysis. Sure, most of that isn't sent via psql,
but it's imo still surpising when testing or re-executing stuff from the log.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
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] trailing comment ghost-timing

2013-12-24 Thread Tom Lane
Andres Freund and...@2ndquadrant.com writes:
 This is inconsistent, IMO.  I think if we were to fix things so that
 leading block comments were dropped the same way -- comments are, that
 would also take care of the behavior complained of in this thread.
 There's been some previous discussion of this point, I think.

 FWIW, I find dropping comments a rather annoying behaviour. I'd rather
 include dash comments in the statements sent to the server than start
 dropping block comments.

What I was proposing was that we do include comments in what we send,
as long as those comments are embedded in the statement text, not
on lines before it.

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] trailing comment ghost-timing

2013-12-24 Thread Andreas Karlsson

On 12/24/2013 08:53 AM, Martijn van Oosterhout wrote:

(Untested). Isn't this just a case of psql not printing out a timing if
the server responds with PGRES_EMPTY_QUERY?


Yes, it is. Sorry should have made myself more clear (way more clear 
when I read my messages from yesterday). Then I thought the server 
should print timing even if PGRES_EMPTY_QUERY is returned. Now after 
thinking on it I am not so sure.


--
Andreas Karlsson


--
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] trailing comment ghost-timing

2013-12-23 Thread Andreas Karlsson

On 12/24/2013 02:05 AM, Erik Rijkers wrote:

With \timing on, a trailing comment yields a timing.

# test.sql
select 1;

/*
select 2
*/

$ psql -f test.sql
  ?column?
--
 1
(1 row)

Time: 0.651 ms
Time: 0.089 ms

I assume it is timing something about that comment (right?).

Confusing and annoying, IMHO.  Is there any chance such trailing ghost-timings 
can be removed?


This seems to be caused by psql sending the comment to the server to 
evaluate as a query. I personally think timing should always output 
something for every command sent to the server. To fix this problem I 
guess one would have to modify psql_scan() to check if a scanned query 
only contains comments and then not send it to the server at all.


The minimal file to reproduce it is:

/**/

--
Andreas Karlsson


--
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] trailing comment ghost-timing

2013-12-23 Thread David Johnston
Andreas Karlsson wrote
 On 12/24/2013 02:05 AM, Erik Rijkers wrote:
 With \timing on, a trailing comment yields a timing.

 # test.sql
 select 1;

 /*
 select 2
 */

 $ psql -f test.sql
   ?column?
 --
  1
 (1 row)

 Time: 0.651 ms
 Time: 0.089 ms

 I assume it is timing something about that comment (right?).

 Confusing and annoying, IMHO.  Is there any chance such trailing
 ghost-timings can be removed?
 
 This seems to be caused by psql sending the comment to the server to 
 evaluate as a query. I personally think timing should always output 
 something for every command sent to the server. To fix this problem I 
 guess one would have to modify psql_scan() to check if a scanned query 
 only contains comments and then not send it to the server at all.
 
 The minimal file to reproduce it is:
 
 /**/
 
 -- 
 Andreas Karlsson

I need to be convinced that the server should not just silently ignore
trailing comments.  I'd consider an exception if the only text sent is a
comment ( in such a case we should throw an error ) but if valid commands
are sent and there is just some comment text at the end it should be ignored
the same as if the comments were embedded in the middle of the query text.

I've encountered other clients that output phantom results in this situation
and solving it at the server seems worthwhile so client applications do not
have to care.

In the example case, I think, putting the comment before the command results
in only one timing.  This inconsistency is a symptom of this situation being
handled incorrectly.

David J.





--
View this message in context: 
http://postgresql.1045698.n5.nabble.com/trailing-comment-ghost-timing-tp5784473p5784484.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


-- 
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] trailing comment ghost-timing

2013-12-23 Thread Andreas Karlsson

On 12/24/2013 03:17 AM, David Johnston wrote:

I need to be convinced that the server should not just silently ignore
trailing comments.  I'd consider an exception if the only text sent is a
comment ( in such a case we should throw an error ) but if valid commands
are sent and there is just some comment text at the end it should be ignored
the same as if the comments were embedded in the middle of the query text.

I've encountered other clients that output phantom results in this situation
and solving it at the server seems worthwhile so client applications do not
have to care.

In the example case, I think, putting the comment before the command results
in only one timing.  This inconsistency is a symptom of this situation being
handled incorrectly.


It is not sent to the server as a trailing comment. The following file 
is sent to the server like this.


File:
/**/;
/**/

Commands:
PQexec(..., /**/;);
PQexec(..., /**/);

If this has to be fixed it should be in the client. I think people would 
complain if we broke the API by starting to throw an exception on PQexec 
with a string containing no actual query.


--
Andreas Karlsson


--
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] trailing comment ghost-timing

2013-12-23 Thread Martijn van Oosterhout
On Tue, Dec 24, 2013 at 03:40:58AM +0100, Andreas Karlsson wrote:
 On 12/24/2013 03:17 AM, David Johnston wrote:
 It is not sent to the server as a trailing comment. The following
 file is sent to the server like this.
 
 File:
 /**/;
 /**/
 
 Commands:
 PQexec(..., /**/;);
 PQexec(..., /**/);
 
 If this has to be fixed it should be in the client. I think people
 would complain if we broke the API by starting to throw an exception
 on PQexec with a string containing no actual query.

(Untested). Isn't this just a case of psql not printing out a timing if
the server responds with PGRES_EMPTY_QUERY?

Have a nice day,
-- 
Martijn van Oosterhout   klep...@svana.org   http://svana.org/kleptog/
 He who writes carelessly confesses thereby at the very outset that he does
 not attach much importance to his own thoughts.
   -- Arthur Schopenhauer


signature.asc
Description: Digital signature