On Sun, Feb 19, 2017 at 10:11 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Thu, Feb 16, 2017 at 6:41 PM, Kuntal Ghosh > <kuntalghosh.2...@gmail.com> wrote: > > On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih > > <rafia.sa...@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. > > 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/
pass_queryText_to_workers_v6.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
