Re: TODO item: list prepared queries

Hi,

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.
Besides the name of the plan and the actual query the view also displays
the timestamp of the preparation. This can help applications decide whether
or not they want to replan an existing prepared query.

Joachim

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(at)candle(dot)pha(dot)pa(dot)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(at)samurai(dot)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(at)samurai(dot)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(at)samurai(dot)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(at)gmx(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Michael Paesold" <mpaesold(at)gmx(dot)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(at)mcknight(dot)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(at)samurai(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)samurai(dot)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(at)sonic(dot)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(at)samurai(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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

Neil Conway <neilc(at)samurai(dot)com> writes:
> The docs need some improvement, but I'm not aware of any major remaining
> issues with the patch.

I object VERY strongly to the part of the patch that inserts a
deparse_query_list() call into exec_parse_message(). That is not a
cheap operation, and imposing that sort of overhead on every Parse
message is entirely unacceptable from a performance point of view.

I see no need for it either. What's wrong with regurgitating the
original source string, which is already saved in prepared queries?

Other than that show-stopper, the patch looks reasonable at first glance.

regards, tom lane

Tom Lane wrote:
> I object VERY strongly to the part of the patch that inserts a
> deparse_query_list() call into exec_parse_message(). That is not a
> cheap operation, and imposing that sort of overhead on every Parse
> message is entirely unacceptable from a performance point of view.

Well, it doesn't insert a deparse_query_list() into the processing of
*every* Parse message -- it only does so for Parse messages that create
named prepared statements. I don't see that there is a fundamental
difference between a named Parse and an SQL-level PREPARE: if adding
deparse_query_list() to one is too expensive, ISTM it is too expensive
for either.

> I see no need for it either. What's wrong with regurgitating the
> original source string, which is already saved in prepared queries?

It is inconsistent to use the string supplied by the client for
protocol-level prepared statements, but to use the SQL produced by
deparsing for SQL PREPARE.

One possibility would be to execute deparse_query_list() in the SRF
(which is what Joachim's patch did originally), but that is fragile: if
a table a prepared statement depends on is dropped, the view will be
broken. We could workaround that by enclosing the deparse_query_list()
call in a PG_TRY block (and displaying a NULL query string for broken
prepared statements), but that doesn't prevent more subtle problems like
the search_path changing.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> Well, it doesn't insert a deparse_query_list() into the processing of
> *every* Parse message -- it only does so for Parse messages that create
> named prepared statements. I don't see that there is a fundamental
> difference between a named Parse and an SQL-level PREPARE: if adding
> deparse_query_list() to one is too expensive, ISTM it is too expensive
> for either.

I quite agree ;-)

> One possibility would be to execute deparse_query_list() in the SRF
> (which is what Joachim's patch did originally), but that is fragile: if
> a table a prepared statement depends on is dropped, the view will be
> broken. We could workaround that by enclosing the deparse_query_list()
> call in a PG_TRY block (and displaying a NULL query string for broken
> prepared statements), but that doesn't prevent more subtle problems like
> the search_path changing.

Arguably, deparsing when the view is read is the only correct way to
handle search-path changes. But I really think that storing the source
string is the most useful as well as fastest definition. The average
application that wants to use this view at all will be looking to see
"did I already prepare FOO". If it's using the query definition string
for this purpose, comparing source text is easy while comparing deparsed
text to source is a nightmare.

regards, tom lane

Tom Lane wrote:
> The average application that wants to use this view at all will be
> looking to see "did I already prepare FOO". If it's using the query
> definition string for this purpose, comparing source text is easy
> while comparing deparsed text to source is a nightmare.

Well, I don't see why an application would want to look at the query
string in the first place -- as you pointed out earlier, using the
prepared statement's name seems a much easier way to identify prepared
statements.

In any case, if we use the query string as supplied by the user, how do
we produce that string in the case of SQL PREPARE? Manually stripping a
"PREPARE ... AS" prefix from the query string is difficult to do
robustly, but it seems (a) expensive (b) inconsistent to deparse the
Query for SQL PREPARE but not for Parse messages. We could just include
the "PREPARE ... AS" prefix for SQL PREPAREs, but that seems ugly.

-Neil

Neil Conway <neilc(at)samurai(dot)com> writes:
> In any case, if we use the query string as supplied by the user, how do
> we produce that string in the case of SQL PREPARE? Manually stripping a
> "PREPARE ... AS" prefix from the query string is difficult to do
> robustly, but it seems (a) expensive (b) inconsistent to deparse the
> Query for SQL PREPARE but not for Parse messages. We could just include
> the "PREPARE ... AS" prefix for SQL PREPAREs, but that seems ugly.

I don't see the problem. Defining the view field as "the string sent to
the server to create the prepared statement" seems perfectly consistent
to me.

In practice, any given application will probably use one method to the
exclusion of the other, and wouldn't notice the "inconsistency" anyway.
If you are using both methods of preparing statements for some reason,
it's not improbable that you would want to know which way a given
statement was created, and seeing the PREPARE in there would be a useful
cue.

regards, tom lane

Tom Lane wrote:
> In practice, any given application will probably use one method to the
> exclusion of the other, and wouldn't notice the "inconsistency" anyway.
> If you are using both methods of preparing statements for some reason,
> it's not improbable that you would want to know which way a given
> statement was created, and seeing the PREPARE in there would be a useful
> cue.

The "from_sql" field of the view is an infinitely better way to
determine the source of the prepared statement.

Anyway, if there was a reasonably cheap way to present the query strings
of protocol-level and SQL prepared statements in the same manner, I
think we should definitely do so. Since there doesn't appear to be one,
I'm content to just use the query string as sent by the user. I'll post
a revised patch that does that soon.

-Neil

On Tue, 2006-01-03 at 18:00 -0500, Neil Conway wrote:
> Anyway, if there was a reasonably cheap way to present the query strings
> of protocol-level and SQL prepared statements in the same manner, I
> think we should definitely do so. Since there doesn't appear to be one,
> I'm content to just use the query string as sent by the user. I'll post
> a revised patch that does that soon.

Attached is the patch I applied to HEAD that uses the query string
supplied by the client, without any rewriting.

-Neil