Thread: Re: [COMMITTERS] pgsql: Adjust things so that the query_string of a cached plan and the

Tom Lane wrote:
> Log Message:
> -----------
> Adjust things so that the query_string of a cached plan and the sourceText of
> a portal are never NULL, but reliably provide the source text of the query.
> It turns out that there was only one place that was really taking a short-cut,
> which was the 'EXECUTE' utility statement.  That doesn't seem like a
> sufficiently critical performance hotspot to justify not offering a guarantee
> of validity of the portal source text.  Fix it to copy the source text over
> from the cached plan.  Add Asserts in the places that set up cached plans and
> portals to reject null source strings, and simplify a bunch of places that
> formerly needed to guard against nulls.
> 
> There may be a few places that cons up statements for execution without
> having any source text at all; I found one such in ConvertTriggerToFK().
> It seems sufficient to inject a phony source string in such a case,
> for instance
>         ProcessUtility((Node *) atstmt,
>                        "(generated ALTER TABLE ADD FOREIGN KEY command)",
>                        NULL, false, None_Receiver, NULL);
> 
> We should take a second look at the usage of debug_query_string,
> particularly the recently added current_query() SQL function.

I looked at the use of 'debug_query_string';  I didn't see how
current_query() could access the more concise query string that is
stored in various structures (comment added);  it works now by accessing
the global variable 'debug_query_string'.

I think we should keep the use of 'debug_query_string' as output in the
server logs unchanged because we need all the queries printed in the
logs in one line.

I did update the comment next to debug_query_string to mention it is the
_client_ query string;  I am afraid renaming it to 'client_query_string'
would break 3rd party apps.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> We should take a second look at the usage of debug_query_string,
>> particularly the recently added current_query() SQL function.

> I looked at the use of 'debug_query_string';  I didn't see how
> current_query() could access the more concise query string that is
> stored in various structures (comment added);  it works now by accessing
> the global variable 'debug_query_string'.

The alternative I was envisioning was to have it look at the
ActivePortal's query string.  However, if you prefer to define the
function as returning the current client query, it's fine as-is.
We should make sure the documentation explains it like that however.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> We should take a second look at the usage of debug_query_string,
> >> particularly the recently added current_query() SQL function.
>
> > I looked at the use of 'debug_query_string';  I didn't see how
> > current_query() could access the more concise query string that is
> > stored in various structures (comment added);  it works now by accessing
> > the global variable 'debug_query_string'.
>
> The alternative I was envisioning was to have it look at the
> ActivePortal's query string.  However, if you prefer to define the
> function as returning the current client query, it's fine as-is.
> We should make sure the documentation explains it like that however.

Oh, I certainly didn't like current_query() using 'debug_query_string'.

Now that you told me about ActivePortal I have used that and it seems to
work fine.  Patch attached and applied; documentation updated.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: doc/src/sgml/func.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.465
diff -c -c -r1.465 func.sgml
*** doc/src/sgml/func.sgml    31 Dec 2008 00:08:33 -0000    1.465
--- doc/src/sgml/func.sgml    7 Jan 2009 21:04:02 -0000
***************
*** 11343,11349 ****
        <row>
         <entry><literal><function>current_query</function></literal></entry>
         <entry><type>text</type></entry>
!        <entry>text of the currently executing query (might contain more than one statement)</entry>
        </row>

        <row>
--- 11343,11350 ----
        <row>
         <entry><literal><function>current_query</function></literal></entry>
         <entry><type>text</type></entry>
!        <entry>text of the currently executing query (might match
!          client-supplied query or be internal query string)</entry>
        </row>

        <row>
Index: src/backend/utils/adt/misc.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/misc.c,v
retrieving revision 1.68
diff -c -c -r1.68 misc.c
*** src/backend/utils/adt/misc.c    7 Jan 2009 19:51:21 -0000    1.68
--- src/backend/utils/adt/misc.c    7 Jan 2009 21:04:02 -0000
***************
*** 31,36 ****
--- 31,37 ----
  #include "storage/pmsignal.h"
  #include "storage/procarray.h"
  #include "utils/builtins.h"
+ #include "tcop/pquery.h"
  #include "tcop/tcopprot.h"

  #define atooid(x)  ((Oid) strtoul((x), NULL, 10))
***************
*** 59,69 ****
  Datum
  current_query(PG_FUNCTION_ARGS)
  {
!     /* there is no easy way to access the more concise 'query_string' */
!     if (debug_query_string)
!         PG_RETURN_TEXT_P(cstring_to_text(debug_query_string));
!     else
!         PG_RETURN_NULL();
  }

  /*
--- 60,66 ----
  Datum
  current_query(PG_FUNCTION_ARGS)
  {
!     PG_RETURN_TEXT_P(cstring_to_text(ActivePortal->sourceText));
  }

  /*

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> The alternative I was envisioning was to have it look at the
>> ActivePortal's query string.  However, if you prefer to define the
>> function as returning the current client query, it's fine as-is.
>> We should make sure the documentation explains it like that however.

> Now that you told me about ActivePortal I have used that and it seems to
> work fine.  Patch attached and applied; documentation updated.

Well, hold on a minute.  I said that was an alternative to look at,
not that it was necessarily better.  Can you define in words of one
syllable which queries will be exposed this way?  I don't believe
it's "all of them".
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> The alternative I was envisioning was to have it look at the
> >> ActivePortal's query string.  However, if you prefer to define the
> >> function as returning the current client query, it's fine as-is.
> >> We should make sure the documentation explains it like that however.
> 
> > Now that you told me about ActivePortal I have used that and it seems to
> > work fine.  Patch attached and applied; documentation updated.
> 
> Well, hold on a minute.  I said that was an alternative to look at,
> not that it was necessarily better.  Can you define in words of one
> syllable which queries will be exposed this way?  I don't believe
> it's "all of them".

Well, if you call a pl function, it is going to show you the current SPI
function running rather than the user query.  Because the comment next
to the function says:
 *  Expose the current query to the user (useful in stored procedures)

I assume the portal string is better for stored procedures then
debug_query_string for current_query().

As I remember, there was lots of objection to adding current_query()
because it seemed useless, but the stored procedure logic won us over,
and I assume the portal string is a better representation for that
usage.  In fact, because the application knows the query string, you
could argue that anything that has more information than the client
query string is preferable.

But of course I might be wrong.  ;-)

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:
> Tom Lane wrote:
>> Well, hold on a minute.  I said that was an alternative to look at,
>> not that it was necessarily better.  Can you define in words of one
>> syllable which queries will be exposed this way?  I don't believe
>> it's "all of them".

> Well, if you call a pl function, it is going to show you the current SPI
> function running rather than the user query.  Because the comment next
> to the function says:

>      *  Expose the current query to the user (useful in stored procedures)

> I assume the portal string is better for stored procedures then
> debug_query_string for current_query().

Uh, no, not necessarily.  As an example, if the thing were really
returning the most closely nested query (I'm not sure it is) then
a plpgsql function trying to inspect the value of current_query()
would always get back the result "SELECT current_query()".  Not
too helpful, eh?  So we actually do have to think a little bit
about exactly *which* query we want to return and whether the
ActivePortal can be counted on to be that one.

The good thing about using debug_query_string is that "the current
client query" is well-defined and easy to explain.  I'm worried
whether using ActivePortal isn't likely to result in a rather
implementation-dependent behavior that changes from release to release.

Or maybe it really is the Right Thing ... but I'm not feeling
confident of that.
        regards, tom lane

I wrote:
> The good thing about using debug_query_string is that "the current
> client query" is well-defined and easy to explain.  I'm worried
> whether using ActivePortal isn't likely to result in a rather
> implementation-dependent behavior that changes from release to release.

In fact, it *is* rather implementation-dependent.  Compare what you'll
get from these two functions:

create function plpgsqlf() returns text as '
begin return current_query();
end' language plpgsql;

create function plpgsqlf2() returns text as '
declare t text;
begin for t in select current_query() loop   return t; end loop;
end' language plpgsql;

My testing says that if we use ActivePortal then "select plpgsqlf()"
returns "select plpgsqlf()" but "select plpgsqlf2()" returns
" select current_query()", ie, the query associated with the implicit
plpgsql cursor.  I'm interested to know how you'll document that.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> Well, hold on a minute.  I said that was an alternative to look at,
> >> not that it was necessarily better.  Can you define in words of one
> >> syllable which queries will be exposed this way?  I don't believe
> >> it's "all of them".
> 
> > Well, if you call a pl function, it is going to show you the current SPI
> > function running rather than the user query.  Because the comment next
> > to the function says:
> 
> >      *  Expose the current query to the user (useful in stored procedures)
> 
> > I assume the portal string is better for stored procedures then
> > debug_query_string for current_query().
> 
> Uh, no, not necessarily.  As an example, if the thing were really
> returning the most closely nested query (I'm not sure it is) then
> a plpgsql function trying to inspect the value of current_query()
> would always get back the result "SELECT current_query()".  Not
> too helpful, eh?  So we actually do have to think a little bit
> about exactly *which* query we want to return and whether the
> ActivePortal can be counted on to be that one.

I thought they would be calling this via some function call fastpath but
I can see it being used in SQL functions, now that you mention it.

OK, reverted, but I added a comment we might want to use
ActivePortal->sourceText.

> The good thing about using debug_query_string is that "the current
> client query" is well-defined and easy to explain.  I'm worried
> whether using ActivePortal isn't likely to result in a rather
> implementation-dependent behavior that changes from release to release.
> 
> Or maybe it really is the Right Thing ... but I'm not feeling
> confident of that.

Agreed.  

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +