Обсуждение: Re: [COMMITTERS] pgsql: Improve access to parallel query from procedural languages.
Robert Haas <rhaas@postgresql.org> 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&dt=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
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
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Rafia Sabih
Дата:
On Sun, Mar 26, 2017 at 3:34 AM, Tom Lane <tgl@sss.pgh.pa.us> 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/
Вложения
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Robert Haas
Дата:
On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih <rafia.sabih@enterprisedb.com> 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
Robert Haas <robertmhaas@gmail.com> writes: > On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih > <rafia.sabih@enterprisedb.com> 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
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Robert Haas
Дата:
On Mon, Mar 27, 2017 at 9:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Mon, Mar 27, 2017 at 1:48 AM, Rafia Sabih >> <rafia.sabih@enterprisedb.com> 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
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Rafia Sabih
Дата:
On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas <robertmhaas@gmail.com> 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/
Вложения
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Robert Haas
Дата:
On Mon, Mar 27, 2017 at 11:57 PM, Rafia Sabih <rafia.sabih@enterprisedb.com> wrote: > On Mon, Mar 27, 2017 at 5:54 PM, Robert Haas <robertmhaas@gmail.com> 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
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Rafia Sabih
Дата:
On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> 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/
Вложения
Re: [COMMITTERS] pgsql: Improve access to parallel queryfrom procedural languages.
От
Robert Haas
Дата:
On Wed, Mar 29, 2017 at 12:02 AM, Rafia Sabih <rafia.sabih@enterprisedb.com> wrote: > On Tue, Mar 28, 2017 at 9:05 PM, Robert Haas <robertmhaas@gmail.com> 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