TODO item: list prepared queries

Joachim Wieland wrote:

I propose the attached patch for the TODO item:

* %Allow pooled connections to list all prepared queries

The patch adds a new SRF and a new view that contain all prepared
queries available in the session.

This looks nice, but for consistency in naming, this should be about
prepared *statements*.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:

Joachim Wieland wrote:

I propose the attached patch for the TODO item:

* %Allow pooled connections to list all prepared queries

The patch adds a new SRF and a new view that contain all prepared
queries available in the session.

This looks nice, but for consistency in naming, this should be about
prepared *statements*.

I have updated the TODO list to use 'statement' more often.

Also, does anyone know what this item means:

o Allow function argument names to be statements from PL/PgSQL

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Mon, Dec 12, 2005 at 12:32:09PM +0100, Peter Eisentraut wrote:

Joachim Wieland wrote:

* %Allow pooled connections to list all prepared queries

This looks nice, but for consistency in naming, this should be about
prepared *statements*.

Okay, the appended patch is basically a s/query/statement/g.

Whoever reviews this patch could also apply this renaming to at least the
hash table in src/backend/commands/prepare.c:

static HTAB *prepared_queries = NULL;

Joachim

On Tue, 2005-12-13 at 00:39 +0100, Joachim Wieland wrote:

Okay, the appended patch is basically a s/query/statement/g.

Barring any objections, I'll review and apply the patch later this week.

-Neil

On Mon, 2005-12-12 at 10:56 +0100, Joachim Wieland wrote:

I propose the attached patch for the TODO item:

* %Allow pooled connections to list all prepared queries

I think we should also return the parameters of each prepared statement.
Probably the best way to do this is to add another column to
pg_prepared_statements, containing an array of parameter type OIDs. I'll
do that before applying the patch.

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,
but I can't see a clean way to do it: I think we'd need to munge the
actual SQL string itself and remove the "PREPARE ..." prefix. Thoughts?

-Neil

Neil Conway wrote:

On Mon, 2005-12-12 at 10:56 +0100, Joachim Wieland wrote:

I propose the attached patch for the TODO item:

* %Allow pooled connections to list all prepared queries

I think we should also return the parameters of each prepared statement.
Probably the best way to do this is to add another column to
pg_prepared_statements, containing an array of parameter type OIDs. I'll
do that before applying the patch.

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,
but I can't see a clean way to do it: I think we'd need to munge the
actual SQL string itself and remove the "PREPARE ..." prefix. Thoughts?

Is there a way to do it in the parser/analyzer, and save only the actual
prepared query instead of the whole thing? We could show additional
columns in the pg_prepared_statements, indicating whether this is
PREPARE (and the statement's name) or a Parse message.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,

That's debatable. Earlier today, I was busy being annoyed all over
again with the way that Bruce set up Parse/Bind/Execute logging to
deliberately obscure the difference between a SQL PREPARE command and a
protocol-level Parse operation. I think it's a good thing to be able to
tell which level a prepared statement came from. Yeah, much of the time
you may not care, but when you do care it's important.

regards, tom lane

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,
but I can't see a clean way to do it: I think we'd need to munge the
actual SQL string itself and remove the "PREPARE ..." prefix. Thoughts?

BTW, pursuant to comments about David's proposal just now --- why is the
patch using text at all, rather than reverse-compiling the prepared
statement's querytree?

regards, tom lane

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,
but I can't see a clean way to do it: I think we'd need to munge the
actual SQL string itself and remove the "PREPARE ..." prefix. Thoughts?

BTW, pursuant to comments about David's proposal just now --- why is the
patch using text at all, rather than reverse-compiling the prepared
statement's querytree?

Well, I think for the driver or application, to recognize queries as their
own, it seems much easier if the query is given exaclty as it was sent. I.e.
even including PREPARE -- if it sent this way. I am not sure of the latter,
but I would prefer to be given the original query string, PREPARE stripped
or not.

This comment is based on my assumption -- hopefully correct -- that the
querytree has indeed changed from the original query. E.g. removed redundant
parenthesis, added casts, etc.

Best Regards,
Michael Paesold

"Michael Paesold" <mpaesold@gmx.at> writes:

Tom Lane wrote:

BTW, pursuant to comments about David's proposal just now --- why is the
patch using text at all, rather than reverse-compiling the prepared
statement's querytree?

Well, I think for the driver or application, to recognize queries as their
own, it seems much easier if the query is given exaclty as it was sent.

Depends on what the intended use of the view is, I suppose --- but I
should think that drivers would tend to just look at the statement name
to decide if it's something they sent, rather than comparing the text
of the body. Showing a reverse-compiled version would be more robust
in the face of cases like a subsequent change of schema search path,
RENAME commands, etc.

Also, while I have not looked at the patch to see where it's getting
the "original text" from, I'll bet lunch that it's wrong. The structure
of the parser doesn't permit easy extraction of the original text
corresponding to just one SQL command.

regards, tom lane

On December 14, 4:58 pm Tom Lane <tgl@sss.pgh.pa.us> wrote:

"Michael Paesold" <mpaesold@gmx.at> writes:

Well, I think for the driver or application, to recognize queries as
their own, it seems much easier if the query is given exaclty as it
was sent.

Depends on what the intended use of the view is, I suppose --- but I
should think that drivers would tend to just look at the statement name
to decide if it's something they sent, rather than comparing the text
of the body.

Well, one could argue that relying on the identifier might be dangerous.
Someone else could prepare a query with the identifier of another
application and thus this application might execute something different
than what it actually wants to, but then we're in the area of how to manage
users and pooled connections.

Anyway as you say it depends on what you want to use the view for. For an
automatised usage the deparsed form is of no value, for your eye however it
might be nicer.

Another problem I just found out: you can drop a table a prepared query is
referring to. Can the reverse-compiling function cope with that situation?
Well, the cleanest solution might be to prevent this in the first place...

Why not just display both versions?

Also, while I have not looked at the patch to see where it's getting
the "original text" from, I'll bet lunch that it's wrong.

It uses the query string that was already in the prepared queries hash
table. This one uses debug_query_string. I'm a poor student and can't
afford paying you a lunch, but I'd offer you a coke if you tell me why this
is wrong ;-)

Joachim

"" <joe@mcknight.de> writes:

It uses the query string that was already in the prepared queries hash
table. This one uses debug_query_string. I'm a poor student and can't
afford paying you a lunch, but I'd offer you a coke if you tell me why this
is wrong ;-)

(1) Multiple statements in same query string. (2) Statements prepared
via paths other than front-door, client-submitted command; for instance,
inside a plpgsql function.

It was OK to not be very accurate about this as long as the string was
just being used for debugging purposes, but if it's going to be exposed
to users then I'm going to demand higher standards --- because those
corner cases *will* come back to us as bug reports.

regards, tom lane

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,

That's debatable. Earlier today, I was busy being annoyed all over
again with the way that Bruce set up Parse/Bind/Execute logging to
deliberately obscure the difference between a SQL PREPARE command and a
protocol-level Parse operation. I think it's a good thing to be able to
tell which level a prepared statement came from. Yeah, much of the time
you may not care, but when you do care it's important.

I have applied the following patch to CVS HEAD to mark client-side
prepare/bind/execute statements with "[client]" so they can be easily
distinguished from SQL commands. I hesitate to apply this logging
change to 8.1.X.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Fri, Dec 30, 2005 at 05:55:23PM -0500, Bruce Momjian wrote:

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,

That's debatable. Earlier today, I was busy being annoyed all over
again with the way that Bruce set up Parse/Bind/Execute logging to
deliberately obscure the difference between a SQL PREPARE command and a
protocol-level Parse operation. I think it's a good thing to be able to
tell which level a prepared statement came from. Yeah, much of the time
you may not care, but when you do care it's important.

I have applied the following patch to CVS HEAD to mark client-side
prepare/bind/execute statements with "[client]" so they can be easily
distinguished from SQL commands. I hesitate to apply this logging
change to 8.1.X.

Could I suggest the reverse? That is, leave client statements alone and
mark server side ones specially. It seems to me that "client" is the "normal"
case and leaving it alone would be less intrusive.

-dg

--
David Gould daveg@sonic.net
If simplicity worked, the world would be overrun with insects.

daveg wrote:

On Fri, Dec 30, 2005 at 05:55:23PM -0500, Bruce Momjian wrote:

Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

One minor irritation is that the query string of prepared statements
created via SQL has "PREPARE ... AS" prefixed to it, whereas statements
prepared via the FE-BE protocol do not. This should probably be fixed,

That's debatable. Earlier today, I was busy being annoyed all over
again with the way that Bruce set up Parse/Bind/Execute logging to
deliberately obscure the difference between a SQL PREPARE command and a
protocol-level Parse operation. I think it's a good thing to be able to
tell which level a prepared statement came from. Yeah, much of the time
you may not care, but when you do care it's important.

I have applied the following patch to CVS HEAD to mark client-side
prepare/bind/execute statements with "[client]" so they can be easily
distinguished from SQL commands. I hesitate to apply this logging
change to 8.1.X.

Could I suggest the reverse? That is, leave client statements alone and
mark server side ones specially. It seems to me that "client" is the "normal"
case and leaving it alone would be less intrusive.

Uh, the problem is that we don't normally mark SQL queries, so marking
only the server prepares and leaving the client prepares alone seems
inconsistent.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

I have applied the following patch to CVS HEAD to mark client-side
prepare/bind/execute statements with "[client]" so they can be easily
distinguished from SQL commands.

There is no such thing as a "client-side prepare/bind/execute"
statement. The distinction is between SQL-level and protocol-level
prepared queries. "[client]" seems wrong; perhaps "[protocol]" could be
used instead?

(I'm not thrilled by the idea of prefixing the statement log with
"[...]" in any case: it makes it more difficult to determine the actual
query string submitted by the user. However I can't see a better
alternative...)

I hesitate to apply this logging change to 8.1.X.

I don't see any reason to do that -- this is not a bug fix. Furthermore,
there are backward-compatibility concerns.

-Neil

Neil Conway wrote:

Bruce Momjian wrote:

I have applied the following patch to CVS HEAD to mark client-side
prepare/bind/execute statements with "[client]" so they can be easily
distinguished from SQL commands.

There is no such thing as a "client-side prepare/bind/execute"
statement. The distinction is between SQL-level and protocol-level
prepared queries. "[client]" seems wrong; perhaps "[protocol]" could be
used instead?

Agreed. I never liked "client" either. It got me confused. I have
changed it to protocol; patch attached.

(I'm not thrilled by the idea of prefixing the statement log with
"[...]" in any case: it makes it more difficult to determine the actual
query string submitted by the user. However I can't see a better
alternative...)

Yep.

I hesitate to apply this logging change to 8.1.X.

I don't see any reason to do that -- this is not a bug fix. Furthermore,
there are backward-compatibility concerns.

Right.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

daveg wrote:

Could I suggest the reverse? That is, leave client statements alone and
mark server side ones specially. It seems to me that "client" is the "normal"
case and leaving it alone would be less intrusive.

Uh, the problem is that we don't normally mark SQL queries, so marking
only the server prepares and leaving the client prepares alone seems
inconsistent.

Yesterday I was going to complain that this patch makes things more
obscure rather than less so. daveg's confusion seems to confirm my
feeling about it. I'll try to think of some wording I like better.

regards, tom lane

Joachim Wieland wrote:

I propose the attached patch for the TODO item:

* %Allow pooled connections to list all prepared queries

Attached is a revised version of this patch, based on some improvements
sent to me offlist by Joachim, as well as some code review and fixes by
myself. Changes:

- the query string in the view is produced by deparsing the parsetree
after the parse-analysis phase using adt/ruleutils (but before the
rewriter or planner have been invoked).

- two new columns: "parameter_types" is an array of oid that contains
the OIDs of the prepared statement's parameters, and "from_sql" is a
boolean field that is true if the prepared statement was prepared via
SQL, and false if it was prepared via the FE/BE protocol.

The docs need some improvement, but I'm not aware of any major remaining
issues with the patch. Comments are welcome -- barring any major
problems, I'll apply the patch tomorrow.

-Neil