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