Re: parallelize queries containing initplans

On Fri, Oct 6, 2017 at 2:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Oct 5, 2017 at 1:16 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> On Thu, Oct 5, 2017 at 6:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Oct 5, 2017 at 5:52 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>>> Now, unless, I am missing something here, it won't be possible to
>>>> detect params in such cases during forming of join rels and hence we
>>>> need the tests in generate_gather_paths. Let me know if I am missing
>>>> something in this context or if you have any better ideas to make it
>>>> work?
>>>
>>> Hmm, in a case like this, I think we shouldn't need to detect it. The
>>> Var is perfectly parallel-safe, the problem is that we can't use a
>>> not-safe plan for the inner rel. I wonder why that's happening
>>> here...
>>
>> It is because the inner rel (Result Path) contains a reference to a
>> param which appears to be at the same query level. Basically due to
>> changes in max_parallel_hazard_walker().
>
> I spent several hours debugging this issue this afternoon.
>

Thanks a lot for spending time.

> I think
> you've misdiagnosed the problem. I think that the Param reference in
> the result path is parallel-safe; that doesn't seem to me to be wrong.
> If we see a Param reference for our own query level, then either we're
> below the Gather and the new logic added by this patch will pass down
> the value or we're above the Gather and we can access the value
> directly. Either way, no problem.
>
> However, I think that if you attach an InitPlan to a parallel-safe
> plan, it becomes parallel-restricted. This is obvious in the case
> where the InitPlan's plan isn't itself parallel-safe, but it's also
> arguably true even when the InitPlan's plan *is* parallel-safe,
> because pushing that below a Gather introduces a multiple-evaluation
> hazard. I think we should fix that problem someday by providing a
> DSA-based parameter store, but that's a job for another day.
>
> And it turns out that we actually have such logic already, but this
> patch removes it:
>
> @@ -2182,7 +2181,6 @@ SS_charge_for_initplans(PlannerInfo *root,
> RelOptInfo *final_rel)
>
> path->startup_cost += initplan_cost;
> path->total_cost += initplan_cost;
> - path->parallel_safe = false;
> }
>
> /* We needn't do set_cheapest() here, caller will do it */
>

Yeah, it is a mistake from my side. I thought that we don't need it
now as we pass the computed value of initplan params for valid cases
and for other cases we can prohibit them as was done in the patch.

> Now, the header comment for SS_charge_for_initplans() is wrong; it
> claims we can't transmit initPlans to workers, but I think that's
> already wrong even before this patch.
>

What exactly you want to convey as part of that comment?

I have fixed the other review comment related to using safe_param_list
in the attached patch. I think I have fixed all comments given by
you, but let me know if I have missed anything or you have any other
comment.

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