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

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

*** src/backend/commands/prepare.c    1f53747076d3cb8d83832179c2e8a0ee3d8f2d37
--- src/backend/commands/prepare.c    0b2ffacdca58b5a073fe6a57b3aa2c7d61d317a4
*************** ExecuteQuery(ExecuteStmt *stmt, const ch
*** 174,180 ****
      ParamListInfo paramLI = NULL;
      EState       *estate = NULL;
      Portal        portal;
!     char       *query_string;

      /* Look it up in the hash table */
      entry = FetchPreparedStatement(stmt->name, true);
--- 174,180 ----
      ParamListInfo paramLI = NULL;
      EState       *estate = NULL;
      Portal        portal;
!     char       *prepared_qs;

      /* Look it up in the hash table */
      entry = FetchPreparedStatement(stmt->name, true);
*************** ExecuteQuery(ExecuteStmt *stmt, const ch
*** 205,212 ****
      portal->visible = false;

      /* Copy the plan's saved query string into the portal's memory */
!     query_string = MemoryContextStrdup(PortalGetHeapMemory(portal),
!                                        entry->plansource->query_string);

      /*
       * For CREATE TABLE / AS EXECUTE, we must make a copy of the stored query
--- 205,212 ----
      portal->visible = false;

      /* Copy the plan's saved query string into the portal's memory */
!     prepared_qs = MemoryContextStrdup(PortalGetHeapMemory(portal),
!                                       entry->plansource->query_string);

      /*
       * For CREATE TABLE / AS EXECUTE, we must make a copy of the stored query
*************** ExecuteQuery(ExecuteStmt *stmt, const ch
*** 256,262 ****

      PortalDefineQuery(portal,
                        NULL,
!                       query_string,
                        entry->plansource->commandTag,
                        plan_list,
                        cplan);
--- 256,262 ----

      PortalDefineQuery(portal,
                        NULL,
!                       prepared_qs,
                        entry->plansource->commandTag,
                        plan_list,
                        cplan);

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

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@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