Re: [HACKERS] Passing query string to workers

On Thu, Feb 16, 2017 at 6:41 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih
> <rafia.sabih@enterprisedb.com> wrote:
>> Other that that I updated some comments and other cleanup things. Please
>> find the attached patch for the revised version.
> Looks good.
>
> It has passed the regression tests (with and without regress mode).
> Query is getting displayed for parallel workers in pg_stat_activity,
> log statements and failed-query statements. Moved status to Ready for
> committer.

The first hunk of this patch says:

+    estate->es_queryString = (char *)
palloc0(strlen(queryDesc->sourceText) + 1);
+    estate->es_queryString = queryDesc->sourceText;

This is all kinds of wrong.

1. If you assign a value to a variable, and then immediately assign
another value to a variable, the first assignment might as well not
have happened.  In other words, this is allocating a string and then
immediately leaking the allocated memory.

2. If the intent was to copy the string in into
estate->es_queryString, ignoring for the moment that you'd need to use
strcpy() rather than an assignment to make that happen, the use of
palloc0 would be unnecessary.  Regular old palloc would be fine,
because you don't need to zero the memory if you're about to overwrite
it with some new contents.  Zeroing isn't free, so it's best not to do
it unnecessarily.

3. Instead of using palloc and then copying the string, you could just
use pstrdup(), which does the whole thing for you.

4. I don't actually think you need to copy the query string at all.
Tom's email upthread referred to copying the POINTER to the string,
not the string itself, and src/backend/executor/README seems to
suggest that FreeQueryDesc can't be called until after ExecutorEnd().
So I think you could replace these two lines of code with
estate->es_queryString = queryDesc->sourceText and call it good.
That's essentially what this is doing anyway, as the code is written.

+/* For passing query text to the workers */

Unnecessary, because it's clear from the name PARALLEL_KEY_QUERY_TEXT.
#define PARALLEL_TUPLE_QUEUE_SIZE              65536
-/*

Don't remove this blank line.

+       query_data = (char *) palloc0(strlen(estate->es_queryString) + 1);
+       strcpy(query_data, estate->es_queryString);

It's unnecessary to copy the query string here; you're going to use it
later on in the very same function.  That code can just refer to
estate->es_queryString directly.

+       const char *es_queryString; /* Query string for passing to workers */

Maybe this should be called es_sourceText, since it's being copied
from querydesc->sourceText?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company