From: | Rafia Sabih <rafia(dot)sabih(at)enterprisedb(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Passing query string to workers |
Date: | 2017-02-20 04:41:35 |
Message-ID: | CAOGQiiOnS5KoH50du4gBi+kqosq-DNcd+n22ZNMTLG8ex-BGzQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
Attachment | Content-Type | Size |
---|---|---|
pass_queryText_to_workers_v6.patch | application/octet-stream | 4.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2017-02-20 05:17:25 | fd,c just Assert()s that lseek() succeeds |
Previous Message | Devrim Gündüz | 2017-02-20 04:33:32 | Re: drop support for Python 2.3 |