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. +/* 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 -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers