pgsql: Adjust things so that the query_string of a cached plan and the

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.

ITAGAKI Takahiro and Tom Lane

Modified Files:
--------------
pgsql/src/backend/commands:
portalcmds.c (r1.74 -> r1.75)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/portalcmds.c?r1=1.74&r2=1.75)
prepare.c (r1.87 -> r1.88)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/prepare.c?r1=1.87&r2=1.88)
trigger.c (r1.235 -> r1.236)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/trigger.c?r1=1.235&r2=1.236)
pgsql/src/backend/executor:
spi.c (r1.196 -> r1.197)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/spi.c?r1=1.196&r2=1.197)
pgsql/src/backend/parser:
analyze.c (r1.372 -> r1.373)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/analyze.c?r1=1.372&r2=1.373)
pgsql/src/backend/tcop:
postgres.c (r1.553 -> r1.554)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c?r1=1.553&r2=1.554)
utility.c (r1.294 -> r1.295)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/utility.c?r1=1.294&r2=1.295)
pgsql/src/backend/utils/cache:
plancache.c (r1.18 -> r1.19)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/cache/plancache.c?r1=1.18&r2=1.19)
pgsql/src/backend/utils/mmgr:
portalmem.c (r1.110 -> r1.111)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/mmgr/portalmem.c?r1=1.110&r2=1.111)
pgsql/src/include/utils:
plancache.h (r1.11 -> r1.12)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/plancache.h?r1=1.11&r2=1.12)
portal.h (r1.78 -> r1.79)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/portal.h?r1=1.78&r2=1.79)

Hi,

Tom Lane wrote:
> 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....

This commit added a variable 'query_string' to the function
ExecuteQuery() in src/backend/commands/prepare.c, but that function
already takes an argument named 'queryString'. What's the difference?
Which is which? Do we need both?

It looks like the second is the query string of the prepare statement,
where the string passed as an argument contains the EXECUTE command. I
propose renaming the variable (as in the attached patch) or at least
explaining it better in additional comments.

Sorry, if this is nitpicking. I just happened to stumbled over it and
thought I better tell you.

Regards

Markus

Markus Wanner <markus(at)bluegap(dot)ch> writes:
> This commit added a variable 'query_string' to the function
> ExecuteQuery() in src/backend/commands/prepare.c, but that function
> already takes an argument named 'queryString'. What's the difference?
> Which is which? Do we need both?

The query_string variable is the original PREPARE's query_string copied
into the portal's context, which we do to ensure that it lives as long
as the portal does. There's no guarantee that the CachedPlanSource
will survive that long (there could be a DEALLOCATE while the query
executes).

The one passed in is the query string for the EXECUTE statement.
I think it's just used for error reporting in EvaluateParams.

> I propose renaming the variable (as in the attached patch) or at least
> explaining it better in additional comments.

This seems like a bad idea, because it makes the code gratuitously
different from the names used for this purpose everywhere else.

regards, tom lane

Hi,

Tom Lane wrote:
> This seems like a bad idea, because it makes the code gratuitously
> different from the names used for this purpose everywhere else.

I find that a pretty dubious reason for having 'query_string' and
'queryString' in the same function. In fact, having it in the same code
base seems strange. It makes me wish we had (better!) naming
conventions... Something I've stumbled over often enough during my work
with Postgres - What was it again: 'query_string' (87 times),
'queryString' (77 times) or 'querystr' (42 times)?

However, what about at least adding a comment, so fellow hackers have a
chance of understanding the subtle difference there?

Regards

Markus

Markus Wanner <markus(at)bluegap(dot)ch> writes:
> However, what about at least adding a comment, so fellow hackers have a
> chance of understanding the subtle difference there?

Sure, done.

regards, tom lane

This commit mentions checking 'debug_query_string' again; exactly what
needs checking?

---------------------------------------------------------------------------

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.
>
> ITAGAKI Takahiro and Tom Lane
>
> Modified Files:
> --------------
> pgsql/src/backend/commands:
> portalcmds.c (r1.74 -> r1.75)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/portalcmds.c?r1=1.74&r2=1.75)
> prepare.c (r1.87 -> r1.88)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/prepare.c?r1=1.87&r2=1.88)
> trigger.c (r1.235 -> r1.236)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/commands/trigger.c?r1=1.235&r2=1.236)
> pgsql/src/backend/executor:
> spi.c (r1.196 -> r1.197)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/executor/spi.c?r1=1.196&r2=1.197)
> pgsql/src/backend/parser:
> analyze.c (r1.372 -> r1.373)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/parser/analyze.c?r1=1.372&r2=1.373)
> pgsql/src/backend/tcop:
> postgres.c (r1.553 -> r1.554)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/postgres.c?r1=1.553&r2=1.554)
> utility.c (r1.294 -> r1.295)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/tcop/utility.c?r1=1.294&r2=1.295)
> pgsql/src/backend/utils/cache:
> plancache.c (r1.18 -> r1.19)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/cache/plancache.c?r1=1.18&r2=1.19)
> pgsql/src/backend/utils/mmgr:
> portalmem.c (r1.110 -> r1.111)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/mmgr/portalmem.c?r1=1.110&r2=1.111)
> pgsql/src/include/utils:
> plancache.h (r1.11 -> r1.12)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/plancache.h?r1=1.11&r2=1.12)
> portal.h (r1.78 -> r1.79)
> (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/portal.h?r1=1.78&r2=1.79)
>
> --
> Sent via pgsql-committers mailing list (pgsql-committers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-committers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> This commit mentions checking 'debug_query_string' again; exactly what
> needs checking?

Mainly, whether we should still be using it at all anywhere.

regards, tom lane

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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)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(at)momjian(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +