Re: Passing query string to workers

On Sun, Feb 19, 2017 at 10:11 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Feb 16, 2017 at 6:41 PM, Kuntal Ghosh
> <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
> > On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih
> > <rafia(dot)sabih(at)enterprisedb(dot)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.
>
> Fixed.

> +/* For passing query text to the workers */
>
> Unnecessary, because it's clear from the name PARALLEL_KEY_QUERY_TEXT.
>

True, done.

>
> #define PARALLEL_TUPLE_QUEUE_SIZE 65536
> -
> /*
>
> Don't remove this blank line.
>

Done.

>
> + 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.
>

Done.

>
> + 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?
>

Done.

Please find the attached patch for the revised version.

--
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/