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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Markus Wanner <markus@bluegap.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Markus Wanner <markus@bluegap.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers