Re: [HACKERS] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-29 Thread Robert Haas 
On Wed, Mar 29, 2017 at 12:02 AM, Rafia Sabih
 wrote:
> On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas  wrote:
>> OK, but don't pg_event_trigger_dropped_objects and
>> pg_event_trigger_ddl_commands need the same treatment?
>>
> Done.
> I was only concentrating on the build farm failure cases, otherwise I
> think more work might be required in this direction.

Sure, but there's some happy medium between checking every line in
pg_proc.h for an error and checking nothing other than the functions
immediately.  In this case, there's a group of 4 similarly-named
functions with a similar problem and you changed only the middle two.
Trying to audit the entire file for other mistakes is probably a
fruitless response to this discovery, but auditing the other functions
defined in the same file and with the same naming pattern as the one
you changed seems like something you should do.

Anyway, thanks for debugging this.  Committed this version.

-- 
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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-28 Thread Rafia Sabih 
On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas  wrote:
> OK, but don't pg_event_trigger_dropped_objects and
> pg_event_trigger_ddl_commands need the same treatment?
>
Done.
I was only concentrating on the build farm failure cases, otherwise I
think more work might be required in this direction.

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


system_defined_fn_update_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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-28 Thread Robert Haas 
On Mon, Mar 27, 2017 at 11:57 PM, Rafia Sabih
 wrote:
> On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas  wrote:
>>
>> If it's just that they are relying on unsynchronized global variables,
>> then it's sufficient to mark them parallel-restricted ('r').  Do we
>> really need to go all the way to parallel-unsafe ('u')?
>>
> Done.

OK, but don't pg_event_trigger_dropped_objects and
pg_event_trigger_ddl_commands need the same treatment?

-- 
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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-27 Thread Rafia Sabih 
On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas  wrote:
>
> If it's just that they are relying on unsynchronized global variables,
> then it's sufficient to mark them parallel-restricted ('r').  Do we
> really need to go all the way to parallel-unsafe ('u')?
>
Done.

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


system_defined_fn_update_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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-27 Thread Robert Haas 
On Mon, Mar 27, 2017 at 9:25 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
>>  wrote:
>>> This is caused because trigger related functions are marked safe and
>>> using global variables, hence when executed in parallel are giving
>>> incorrect  output.
>
>> If it's just that they are relying on unsynchronized global variables,
>> then it's sufficient to mark them parallel-restricted ('r').  Do we
>> really need to go all the way to parallel-unsafe ('u')?
>
> Color me confused, but under what circumstances would triggers get
> executed by a parallel worker at all?  I thought we did not allow
> updating queries to be parallelized.

Right, we don't.  But if the updating query fires a trigger, and the
trigger runs an SQL statement that is itself safe for parallelism,
*that* statement could be run in parallel.  In almost all cases it
won't make sense because triggers aren't likely to contain SQL
statements that are expensive enough to justify running them in
parallel, but there's no a priori reason to disallow it.

(And, indeed, I think this commit and the subsequent breakage shows
the value of making sure that parallel query is allowed in as many
places as possible.  Running the regression tests with
force_parallel_mode=regress is the best automated tool we have to find
cases where functions are labeled as being more safe than they
actually are, but those tests only try inserting the invisible
single-copy Gather node in cases where parallel query is allowable at
all.)

-- 
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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-27 Thread Tom Lane 
Robert Haas  writes:
> On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
>  wrote:
>> This is caused because trigger related functions are marked safe and
>> using global variables, hence when executed in parallel are giving
>> incorrect  output.

> If it's just that they are relying on unsynchronized global variables,
> then it's sufficient to mark them parallel-restricted ('r').  Do we
> really need to go all the way to parallel-unsafe ('u')?

Color me confused, but under what circumstances would triggers get
executed by a parallel worker at all?  I thought we did not allow
updating queries to be parallelized.

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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-27 Thread Robert Haas 
On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih
 wrote:
> This is caused because trigger related functions are marked safe and
> using global variables, hence when executed in parallel are giving
> incorrect  output. Attached patch fixes this. I have modified only
> those functions that are showing incorrect behaviour in the regress
> output and checked regression test with force_parallel_mode = regress
> and all testcases are passing now.

If it's just that they are relying on unsynchronized global variables,
then it's sufficient to mark them parallel-restricted ('r').  Do we
really need to go all the way to parallel-unsafe ('u')?

> This concerns me that should we be checking all the system defined
> functions once again if they are actually parallel safe...?

It's certainly possible that we've made mistakes in other places, too.

-- 
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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-26 Thread Rafia Sabih 
On Sun, Mar 26, 2017 at 3:34 AM, Tom Lane  wrote:
> I wrote:
>> It doesn't seem to be a platform-specific problem: I can duplicate the
>> failure here by applying same settings mandrill uses, ie build with
>> -DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.
>
> Oh ... scratch that: you don't even need -DRANDOMIZE_ALLOCATED_MEMORY.
> For some reason I just assumed that any parallelism-related patch
> would have been tested with force_parallel_mode = regress.  This one
> evidently was not.
>
> regards, tom lane
>

This is caused because trigger related functions are marked safe and
using global variables, hence when executed in parallel are giving
incorrect  output. Attached patch fixes this. I have modified only
those functions that are showing incorrect behaviour in the regress
output and checked regression test with force_parallel_mode = regress
and all testcases are passing now.

This concerns me that should we be checking all the system defined
functions once again if they are actually parallel safe...?

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


system_defined_fn_update.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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-25 Thread Tom Lane 
I wrote:
> It doesn't seem to be a platform-specific problem: I can duplicate the
> failure here by applying same settings mandrill uses, ie build with
> -DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.

Oh ... scratch that: you don't even need -DRANDOMIZE_ALLOCATED_MEMORY.
For some reason I just assumed that any parallelism-related patch
would have been tested with force_parallel_mode = regress.  This one
evidently was not.

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] [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.

2017-03-25 Thread Tom Lane 
Robert Haas  writes:
> Improve access to parallel query from procedural languages.

Mandrill has been failing since this patch went in, eg

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill=2017-03-25%2021%3A34%3A08

It doesn't seem to be a platform-specific problem: I can duplicate the
failure here by applying same settings mandrill uses, ie build with
-DRANDOMIZE_ALLOCATED_MEMORY and set force_parallel_mode = regress.

I doubt that the bug is directly in that patch; more likely it's
exposed a pre-existing bug in parallel logic related to triggers.

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