Re: [HACKERS] Passing query string to workers

2017-02-21 Thread Rafia Sabih 
On Wed, Feb 22, 2017 at 12:25 PM, Robert Haas  wrote:
> Looks fine to me.  Committed.  I did move es_queryText to what I think
> is a more appropriate location in the structure definition.
>
> Thanks.
>
Many thanks to Robert for committing and to Kuntal and Amit for reviewing.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-21 Thread Robert Haas 
On Tue, Feb 21, 2017 at 9:18 AM, Rafia Sabih
 wrote:
> Done.

Looks fine to me.  Committed.  I did move es_queryText to what I think
is a more appropriate location in the structure definition.

Thanks.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-20 Thread Rafia Sabih 
On Mon, Feb 20, 2017 at 8:35 PM, Kuntal Ghosh 
wrote:
>
> +   char   *query_data;
> +   query_data = estate->es_sourceText;
> estate->es_sourceText is a const char* variable. Assigning this const
> pointer to a non-const pointer violates the rules
> constant-correctness. So, either you should change query_data as const
> char*, or as Robert suggested, you can directly use
> estate->es_sourceText.
>
Done.

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


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

Re: [HACKERS] Passing query string to workers

2017-02-20 Thread Kuntal Ghosh 
On Mon, Feb 20, 2017 at 10:11 AM, Rafia Sabih
 wrote:
> On Sun, Feb 19, 2017 at 10:11 PM, Robert Haas  wrote:
>> +   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.
+   char   *query_data;
+   query_data = estate->es_sourceText;
estate->es_sourceText is a const char* variable. Assigning this const
pointer to a non-const pointer violates the rules
constant-correctness. So, either you should change query_data as const
char*, or as Robert suggested, you can directly use
estate->es_sourceText.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-19 Thread Rafia Sabih 
On Sun, Feb 19, 2017 at 10:11 PM, Robert Haas  wrote:

> On Thu, Feb 16, 2017 at 6:41 PM, Kuntal Ghosh
>  wrote:
> > On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih
> >  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

Re: [HACKERS] Passing query string to workers

2017-02-19 Thread Robert Haas 
On Thu, Feb 16, 2017 at 6:41 PM, Kuntal Ghosh
 wrote:
> On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih
>  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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-16 Thread Kuntal Ghosh 
On Thu, Feb 16, 2017 at 5:47 PM, Rafia Sabih
 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.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-16 Thread Rafia Sabih 
On Thu, Feb 16, 2017 at 5:06 PM, Kuntal Ghosh 
wrote:
>
> >>
> >> Another question is don't we need to set debug_query_string in worker?
> >
> > In the updated version I am setting it in ParallelQueryMain.
> >
> Ahh, I missed that. debug_query_string is used to show the log
> statements. Hence, it should be set.
>
> +   queryString = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT);
> +   debug_query_string = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT);
> +   pgstat_report_activity(STATE_RUNNING, shm_toc_lookup(toc,
> PARALLEL_KEY_QUERY_TEXT));
> Just one lookup is sufficient.
>
> Fixed.

Other that that I updated some comments and other cleanup things. Please
find the attached patch for the revised version.

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


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

Re: [HACKERS] Passing query string to workers

2017-02-16 Thread Kuntal Ghosh 
On Sat, Feb 11, 2017 at 8:38 AM, Rafia Sabih
 wrote:
>
>>
>> Another question is don't we need to set debug_query_string in worker?
>
> In the updated version I am setting it in ParallelQueryMain.
>
Ahh, I missed that. debug_query_string is used to show the log
statements. Hence, it should be set.

> Please find the attached file for the revised version.
>
+   queryString = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT);
+   debug_query_string = shm_toc_lookup(toc, PARALLEL_KEY_QUERY_TEXT);
+   pgstat_report_activity(STATE_RUNNING, shm_toc_lookup(toc,
PARALLEL_KEY_QUERY_TEXT));
Just one lookup is sufficient.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-10 Thread Rafia Sabih 
> >
> > There are some spacing issues in the code. For example,
> > +   estate->es_queryString = (char
> > *)palloc0(strlen(queryDesc->sourceText) + 1);
> > +   /*Estimate space for query text. */
> > pgindent might be helpful to track all such changes.
> >
>
Fixed.


> > +#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xE010)
> > I'm uncomfortable with declaring the same macro in two
> > files(parallel.c, execParallel.c). My suggestion would be to move
> > pgstat_report_activity in ParallelQueryMain instead of
> > ParallelWorkerMain. Then, you can remove the macro definition from
> > parallel.c. Thoughts?
> >

Yes, I also don't see any need of defining it in parallel.c.  I think
> she has kept to report it in pg_stat_activity, but I feel that code
> can also be moved to execParallel.c.
>
> Agree and fixed.


> Another question is don't we need to set debug_query_string in worker?

In the updated version I am setting it in ParallelQueryMain.

Please find the attached file for the revised version.

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


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

Re: [HACKERS] Passing query string to workers

2017-02-10 Thread Amit Kapila 
On Fri, Feb 10, 2017 at 2:54 PM, Kuntal Ghosh
 wrote:
> On Tue, Feb 7, 2017 at 10:19 AM, Rafia Sabih
>  wrote:
>> Thanks a lot Kuntal for the review, please find attached patch for the
>> revised version.
> Few comments on the patch:
>
> There are some spacing issues in the code. For example,
> +   estate->es_queryString = (char
> *)palloc0(strlen(queryDesc->sourceText) + 1);
> +   /*Estimate space for query text. */
> pgindent might be helpful to track all such changes.
>
> +#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xE010)
> I'm uncomfortable with declaring the same macro in two
> files(parallel.c, execParallel.c). My suggestion would be to move
> pgstat_report_activity in ParallelQueryMain instead of
> ParallelWorkerMain. Then, you can remove the macro definition from
> parallel.c. Thoughts?
>

Yes, I also don't see any need of defining it in parallel.c.  I think
she has kept to report it in pg_stat_activity, but I feel that code
can also be moved to execParallel.c.

Another question is don't we need to set debug_query_string in worker?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-10 Thread Kuntal Ghosh 
On Tue, Feb 7, 2017 at 10:19 AM, Rafia Sabih
 wrote:
> Thanks a lot Kuntal for the review, please find attached patch for the
> revised version.
Few comments on the patch:

There are some spacing issues in the code. For example,
+   estate->es_queryString = (char
*)palloc0(strlen(queryDesc->sourceText) + 1);
+   /*Estimate space for query text. */
pgindent might be helpful to track all such changes.

+#define PARALLEL_KEY_QUERY_TEXT UINT64CONST(0xE010)
I'm uncomfortable with declaring the same macro in two
files(parallel.c, execParallel.c). My suggestion would be to move
pgstat_report_activity in ParallelQueryMain instead of
ParallelWorkerMain. Then, you can remove the macro definition from
parallel.c. Thoughts?

And, the value of the macro seems pretty random to me. IMO, it should
be UINT64CONST(0xE007).

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-02-06 Thread Rafia Sabih 
On Mon, Jan 23, 2017 at 2:46 PM, Kuntal Ghosh
 wrote:
> I've looked into the patch. I've some comments regarding that.
>
> +#define PARALLEL_KEY_QUERY_TEXTUINT64CONST(0xE010)
> It should be UINT64CONST(0xE00A)
>
> +   query_len = strlen(query_data) + 1;
> +   shm_toc_estimate_chunk(>estimator, query_len);
> +   shm_toc_estimate_keys(>estimator, 1);
> +
> Adding a comment before this will be helpful. You should maintain the
> same order for estimating and storing the query string. For example,
> as you've estimated the space for query string after estimation of dsa
> space, you should store the same after creating dsa. Besides, I think
> the appropriate place for this would be before planned statement.
Fixed.

> The regression test for select_parallel is failing after applying the
> patch. You can reproduce the scenario by the following sql statements:
>
> CREATE TABLE t1(a int);
> INSERT INTO t1 VALUES (generate_series(1,10));
> SET parallel_setup_cost=0;
> SET parallel_tuple_cost=0;
> SET min_parallel_relation_size=0;
> SET max_parallel_workers_per_gather=4;
> SELECT count(*) FROM t1;
>
> It is showing the following warning.
> WARNING:  problem in alloc set ExecutorState: detected write past
> chunk end in block 0x14f5310, chunk 0x14f6c50
Fixed.

Thanks a lot Kuntal for the review, please find attached patch for the
revised version.
-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


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

Re: [HACKERS] Passing query string to workers

2017-01-23 Thread Kuntal Ghosh 
On Fri, Jan 13, 2017 at 4:25 PM, Rafia Sabih
 wrote:
>
> Please let me know your feedback over the same.
>
I've looked into the patch. I've some comments regarding that.

+#define PARALLEL_KEY_QUERY_TEXTUINT64CONST(0xE010)
It should be UINT64CONST(0xE00A)

+   query_len = strlen(query_data) + 1;
+   shm_toc_estimate_chunk(>estimator, query_len);
+   shm_toc_estimate_keys(>estimator, 1);
+
Adding a comment before this will be helpful. You should maintain the
same order for estimating and storing the query string. For example,
as you've estimated the space for query string after estimation of dsa
space, you should store the same after creating dsa. Besides, I think
the appropriate place for this would be before planned statement.

The regression test for select_parallel is failing after applying the
patch. You can reproduce the scenario by the following sql statements:

CREATE TABLE t1(a int);
INSERT INTO t1 VALUES (generate_series(1,10));
SET parallel_setup_cost=0;
SET parallel_tuple_cost=0;
SET min_parallel_relation_size=0;
SET max_parallel_workers_per_gather=4;
SELECT count(*) FROM t1;

It is showing the following warning.
WARNING:  problem in alloc set ExecutorState: detected write past
chunk end in block 0x14f5310, chunk 0x14f6c50


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-13 Thread Rafia Sabih 
On Thu, Jan 12, 2017 at 6:24 PM, Robert Haas  wrote:
> On Wed, Jan 11, 2017 at 11:12 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Wed, Jan 11, 2017 at 6:36 PM, Tom Lane  wrote:
 That would work, if you had a way to get at the active QueryDesc ...
 but we don't pass that down to executor nodes.
>>
>>> Hmm, that is a bit of a problem.  Do you have a suggestion?
>>
>> Copy that string pointer into the EState, perhaps?
>
> OK, that sounds good.

Thanks a lot Tom and Robert for the suggestions. Please find the
attached file for the revised version of the patch, wherein I added an
extra pointer for queryString in EState and populated it with
queryDesc->sourceText in standard_ExecutorStart. Later, in
ExecInitParallelPlan a token for queryString is created in shared
memory which is used in ExecParallelGetQueryDesc and
ParallelWorkerMain as before (in version 1 of patch).

Please let me know your feedback over the same.

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


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

Re: [HACKERS] Passing query string to workers

2017-01-12 Thread Robert Haas 
On Wed, Jan 11, 2017 at 11:12 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Jan 11, 2017 at 6:36 PM, Tom Lane  wrote:
>>> That would work, if you had a way to get at the active QueryDesc ...
>>> but we don't pass that down to executor nodes.
>
>> Hmm, that is a bit of a problem.  Do you have a suggestion?
>
> Copy that string pointer into the EState, perhaps?

OK, that sounds good.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-11 Thread Tom Lane 
Robert Haas  writes:
> On Wed, Jan 11, 2017 at 6:36 PM, Tom Lane  wrote:
>> That would work, if you had a way to get at the active QueryDesc ...
>> but we don't pass that down to executor nodes.

> Hmm, that is a bit of a problem.  Do you have a suggestion?

Copy that string pointer into the EState, perhaps?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-11 Thread Robert Haas 
On Wed, Jan 11, 2017 at 6:36 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Jan 11, 2017 at 7:37 AM, Tom Lane  wrote:
>>> As far as reproducing the pg_stat_activity query goes, you could probably
>>> grab that string out of the master backend's pgstat entry and pass it over
>>> at parallel query start.  But please don't confuse it with either
>>> debug_query_string or the string referenced by the QueryDesc; I do not
>>> think there's any guarantee that those are the same.
>
>> I think we should pass only the string referenced by the QueryDesc to
>> the worker, and on the worker side report that via debug_query_string
>> and pg_stat_activity and attach it to the QueryDesc itself.  Is that
>> also your view?
>
> That would work, if you had a way to get at the active QueryDesc ...
> but we don't pass that down to executor nodes.

Hmm, that is a bit of a problem.  Do you have a suggestion?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-11 Thread Tom Lane 
Robert Haas  writes:
> On Wed, Jan 11, 2017 at 7:37 AM, Tom Lane  wrote:
>> As far as reproducing the pg_stat_activity query goes, you could probably
>> grab that string out of the master backend's pgstat entry and pass it over
>> at parallel query start.  But please don't confuse it with either
>> debug_query_string or the string referenced by the QueryDesc; I do not
>> think there's any guarantee that those are the same.

> I think we should pass only the string referenced by the QueryDesc to
> the worker, and on the worker side report that via debug_query_string
> and pg_stat_activity and attach it to the QueryDesc itself.  Is that
> also your view?

That would work, if you had a way to get at the active QueryDesc ...
but we don't pass that down to executor nodes.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-11 Thread Robert Haas 
On Wed, Jan 11, 2017 at 7:37 AM, Tom Lane  wrote:
> Rafia Sabih  writes:
>> Approach:
>> A token for query string is created in the shared memory, this token is
>> populated with the query string using the global string --
>> debug_query_string. Now, for each of the worker when
>> ExecGetParallelQueryDesc is called, we retrieve the query text from shared
>> memory and pass it to CreateQueryDesc.
>
> This is simply wrong, because debug_query_string need have nothing to do
> with what the actual parallel query is.  I'm all for sending over the
> correct query, but if you aren't bothering to accurately reproduce the
> master's query descriptor, I think you will increase confusion not
> reduce it.
>
> As far as reproducing the pg_stat_activity query goes, you could probably
> grab that string out of the master backend's pgstat entry and pass it over
> at parallel query start.  But please don't confuse it with either
> debug_query_string or the string referenced by the QueryDesc; I do not
> think there's any guarantee that those are the same.

I think we should pass only the string referenced by the QueryDesc to
the worker, and on the worker side report that via debug_query_string
and pg_stat_activity and attach it to the QueryDesc itself.  Is that
also your view?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Passing query string to workers

2017-01-11 Thread Tom Lane 
Rafia Sabih  writes:
> Approach:
> A token for query string is created in the shared memory, this token is
> populated with the query string using the global string --
> debug_query_string. Now, for each of the worker when
> ExecGetParallelQueryDesc is called, we retrieve the query text from shared
> memory and pass it to CreateQueryDesc.

This is simply wrong, because debug_query_string need have nothing to do
with what the actual parallel query is.  I'm all for sending over the
correct query, but if you aren't bothering to accurately reproduce the
master's query descriptor, I think you will increase confusion not
reduce it.

As far as reproducing the pg_stat_activity query goes, you could probably
grab that string out of the master backend's pgstat entry and pass it over
at parallel query start.  But please don't confuse it with either
debug_query_string or the string referenced by the QueryDesc; I do not
think there's any guarantee that those are the same.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Passing query string to workers

2017-01-10 Thread Rafia Sabih 
Hello everybody,

Currently, query string is not passed to the workers and only master has
it. In the events, when multiple queries are running on a server and for
one query some worker crashes then it becomes quite burdensome to find the
query with the crashed worker, since on the worker crash no query is
displayed.

To fix this, I propose a patch wherein query string is passed to the
workers as well, hence, displayed when worker crashes.

Approach:
A token for query string is created in the shared memory, this token is
populated with the query string using the global string --
debug_query_string. Now, for each of the worker when
ExecGetParallelQueryDesc is called, we retrieve the query text from shared
memory and pass it to CreateQueryDesc.

Next, to ensure that query gets displayed at the time of crash,
BackendStatusArray needs to be populated correctly, specifically for our
purpose, activity needs to be filled with current query. For this I called
pgstat_report_activity in ParallelWorkerMain, with the query string, this
populates workers' tuples in system table -- pgstat_activity as well.
Previously, pgstat_report_activity was only called for master in
exec_simple_query, hence, for workers pgstat_activity remained null.

Results:
Here is an output for artificially created worker crash with and without
the patch.

Without the patch error report on worker crash:
 LOG:  worker process: parallel worker for PID 49739 (PID 49741) was
terminated by signal 11: Segmentation fault

Error report with the patch:
 LOG:  worker process: parallel worker for PID 51757 (PID 51758) was
terminated by signal 11: Segmentation fault
2017-01-11 11:10:27.630 IST [51742] DETAIL:  Failed process was running:
explain analyse select
l_returnflag,
l_linestatus,
sum(l_quantity) as sum_qty,
sum(l_extendedprice) as sum_base_price,
sum(l_extendedprice * (1 - l_discount)) as sum_disc_price,
sum(l_extendedprice * (1 - l_discount) * (1 + l_tax)) as sum_charge,
avg(l_quantity) as avg_qty,
avg(l_extendedprice) as avg_price,
avg(l_discount) as avg_disc,
count(*) as count_order
from
lineitem
where
l_shipdate <= date '1998-12-01' - interval '119' day
group by
l_returnflag,
l_linestatus
order by
l_returnflag,
l_linestatus
LIMIT 1;

Inputs of all sorts are encouraged.
-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


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