Thread: BUG #15324: Non-deterministic behaviour from parallelised sub-query

The following bug has been logged on the website:

Bug reference:      15324
Logged by:          Andrew Fletcher
Email address:      andy@prestigedigital.com
PostgreSQL version: 10.5
Operating system:   macOS High Sierra 10.13.6
Description:

Reproductions - 

Make sure postgresql.conf sets max_parallel_workers_per_gather to 2 or
more

max_parallel_workers_per_gather = 2

Repro 1:

Create table from the following sql file -

https://www.dropbox.com/s/3cm643vmugcgkxh/events.sql.zip?dl=0

Execute this query (multiple times!)

select * from events where account in (select account from events where
data->>'page' = 'success.html' limit 3);

Incorrect output -

 account |   type   |           data           
---------+----------+--------------------------
  304873 | pageview | {"page": "success.html"}
  304875 | pageview | {"page": "c.html"}
  304875 | pageview | {"page": "success.html"}
  304885 | pageview | {"page": "a.html"}
  304885 | pageview | {"page": "success.html"}
(5 rows)

Correct output -

 account |   type   |           data           
---------+----------+--------------------------
  304873 | pageview | {"page": "success.html"}
  304875 | pageview | {"page": "c.html"}
  304875 | pageview | {"page": "success.html"}
  304885 | pageview | {"page": "a.html"}
  304885 | pageview | {"page": "success.html"}
  304873 | pageview | {"page": "b.html"}
(6 rows)


Repro 2 -

Create table from the following sql file -

https://www.dropbox.com/s/mzglgm4a5x1mqno/repro1.sql.zip?dl=0

Execute this query (multiple times!)

select * from repro1 where account in (select account from repro1 where page
= 'success.html' limit 3);

Incorrect Output -

 account |     page     
---------+--------------
      14 | a.html
      14 | success.html
      65 | b.html
      65 | success.html
      80 | b.html
      80 | success.html
   24084 | a.html
   24084 | success.html
   24085 | c.html
   24085 | success.html
   24095 | a.html
   24095 | success.html
(12 rows)

Correct output -

 account |     page     
---------+--------------
      14 | a.html
      14 | success.html
      65 | b.html
      65 | success.html
      80 | b.html
      80 | success.html
(6 rows)


Full version string -

PostgreSQL 10.5 on x86_64-apple-darwin17.7.0, compiled by Apple LLVM version
9.1.0 (clang-902.0.39.2), 64-bit

Also reproduced (with slightly different non-determinism) on -

PostgreSQL 9.6.3, compiled by Visual C++ build 1800, 32-bit on Windows 10
Pro 1709, build 16299.547


Known workarounds -

1.  max_parallel_workers_per_gather = 0
2.  Add order by account asc to the subquery (works for both repros)

Hi,

On 2018-08-13 16:14:03 +0000, PG Bug reporting form wrote:
> Execute this query (multiple times!)
> 
> select * from events where account in (select account from events where
> data->>'page' = 'success.html' limit 3);

Well, the subselect with thelimit going to return different results from
run to run. Unless you add an ORDER BY there's no guaranteed order in
which tuples are returned.  So I don't think it's surprising that you're
getting results that differ between runs.

Greetings,

Andres Freund

Marko Tiikkaja <marko@joh.to> writes:
> On Mon, Aug 13, 2018 at 7:35 PM, Andres Freund <andres@anarazel.de> wrote:
>> Well, the subselect with thelimit going to return different results from
>> run to run. Unless you add an ORDER BY there's no guaranteed order in
>> which tuples are returned.  So I don't think it's surprising that you're
>> getting results that differ between runs.

> While this is true, that's missing the point.

Yeah, I agree.  I think probably what's happening is that the sub-select
is getting pushed down to the parallel workers and they are not all
computing the same set of sub-select results, leading to inconsistent
answers at the top level.

Likely, we need to treat the presence of a LIMIT/OFFSET in a sub-select
as making it parallel-unsafe, for exactly the reason that that makes
its results non-deterministic.

            regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2018-08-13 19:26 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> Likely, we need to treat the presence of a LIMIT/OFFSET in a sub-select
>> as making it parallel-unsafe, for exactly the reason that that makes
>> its results non-deterministic.

> Isn't it default behave of LIMIT/OFFSET without ORDER BY clause?

In principle, the planner could prove in some cases that the results
were deterministic even with LIMIT/OFFSET.  BuT I doubt it's worth
the trouble.  I certainly wouldn't advocate for such logic to be
part of a back-patched bug fix.

            regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2018-08-13 19:26 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> Likely, we need to treat the presence of a LIMIT/OFFSET in a sub-select
>> as making it parallel-unsafe, for exactly the reason that that makes
>> its results non-deterministic.

> Isn't it default behave of LIMIT/OFFSET without ORDER BY clause?

In principle, the planner could prove in some cases that the results
were deterministic even with LIMIT/OFFSET.  BuT I doubt it's worth
the trouble.  I certainly wouldn't advocate for such logic to be
part of a back-patched bug fix.

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> Could the planner stick a materialize node there that feeds the same set of
> originally selected records to any parallel executors that end up pulling
> from it?

No.  At least, Materialize as it stands doesn't help.  There's no
provision for pushing rowsets to parallel workers, only pulling
from them, and it would be an extremely nontrivial thing to add
AFAICS (for one thing, it'd make the leader process even more of
a bottleneck than it is already).

Maybe you could do something with dumping a rowset into a tuplestore
and forcing that out to temp files on-disk before starting any of
the parallel workers, then letting them read it in from the temp
files.  But that still looks like a lot of new work and completely
not fit for back-patching, even if we had it.

            regards, tom lane

On Mon, Aug 13, 2018 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> On Mon, Aug 13, 2018 at 7:35 PM, Andres Freund <andres@anarazel.de> wrote:
>>> Well, the subselect with thelimit going to return different results from
>>> run to run. Unless you add an ORDER BY there's no guaranteed order in
>>> which tuples are returned.  So I don't think it's surprising that you're
>>> getting results that differ between runs.
>
>> While this is true, that's missing the point.
>
> Yeah, I agree.  I think probably what's happening is that the sub-select
> is getting pushed down to the parallel workers and they are not all
> computing the same set of sub-select results, leading to inconsistent
> answers at the top level.
>

Your analysis is correct.  The plan for one of the reported query is as follows:

postgres=# explain select * from repro1 where account in (select
account from repro1 where page
postgres(# = 'success.html' limit 3);
                                          QUERY PLAN
-----------------------------------------------------------------------------------------------
 Gather  (cost=1000.71..12727.24 rows=3 width=11)
   Workers Planned: 2
   ->  Hash Semi Join  (cost=0.71..11726.94 rows=1 width=11)
         Hash Cond: (repro1.account = repro1_1.account)
         ->  Parallel Seq Scan on repro1  (cost=0.00..10532.50
rows=454750 width=11)
         ->  Hash  (cost=0.67..0.67 rows=3 width=4)
               ->  Limit  (cost=0.00..0.64 rows=3 width=4)
                     ->  Seq Scan on repro1 repro1_1
(cost=0.00..19627.50 rows=91823 width=4)
                           Filter: ((page)::text = 'success.html'::text)
(9 rows)


As Tom said, it is evident from the plan that the Limit clause is
pushed in the inner-side of the parallel plan and not all the workers
compute the same result set for the inner side.

> Likely, we need to treat the presence of a LIMIT/OFFSET in a sub-select
> as making it parallel-unsafe, for exactly the reason that that makes
> its results non-deterministic.
>

Yeah, one idea could be that we detect this in
max_parallel_hazard_walker during the very first pass it performs on
query-tree.  Basically, in the SubLink node check, we can detect
whether the subselect has Limit/Offset clause and if so, then we can
treat it as parallel_unsafe.  I have tried that way and it prohibits
the parallel plan for the reported queries.  However, I think more
analysis and verification is required to see if it can happen in any
other related cases.  BTW, will there be any problem if we allow
sub-selects which have sortclause even if the Limit/Offset is present?

Let me know if you have already started working on it, otherwise, I
will prepare an initial patch.

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

On Tue, Aug 14, 2018 at 9:40 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 13, 2018 at 10:56 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> As Tom said, it is evident from the plan that the Limit clause is
> pushed in the inner-side of the parallel plan and not all the workers
> compute the same result set for the inner side.
>
>> Likely, we need to treat the presence of a LIMIT/OFFSET in a sub-select
>> as making it parallel-unsafe, for exactly the reason that that makes
>> its results non-deterministic.
>>
>
> Yeah, one idea could be that we detect this in
> max_parallel_hazard_walker during the very first pass it performs on
> query-tree.
>

I have written a patch along those lines.  This is still a WIP patch
and it is mainly to demonstrate what I have in mind.  There is one
test in select_parallel.sql failing after this patch, but I think that
is expected and we need to adjust that test.   Let me know if you see
a flaw in this approach?

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

On Tue, Aug 14, 2018 at 3:52 PM, Marko Tiikkaja <marko@joh.to> wrote:
> On Tue, Aug 14, 2018 at 7:10 AM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>>
>> Yeah, one idea could be that we detect this in
>> max_parallel_hazard_walker during the very first pass it performs on
>> query-tree.  Basically, in the SubLink node check, we can detect
>> whether the subselect has Limit/Offset clause and if so, then we can
>> treat it as parallel_unsafe.  I have tried that way and it prohibits
>> the parallel plan for the reported queries.  However, I think more
>> analysis and verification is required to see if it can happen in any
>> other related cases.
>
>
> This seems broken as well:
>
>     create table qwr(a int not null, b int not null, c text not null);
>     insert into qwr select i, i, (select prosrc from pg_proc where
> oid=11734) from generate_series(1, 128000) i;
>     set parallel_setup_cost to 0;
>     analyze qwr;
>     select count(*) from qwr where (a, b) in (select a, row_number() over()
> from qwr);
>

I am getting below error in above steps:

postgres=#     insert into qwr select i, i, (select prosrc from
pg_proc where oid=11734) from generate_series(1, 128000) i;
ERROR:  null value in column "c" violates not-null constraint
DETAIL:  Failing row contains (1, 1, null).

If I remove 'not null' constraint from column c, then the above
statement works fine, but I am getting below plan which is a serial
plan:

postgres=# Explain select count(*) from qwr where (a, b) in (select a,
row_number() over() from qwr)
                                       QUERY PLAN
-----------------------------------------------------------------------------------------
 Aggregate  (cost=12360.00..12360.01 rows=1 width=8)
   ->  Hash Semi Join  (cost=7272.00..12200.00 rows=64000 width=0)
         Hash Cond: ((qwr.a = qwr_1.a) AND (qwr.b = (row_number() OVER (?))))
         ->  Seq Scan on qwr  (cost=0.00..1847.00 rows=128000 width=8)
         ->  Hash  (cost=4727.00..4727.00 rows=128000 width=12)
               ->  WindowAgg  (cost=0.00..3447.00 rows=128000 width=12)
                     ->  Seq Scan on qwr qwr_1  (cost=0.00..1847.00
rows=128000 width=4)
(7 rows)

I am not sure why I am not seeing the same problem as you.

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

On Tue, Aug 14, 2018 at 4:42 PM, Marko Tiikkaja <marko@joh.to> wrote:
> Hi Amit,
>
>> >
>> > This seems broken as well:
>> >
>> >     create table qwr(a int not null, b int not null, c text not null);
>> >     insert into qwr select i, i, (select prosrc from pg_proc where
>> > oid=11734) from generate_series(1, 128000) i;
>> >     set parallel_setup_cost to 0;
>> >     analyze qwr;
>> >     select count(*) from qwr where (a, b) in (select a, row_number()
>> > over()
>> > from qwr);
>> >
>>
>> I am getting below error in above steps:
>>
>> postgres=#     insert into qwr select i, i, (select prosrc from
>> pg_proc where oid=11734) from generate_series(1, 128000) i;
>> ERROR:  null value in column "c" violates not-null constraint
>> DETAIL:  Failing row contains (1, 1, null).
>
>
> Sorry, try this instead:
>
>  insert into qwr select i, i, (select prosrc from pg_proc where
> oid='ts_debug(regconfig,text)'::regprocedure) from generate_series(1,
> 128000) i;
>

This looks related, but I think this is a different issue.  The real
reason for this case is that row_number is marked as parallel_safe
which seems to be wrong.  I think it should be marked as
parallel_unsafe.   This needs some more analysis w.r.t which other
Window functions has a similar problem.

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

Marko Tiikkaja <marko@joh.to> writes:
> Marking the function parallel safe doesn't seem wrong to me.  The
> non-parallel-safe part is that the input gets fed to it in different order
> in different workers.  And I don't really think that to be the function's
> fault.

So that basically opens the question of whether *any* window function
calculation can safely be pushed down to parallel workers.

Somewhat like the LIMIT/OFFSET case, it seems to me that we could only
expect to do this safely if the row ordering induced by the WINDOW clause
can be proven to be fully deterministic.  The planner has no such smarts
at the moment AFAIR.  In principle you could do it if there were
partitioning/ordering by a primary key, but I'm not excited about the
prospects of that being true often enough in practice to justify making
the check.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> Marking the function parallel safe doesn't seem wrong to me. The
 >> non-parallel-safe part is that the input gets fed to it in different
 >> order in different workers. And I don't really think that to be the
 >> function's fault.

 Tom> So that basically opens the question of whether *any* window
 Tom> function calculation can safely be pushed down to parallel
 Tom> workers.

Grepping the spec for the phrase "possibly non-deterministic" is quite
enlightening. Leaving out non-determinisms caused by timezone or actual
volatility, leaving out cases of non-determinism that we'd call
"stable", and leaving out features like multisets that we don't support
at all, here's the list of interesting cases (comments after each quoted
paragraph are mine):

6.28 <value expression>

  d) An <array value constructor by query>.

i.e. ARRAY(select)

  o) An <aggregate function> that specifies MIN or MAX and that simply
     contains a <value expression> whose declared type is based on a
     character string type, user-defined type, or datetime with time
     zone type.

i.e. MIN(x) is non-deterministic if "x" can have distinguishable values
that compare equal. PG doesn't have that for text or timestamptz, unlike
the spec, but it does for citext or other user-defined types.

  q) An <array aggregate function>.

i.e. array_agg()

  u) A <window function> that specifies ROW_NUMBER, FIRST_VALUE,
     LAST_VALUE, NTH_VALUE, NTILE, LEAD, or LAG, or whose associated
     <window specification> specifies ROWS.

This covers those cases where window functions don't treat peer rows
together.

7.6 <table reference>

  27) A <table reference> is possibly non-deterministic if the simply
      contained <table primary> or <joined table> is possibly
      non-deterministic or if <sample clause> is specified.

i.e. TABLESAMPLE is non-deterministic

7.16 <query specification>

  a) The <set quantifier> DISTINCT is specified and one of the columns
     of T has a data type of character string, user-defined type, TIME
     WITH TIME ZONE, or TIMESTAMP WITH TIME ZONE.

  c) The <select list>, <having clause>, or <window clause> contains a
     reference to a column C of T that has a data type of character
     string, user-defined type, TIME WITH TIME ZONE, or TIMESTAMP WITH
     TIME ZONE, and the functional dependency G C, where G is the set
     consisting of the grouping columns of T, holds in T.

For both the above two cases, if distinguishable values of a type
compare equal, it's non-deterministic which gets into the result.

7.17 <query expression>

  a) The <query expression> contains a <result offset clause>.

  b) The <query expression> contains a <fetch first clause>.

  f) Both of the following are true:

    i) T contains a set operator UNION and ALL is not specified, or T
       contains either of the set operators EXCEPT or INTERSECT.

   ii) At least one of the following is true:

       1) The first or second operand contains a column that has a
          declared type of character string.

       2) The first or second operand contains a column that has a
          declared type of datetime with time zone.

       3) The first or second operand contains a column that has a
          declared type that is a user-defined type.

(I've left out the many clauses which just amount to "if $thing contains
something which is possibly non-deterministic then it is possibly
non-deterministic")

-- 
Andrew (irc:RhodiumToad)

On Tue, Aug 14, 2018 at 9:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Marko Tiikkaja <marko@joh.to> writes:
>> Marking the function parallel safe doesn't seem wrong to me.  The
>> non-parallel-safe part is that the input gets fed to it in different order
>> in different workers.  And I don't really think that to be the function's
>> fault.
>
> So that basically opens the question of whether *any* window function
> calculation can safely be pushed down to parallel workers.
>

I think we can consider it as a parallel-restricted operation.  For
the purpose of testing, I have marked row_number as
parallel-restricted in pg_proc and I get the below plan:

postgres=# Explain select count(*) from qwr where (a, b) in (select a,
row_number() over() from qwr);
                                               QUERY PLAN
--------------------------------------------------------------------------------------------------------
 Aggregate  (cost=46522.12..46522.13 rows=1 width=8)
   ->  Hash Semi Join  (cost=24352.08..46362.12 rows=64001 width=0)
         Hash Cond: ((qwr.a = qwr_1.a) AND (qwr.b = (row_number() OVER (?))))
         ->  Gather  (cost=0.00..18926.01 rows=128002 width=8)
               Workers Planned: 2
               ->  Parallel Seq Scan on qwr  (cost=0.00..18926.01
rows=64001 width=8)
         ->  Hash  (cost=21806.06..21806.06 rows=128002 width=12)
               ->  WindowAgg  (cost=0.00..20526.04 rows=128002 width=12)
                     ->  Gather  (cost=0.00..18926.01 rows=128002 width=4)
                           Workers Planned: 2
                           ->  Parallel Seq Scan on qwr qwr_1
(cost=0.00..18926.01 rows=64001 width=4)
(11 rows)

This seems okay, though the results of the above parallel-execution
are not same as serial-execution.  I think the reason for it is that
we don't get rows in predictable order from workers.

> Somewhat like the LIMIT/OFFSET case, it seems to me that we could only
> expect to do this safely if the row ordering induced by the WINDOW clause
> can be proven to be fully deterministic.  The planner has no such smarts
> at the moment AFAIR.  In principle you could do it if there were
> partitioning/ordering by a primary key, but I'm not excited about the
> prospects of that being true often enough in practice to justify making
> the check.
>

Yeah, I am also not sure if it is worth adding the additional checks.
So, for now, we can treat any window function calculation as
parallel-restricted and if later anybody has a reason strong enough to
relax the restriction for some particular case, we will consider it.

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

Greetings,

* Amit Kapila (amit.kapila16@gmail.com) wrote:
> On Tue, Aug 14, 2018 at 9:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Marko Tiikkaja <marko@joh.to> writes:
> >> Marking the function parallel safe doesn't seem wrong to me.  The
> >> non-parallel-safe part is that the input gets fed to it in different order
> >> in different workers.  And I don't really think that to be the function's
> >> fault.
> >
> > So that basically opens the question of whether *any* window function
> > calculation can safely be pushed down to parallel workers.
>
> I think we can consider it as a parallel-restricted operation.  For
> the purpose of testing, I have marked row_number as
> parallel-restricted in pg_proc and I get the below plan:
>
> postgres=# Explain select count(*) from qwr where (a, b) in (select a,
> row_number() over() from qwr);
>                                                QUERY PLAN
> --------------------------------------------------------------------------------------------------------
>  Aggregate  (cost=46522.12..46522.13 rows=1 width=8)
>    ->  Hash Semi Join  (cost=24352.08..46362.12 rows=64001 width=0)
>          Hash Cond: ((qwr.a = qwr_1.a) AND (qwr.b = (row_number() OVER (?))))
>          ->  Gather  (cost=0.00..18926.01 rows=128002 width=8)
>                Workers Planned: 2
>                ->  Parallel Seq Scan on qwr  (cost=0.00..18926.01
> rows=64001 width=8)
>          ->  Hash  (cost=21806.06..21806.06 rows=128002 width=12)
>                ->  WindowAgg  (cost=0.00..20526.04 rows=128002 width=12)
>                      ->  Gather  (cost=0.00..18926.01 rows=128002 width=4)
>                            Workers Planned: 2
>                            ->  Parallel Seq Scan on qwr qwr_1
> (cost=0.00..18926.01 rows=64001 width=4)
> (11 rows)
>
> This seems okay, though the results of the above parallel-execution
> are not same as serial-execution.  I think the reason for it is that
> we don't get rows in predictable order from workers.

You wouldn't get them in a predictable order even without
parallelization due to the lack of an ordering, so this hardly seems
like an issue.

> > Somewhat like the LIMIT/OFFSET case, it seems to me that we could only
> > expect to do this safely if the row ordering induced by the WINDOW clause
> > can be proven to be fully deterministic.  The planner has no such smarts
> > at the moment AFAIR.  In principle you could do it if there were
> > partitioning/ordering by a primary key, but I'm not excited about the
> > prospects of that being true often enough in practice to justify making
> > the check.
>
> Yeah, I am also not sure if it is worth adding the additional checks.
> So, for now, we can treat any window function calculation as
> parallel-restricted and if later anybody has a reason strong enough to
> relax the restriction for some particular case, we will consider it.

Seems likely that we'll want this at some point, but certainly seems
like new work and not a small bit of it.

Thanks!

Stephen

On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Greetings,
>
> * Amit Kapila (amit.kapila16@gmail.com) wrote:
>> On Tue, Aug 14, 2018 at 9:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > Marko Tiikkaja <marko@joh.to> writes:
>> >> Marking the function parallel safe doesn't seem wrong to me.  The
>> >> non-parallel-safe part is that the input gets fed to it in different order
>> >> in different workers.  And I don't really think that to be the function's
>> >> fault.
>> >
>> > So that basically opens the question of whether *any* window function
>> > calculation can safely be pushed down to parallel workers.
>>
>> I think we can consider it as a parallel-restricted operation.  For
>> the purpose of testing, I have marked row_number as
>> parallel-restricted in pg_proc and I get the below plan:
>>
>> postgres=# Explain select count(*) from qwr where (a, b) in (select a,
>> row_number() over() from qwr);
>>                                                QUERY PLAN
>> --------------------------------------------------------------------------------------------------------
>>  Aggregate  (cost=46522.12..46522.13 rows=1 width=8)
>>    ->  Hash Semi Join  (cost=24352.08..46362.12 rows=64001 width=0)
>>          Hash Cond: ((qwr.a = qwr_1.a) AND (qwr.b = (row_number() OVER (?))))
>>          ->  Gather  (cost=0.00..18926.01 rows=128002 width=8)
>>                Workers Planned: 2
>>                ->  Parallel Seq Scan on qwr  (cost=0.00..18926.01
>> rows=64001 width=8)
>>          ->  Hash  (cost=21806.06..21806.06 rows=128002 width=12)
>>                ->  WindowAgg  (cost=0.00..20526.04 rows=128002 width=12)
>>                      ->  Gather  (cost=0.00..18926.01 rows=128002 width=4)
>>                            Workers Planned: 2
>>                            ->  Parallel Seq Scan on qwr qwr_1
>> (cost=0.00..18926.01 rows=64001 width=4)
>> (11 rows)
>>
>> This seems okay, though the results of the above parallel-execution
>> are not same as serial-execution.  I think the reason for it is that
>> we don't get rows in predictable order from workers.
>
> You wouldn't get them in a predictable order even without
> parallelization due to the lack of an ordering, so this hardly seems
> like an issue.
>

Right.

>> > Somewhat like the LIMIT/OFFSET case, it seems to me that we could only
>> > expect to do this safely if the row ordering induced by the WINDOW clause
>> > can be proven to be fully deterministic.  The planner has no such smarts
>> > at the moment AFAIR.  In principle you could do it if there were
>> > partitioning/ordering by a primary key, but I'm not excited about the
>> > prospects of that being true often enough in practice to justify making
>> > the check.
>>
>> Yeah, I am also not sure if it is worth adding the additional checks.
>> So, for now, we can treat any window function calculation as
>> parallel-restricted and if later anybody has a reason strong enough to
>> relax the restriction for some particular case, we will consider it.
>
> Seems likely that we'll want this at some point, but certainly seems
> like new work and not a small bit of it.
>

Yeah, let me summarize the problems which require patches:
(a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
it parallel-unsafe.
(b) Consider the presence of any window function calculation as
parallel-restricted operation.

Initially, I will prepare two separate patches for them and then we
will see if we want to combine them into one before committing.  It
might take me few days to come up with patches, so if anyone else
wants to take a lead, feel free to do so.

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

On Thu, Aug 16, 2018 at 11:47 AM, Andrew Fletcher
<andy@prestigedigital.com> wrote:
>> (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
>> it parallel-unsafe.
>
>
> Presence of a LIMIT/OFFSET in a sub-select without an ORDER BY?
>

Yes, I wanted to do that, but Tom doesn't seem to like that especially
for back-branches.  Let me try to prepare a patch and then we can see
based on the complexity of fix.

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

On Thu, Aug 16, 2018 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> Greetings,
>>
>
> Yeah, let me summarize the problems which require patches:
> (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
> it parallel-unsafe.
>

As mentioned up-thread, I have considered adding a check in
max_parallel_hazard_walker, but it turns out that it will make the
whole query parallel-unsafe even if one of the sub-selects has
Limit/Offset.  I think the better idea is to detect that during
set_rel_consider_parallel.  Attached patch
prohibit_parallel_limit_subselect_v2 implements the fix for same.

I have also tried to consider waving off the case when Order By is
present in the sub-select, but for that, I think we need to ensure
that Order By is on the same column as the target list.  It is not
clear to me whether adding additional code checks to detect that is
valuable because it can consume planner time for some cases.  Also, I
think it is not advisable to have such checks for the back-branches as
pointed by Tom as well.  So, I didn't do anything about it.

> (b) Consider the presence of any window function calculation as
> parallel-restricted operation.
>

For this, we need to mark all the window functions like row_number,
rank, dense_rank, etc as parallel-restricted.   Additionally, we also
need to detect the presence of aggregate functions that act as window
functions (when an OVER clause follows the call).   Attached patch
treat_window_func_calc_parallel_restricted_v1 implements the fix.

> Initially, I will prepare two separate patches for them and then we
> will see if we want to combine them into one before committing.
>

One can apply patches in the order
prohibit_parallel_limit_subselect_v2 and
treat_window_func_calc_parallel_restricted_v1.

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

On Mon, Aug 20, 2018 at 4:20 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Aug 16, 2018 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> (b) Consider the presence of any window function calculation as
>> parallel-restricted operation.
>>
>
> For this, we need to mark all the window functions like row_number,
> rank, dense_rank, etc as parallel-restricted.   Additionally, we also
> need to detect the presence of aggregate functions that act as window
> functions (when an OVER clause follows the call).   Attached patch
> treat_window_func_calc_parallel_restricted_v1 implements the fix.
>

As this patch changes the catalog contents, we need to bump catalog
version number.  However, I have left it for later once we get a
review and or testing of the patch.

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

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Mon, Aug 20, 2018 at 4:20 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Thu, Aug 16, 2018 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>> (b) Consider the presence of any window function calculation as
>>> parallel-restricted operation.

>> For this, we need to mark all the window functions like row_number,
>> rank, dense_rank, etc as parallel-restricted.   Additionally, we also
>> need to detect the presence of aggregate functions that act as window
>> functions (when an OVER clause follows the call).   Attached patch
>> treat_window_func_calc_parallel_restricted_v1 implements the fix.

> As this patch changes the catalog contents, we need to bump catalog
> version number.  However, I have left it for later once we get a
> review and or testing of the patch.

Sounds to me like you're using the wrong approach.  I would just consider
any Agg or WindowFunc node as parallel-restricted regardless of the
function it references.

            regards, tom lane

On Mon, Aug 20, 2018 at 6:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> On Mon, Aug 20, 2018 at 4:20 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>> On Thu, Aug 16, 2018 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
>>>> (b) Consider the presence of any window function calculation as
>>>> parallel-restricted operation.
>
>>> For this, we need to mark all the window functions like row_number,
>>> rank, dense_rank, etc as parallel-restricted.   Additionally, we also
>>> need to detect the presence of aggregate functions that act as window
>>> functions (when an OVER clause follows the call).   Attached patch
>>> treat_window_func_calc_parallel_restricted_v1 implements the fix.
>
>> As this patch changes the catalog contents, we need to bump catalog
>> version number.  However, I have left it for later once we get a
>> review and or testing of the patch.
>
> Sounds to me like you're using the wrong approach.  I would just consider
> any Agg or WindowFunc node as parallel-restricted regardless of the
> function it references.
>

I have below change in the patch which I think is on the lines what
you are describing, do you have something different in mind?

@@ -1197,6 +1197,19 @@ max_parallel_hazard_walker(Node *node,
max_parallel_hazard_context *context)
  }

  /*
+ * Treat window functions as parallel-restricted as the row ordering
+ * induced by them is non-deterministic.  We can relax this condition for
+ * cases where the row ordering can be deterministic like when there is
+ * an ORDER BY on the primary key, but those cases don't seem to be
+ * interesting enough to have additional checks.
+ */
+ if (IsA(node, WindowFunc))
+ {
+ if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
+ return true;
+ }

In addition to the above, I have marked all built-in window functions
as parallel-restricted.  I think even if we don't do that something
like above check should be sufficient, but OTOH, I don't see any
reason to keep the marking of such functions as parallel-safe.  Is
there a reason, why we shouldn't mark them as parallel-restricted?

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

On Mon, Aug 20, 2018 at 4:20 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 16, 2018 at 9:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, Aug 15, 2018 at 4:40 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> Greetings,
> >>
> >
> > Yeah, let me summarize the problems which require patches:
> > (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
> > it parallel-unsafe.
> >
>
> As mentioned up-thread, I have considered adding a check in
> max_parallel_hazard_walker, but it turns out that it will make the
> whole query parallel-unsafe even if one of the sub-selects has
> Limit/Offset.  I think the better idea is to detect that during
> set_rel_consider_parallel.  Attached patch
> prohibit_parallel_limit_subselect_v2 implements the fix for same.
>

I was trying this patch on back-branches and found that it doesn't
apply cleanly beyond PG11, so created separate patches for 10 and 9.6.
  Further, I found that the test for this patch was not failing for
9.6 (without the patch) even though the code doesn't deal with this
problem.  On further investigation, I found that it is because the
commit
655393a022bd653e2b48dbf20b69236981e35195 has not been backpatched to
9.6.  I don't see any reason why we shouldn't backpatch this commit.
So, I have attached a patch (fix_parallel_hash_path_v1.patch) which we
can backpatch in 9.6.

Robert, your input will be highly appreciated here especially for the
back patch (to 9.6) I am proposing?

> > (b) Consider the presence of any window function calculation as
> > parallel-restricted operation.
> >
>
> For this, we need to mark all the window functions like row_number,
> rank, dense_rank, etc as parallel-restricted.   Additionally, we also
> need to detect the presence of aggregate functions that act as window
> functions (when an OVER clause follows the call).   Attached patch
> treat_window_func_calc_parallel_restricted_v1 implements the fix.
>

On again looking at this patch, I found that the test case in the
patch was not sufficient to reproduce the problem reported here, so I
have changed the test on the lines of what is reported in this thread.

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

On Tue, Aug 21, 2018 at 8:44 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Aug 20, 2018 at 6:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Sounds to me like you're using the wrong approach.  I would just consider
> > any Agg or WindowFunc node as parallel-restricted regardless of the
> > function it references.
> >
>
> I have below change in the patch which I think is on the lines what
> you are describing, do you have something different in mind?
>
> @@ -1197,6 +1197,19 @@ max_parallel_hazard_walker(Node *node,
> max_parallel_hazard_context *context)
>   }
>
>   /*
> + * Treat window functions as parallel-restricted as the row ordering
> + * induced by them is non-deterministic.  We can relax this condition for
> + * cases where the row ordering can be deterministic like when there is
> + * an ORDER BY on the primary key, but those cases don't seem to be
> + * interesting enough to have additional checks.
> + */
> + if (IsA(node, WindowFunc))
> + {
> + if (max_parallel_hazard_test(PROPARALLEL_RESTRICTED, context))
> + return true;
> + }
>
> In addition to the above, I have marked all built-in window functions
> as parallel-restricted.  I think even if we don't do that something
> like above check should be sufficient, but OTOH, I don't see any
> reason to keep the marking of such functions as parallel-safe.  Is
> there a reason, why we shouldn't mark them as parallel-restricted?
>

Tom, do you have input on this?  Is it okay to backpatch this fix?

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

Amit Kapila <amit.kapila16@gmail.com> writes:
> Tom, do you have input on this?  Is it okay to backpatch this fix?

Well,

>> + * Treat window functions as parallel-restricted as the row ordering
>> + * induced by them is non-deterministic.  We can relax this condition for
>> + * cases where the row ordering can be deterministic like when there is
>> + * an ORDER BY on the primary key, but those cases don't seem to be
>> + * interesting enough to have additional checks.

This comment seems fairly confused.  I'd say something like

 * Treat window functions as parallel-restricted because we aren't sure
 * whether the input row ordering is fully deterministic, and the output
 * of window functions might vary across workers if not.  (In some cases,
 * like where the window frame orders by a primary key, we could relax
 * this restriction.  But it doesn't currently seem worth expending extra
 * effort to do so.)

>> In addition to the above, I have marked all built-in window functions
>> as parallel-restricted.  I think even if we don't do that something
>> like above check should be sufficient, but OTOH, I don't see any
>> reason to keep the marking of such functions as parallel-safe.  Is
>> there a reason, why we shouldn't mark them as parallel-restricted?

I am *strongly* against this.  It's unnecessary catalog churn that we
might need to undo someday, and it confuses a property of the window
function infrastructure with a property of individual window functions.
As a counterexample, if a window function were parallel-unsafe for
some reason, we'd surely need to honor that.  More realistically,
someone might add a window function that actually needs to be
parallel-restricted for reasons of its own, but then there would be
no obvious distinction between such a function and one that you'd
hacked up to be marked parallel-restricted even though it's safe in
itself.  If we then do make the sort of optimization suggested in the
comment, it's likely that someone would just s/r/s/g for all the window
functions and thereby break such a function.  Better to retain the
correct per-function markings.

            regards, tom lane

On Mon, Sep 3, 2018 at 12:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>
> >> + * Treat window functions as parallel-restricted as the row ordering
> >> + * induced by them is non-deterministic.  We can relax this condition for
> >> + * cases where the row ordering can be deterministic like when there is
> >> + * an ORDER BY on the primary key, but those cases don't seem to be
> >> + * interesting enough to have additional checks.
>
> This comment seems fairly confused.  I'd say something like
>
>  * Treat window functions as parallel-restricted because we aren't sure
>  * whether the input row ordering is fully deterministic, and the output
>  * of window functions might vary across workers if not.  (In some cases,
>  * like where the window frame orders by a primary key, we could relax
>  * this restriction.  But it doesn't currently seem worth expending extra
>  * effort to do so.)
>

Changed.

> >> In addition to the above, I have marked all built-in window functions
> >> as parallel-restricted.  I think even if we don't do that something
> >> like above check should be sufficient, but OTOH, I don't see any
> >> reason to keep the marking of such functions as parallel-safe.  Is
> >> there a reason, why we shouldn't mark them as parallel-restricted?
>
> I am *strongly* against this.  It's unnecessary catalog churn that we
> might need to undo someday, and it confuses a property of the window
> function infrastructure with a property of individual window functions.
> As a counterexample, if a window function were parallel-unsafe for
> some reason, we'd surely need to honor that.  More realistically,
> someone might add a window function that actually needs to be
> parallel-restricted for reasons of its own, but then there would be
> no obvious distinction between such a function and one that you'd
> hacked up to be marked parallel-restricted even though it's safe in
> itself.  If we then do make the sort of optimization suggested in the
> comment, it's likely that someone would just s/r/s/g for all the window
> functions and thereby break such a function.
>

Sounds sensible, I have removed that part from the patch.

You haven't mentioned anything about backpatching, but I don't see any
problem with backpatching this fix.  So, I have prepared patches for
back branches wherever required.  For 10, it is mostly a cosmetic
change (the patch didn't apply cleanly), but for 9.6 the prohibition
check is slightly different.

I will commit the attached patches in a day or so unless somebody sees
any problem.


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

Amit Kapila <amit.kapila16@gmail.com> writes:
> You haven't mentioned anything about backpatching, but I don't see any
> problem with backpatching this fix.

Yeah, we definitely need to back-patch, and that's another reason not
to touch the catalog contents.

> I will commit the attached patches in a day or so unless somebody sees
> any problem.

Looking more closely at the patch:

* The general design in max_parallel_hazard_walker appears to be that
after the initial check_functions_in_node test, the rest of it should be
an if ... else if ... else if ... else if ... chain of mutually-exclusive
IsA tests.  Whoever stuck in the NextValueExpr test (possibly me?) did so
with a tin ear, and you've duplicated that mistake here.  Please make it
"else if", and fix the NextValueExpr test to be "else if" while at it.
(Or else get rid of all the "else"s, but that would be a shade less
efficient unless the compiler is very very smart.)

* The plan tree for the added test case is hard to read because it's
unclear which tenk1 scan is which.  I'd suggest adding aliases to
clarify that, eg

+explain (costs off, verbose)
+  select count(*) from tenk1 a where (unique1, two) in
+    (select unique1, row_number() over() from tenk1 b);

HEAD patch is OK otherwise.  I didn't look at the back-branch patches.

            regards, tom lane

On Mon, Sep 3, 2018 at 7:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > You haven't mentioned anything about backpatching, but I don't see any
> > problem with backpatching this fix.
>
> Yeah, we definitely need to back-patch, and that's another reason not
> to touch the catalog contents.
>
> > I will commit the attached patches in a day or so unless somebody sees
> > any problem.
>
> Looking more closely at the patch:
>
> * The general design in max_parallel_hazard_walker appears to be that
> after the initial check_functions_in_node test, the rest of it should be
> an if ... else if ... else if ... else if ... chain of mutually-exclusive
> IsA tests.  Whoever stuck in the NextValueExpr test (possibly me?) did so
> with a tin ear, and you've duplicated that mistake here.  Please make it
> "else if", and fix the NextValueExpr test to be "else if" while at it.
> (Or else get rid of all the "else"s, but that would be a shade less
> efficient unless the compiler is very very smart.)
>
> * The plan tree for the added test case is hard to read because it's
> unclear which tenk1 scan is which.  I'd suggest adding aliases to
> clarify that, eg
>
> +explain (costs off, verbose)
> +  select count(*) from tenk1 a where (unique1, two) in
> +    (select unique1, row_number() over() from tenk1 b);
>
> HEAD patch is OK otherwise.  I didn't look at the back-branch patches.
>

Thanks for the review.  After making the changes suggested by you, I
have pushed and back-patched it till 9.6.

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

On Sun, Sep 2, 2018 at 10:18 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Yeah, let me summarize the problems which require patches:
> > > (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
> > > it parallel-unsafe.
> > >
> >
> > As mentioned up-thread, I have considered adding a check in
> > max_parallel_hazard_walker, but it turns out that it will make the
> > whole query parallel-unsafe even if one of the sub-selects has
> > Limit/Offset.  I think the better idea is to detect that during
> > set_rel_consider_parallel.  Attached patch
> > prohibit_parallel_limit_subselect_v2 implements the fix for same.
> >
>
> I was trying this patch on back-branches and found that it doesn't
> apply cleanly beyond PG11, so created separate patches for 10 and 9.6.
>   Further, I found that the test for this patch was not failing for
> 9.6 (without the patch) even though the code doesn't deal with this
> problem.  On further investigation, I found that it is because the
> commit
> 655393a022bd653e2b48dbf20b69236981e35195 has not been backpatched to
> 9.6.  I don't see any reason why we shouldn't backpatch this commit.
> So, I have attached a patch (fix_parallel_hash_path_v1.patch) which we
> can backpatch in 9.6.
>
> Robert, your input will be highly appreciated here especially for the
> back patch (to 9.6) I am proposing?
>

I have rebased the HEAD patch and done some cosmetic changes like
improved the test by giving aliases to table names and modified the
comment a bit, otherwise, the core logic remains the same.  As the
back-branch patches are just the matter of rebasing them, I will do
that before commit.

I am still waiting for input, but if there is none, my plan is to
commit this in a day or two and back-patch it as well.  Along with
this, I would also like to back-patch commit
655393a022bd653e2b48dbf20b69236981e35195 for the reasons mentioned
above.

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

On Wed, Sep 5, 2018 at 10:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sun, Sep 2, 2018 at 10:18 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > > Yeah, let me summarize the problems which require patches:
>> > > (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
>> > > it parallel-unsafe.
>> > >
>> >
>> > As mentioned up-thread, I have considered adding a check in
>> > max_parallel_hazard_walker, but it turns out that it will make the
>> > whole query parallel-unsafe even if one of the sub-selects has
>> > Limit/Offset.  I think the better idea is to detect that during
>> > set_rel_consider_parallel.  Attached patch
>> > prohibit_parallel_limit_subselect_v2 implements the fix for same.
>> >
>>
>> I was trying this patch on back-branches and found that it doesn't
>> apply cleanly beyond PG11, so created separate patches for 10 and 9.6.
>>   Further, I found that the test for this patch was not failing for
>> 9.6 (without the patch) even though the code doesn't deal with this
>> problem.  On further investigation, I found that it is because the
>> commit
>> 655393a022bd653e2b48dbf20b69236981e35195 has not been backpatched to
>> 9.6.  I don't see any reason why we shouldn't backpatch this commit.
>> So, I have attached a patch (fix_parallel_hash_path_v1.patch) which we
>> can backpatch in 9.6.
>>
>> Robert, your input will be highly appreciated here especially for the
>> back patch (to 9.6) I am proposing?
>>
>
> I have rebased the HEAD patch and done some cosmetic changes like
> improved the test by giving aliases to table names and modified the
> comment a bit, otherwise, the core logic remains the same.  As the
> back-branch patches are just the matter of rebasing them, I will do
> that before commit.
>
> I am still waiting for input, but if there is none, my plan is to
> commit this in a day or two and back-patch it as well.  Along with
> this, I would also like to back-patch commit
> 655393a022bd653e2b48dbf20b69236981e35195 for the reasons mentioned
> above.

I have reviewed and tested the patch.  The patch looks fine to me and
behaviour is as expected.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 13, 2018 at 5:30 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Wed, Sep 5, 2018 at 10:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Sun, Sep 2, 2018 at 10:18 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> > > Yeah, let me summarize the problems which require patches:
> >> > > (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
> >> > > it parallel-unsafe.
> >> > >
> >> >
> >> > As mentioned up-thread, I have considered adding a check in
> >> > max_parallel_hazard_walker, but it turns out that it will make the
> >> > whole query parallel-unsafe even if one of the sub-selects has
> >> > Limit/Offset.  I think the better idea is to detect that during
> >> > set_rel_consider_parallel.  Attached patch
> >> > prohibit_parallel_limit_subselect_v2 implements the fix for same.
> >> >
> >>
> >> I was trying this patch on back-branches and found that it doesn't
> >> apply cleanly beyond PG11, so created separate patches for 10 and 9.6.
> >>   Further, I found that the test for this patch was not failing for
> >> 9.6 (without the patch) even though the code doesn't deal with this
> >> problem.  On further investigation, I found that it is because the
> >> commit
> >> 655393a022bd653e2b48dbf20b69236981e35195 has not been backpatched to
> >> 9.6.  I don't see any reason why we shouldn't backpatch this commit.
> >> So, I have attached a patch (fix_parallel_hash_path_v1.patch) which we
> >> can backpatch in 9.6.
> >>
> >> Robert, your input will be highly appreciated here especially for the
> >> back patch (to 9.6) I am proposing?
> >>
> >
> > I have rebased the HEAD patch and done some cosmetic changes like
> > improved the test by giving aliases to table names and modified the
> > comment a bit, otherwise, the core logic remains the same.  As the
> > back-branch patches are just the matter of rebasing them, I will do
> > that before commit.
> >
> > I am still waiting for input, but if there is none, my plan is to
> > commit this in a day or two and back-patch it as well.  Along with
> > this, I would also like to back-patch commit
> > 655393a022bd653e2b48dbf20b69236981e35195 for the reasons mentioned
> > above.
>
> I have reviewed and tested the patch.  The patch looks fine to me and
> behaviour is as expected.
>

Do you agree with my proposal to backpatch commit - 655393a022 to 9.6?

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

On Thu, Sep 13, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Sep 13, 2018 at 5:30 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>
>> On Wed, Sep 5, 2018 at 10:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> > On Sun, Sep 2, 2018 at 10:18 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> >> > > Yeah, let me summarize the problems which require patches:
>> >> > > (a) Consider the presence of a LIMIT/OFFSET in a sub-select as making
>> >> > > it parallel-unsafe.
>> >> > >
>> >> >
>> >> > As mentioned up-thread, I have considered adding a check in
>> >> > max_parallel_hazard_walker, but it turns out that it will make the
>> >> > whole query parallel-unsafe even if one of the sub-selects has
>> >> > Limit/Offset.  I think the better idea is to detect that during
>> >> > set_rel_consider_parallel.  Attached patch
>> >> > prohibit_parallel_limit_subselect_v2 implements the fix for same.
>> >> >
>> >>
>> >> I was trying this patch on back-branches and found that it doesn't
>> >> apply cleanly beyond PG11, so created separate patches for 10 and 9.6.
>> >>   Further, I found that the test for this patch was not failing for
>> >> 9.6 (without the patch) even though the code doesn't deal with this
>> >> problem.  On further investigation, I found that it is because the
>> >> commit
>> >> 655393a022bd653e2b48dbf20b69236981e35195 has not been backpatched to
>> >> 9.6.  I don't see any reason why we shouldn't backpatch this commit.
>> >> So, I have attached a patch (fix_parallel_hash_path_v1.patch) which we
>> >> can backpatch in 9.6.
>> >>
>> >> Robert, your input will be highly appreciated here especially for the
>> >> back patch (to 9.6) I am proposing?
>> >>
>> >
>> > I have rebased the HEAD patch and done some cosmetic changes like
>> > improved the test by giving aliases to table names and modified the
>> > comment a bit, otherwise, the core logic remains the same.  As the
>> > back-branch patches are just the matter of rebasing them, I will do
>> > that before commit.
>> >
>> > I am still waiting for input, but if there is none, my plan is to
>> > commit this in a day or two and back-patch it as well.  Along with
>> > this, I would also like to back-patch commit
>> > 655393a022bd653e2b48dbf20b69236981e35195 for the reasons mentioned
>> > above.
>>
>> I have reviewed and tested the patch.  The patch looks fine to me and
>> behaviour is as expected.
>>
>
> Do you agree with my proposal to backpatch commit - 655393a022 to 9.6?
>

Although it was not giving any wrong output. However, this was a bug,
due to which, it may not select the best parallel plan or completely
miss some of the parallel paths so I will vote for backpatching it.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 13, 2018 at 8:42 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Sep 13, 2018 at 6:18 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >> >>
> >> >
> >> > I have rebased the HEAD patch and done some cosmetic changes like
> >> > improved the test by giving aliases to table names and modified the
> >> > comment a bit, otherwise, the core logic remains the same.  As the
> >> > back-branch patches are just the matter of rebasing them, I will do
> >> > that before commit.
> >> >
> >> > I am still waiting for input, but if there is none, my plan is to
> >> > commit this in a day or two and back-patch it as well.  Along with
> >> > this, I would also like to back-patch commit
> >> > 655393a022bd653e2b48dbf20b69236981e35195 for the reasons mentioned
> >> > above.
> >>
> >> I have reviewed and tested the patch.  The patch looks fine to me and
> >> behaviour is as expected.
> >>

Thanks, pushed.

> >
> > Do you agree with my proposal to backpatch commit - 655393a022 to 9.6?
> >
>
> Although it was not giving any wrong output. However, this was a bug,
> due to which, it may not select the best parallel plan or completely
> miss some of the parallel paths so I will vote for backpatching it.
>

Okay, pushed the back-patch patch.

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