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

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