Re: Parallel Seq Scan

On Sat, Nov 7, 2015 at 4:11 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Fri, Oct 23, 2015 at 9:22 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > The base rel's consider_parallel flag won't be percolated to childrels,
so
> > even
> > if we mark base rel as parallel capable, while generating the path it
won't
> > be considered. I think we need to find a way to pass on that
information if
> > we want to follow this way.
>
> Fixed in the attached version. I added a max_parallel_degree check,
> too, per your suggestion.
>

+ else if (IsA(node, SubPlan) || IsA(node, SubLink) ||
+ IsA(node, AlternativeSubPlan) || IsA(node, Param))
+ {
+ /*
+ * Since we don't have the ability to push subplans down to workers
+ * at present, we treat subplan references as parallel-restricted.
+ */
+ if (!context->allow_restricted)
+ return true;
+ }

You seem to have agreed upthread to change this check for PARAM_EXEC
paramkind. I think you have forgotten to change this code.

@@ -714,6 +860,9 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
continue;
}

+ /* Copy consider_parallel flag from parent. */
+ childrel->consider_parallel = rel->consider_parallel;
+

We are changing the childrel quals in this function and in function
adjust_appendrel_attrs()->adjust_appendrel_attrs_mutator(), we are
adding an extra conversion step for one of the case to the Var
participating in qualification. So even though it might be safe to
assume that it won't add any new parallel-restricted or parallel-unsafe
expression in qual, ideally we should have a check for parallel-safety in
childrel quals separately as those might not be identical to parent rel.

> > True, we can do that way. What I was trying to convey by above is
> > that we anyway need checks during path creation atleast in some
> > of the cases, so why not do all the checks at that time only as I
> > think all the information will be available at that time.
> >
> > I think if we store parallelism related info in RelOptInfo, that can
also
> > be made to work, but the only worry I have with that approach is we
> > need to have checks at two levels one at RelOptInfo formation time
> > and other at Path formation time.
>
> I don't really see that as a problem. What I'm thinking about doing
> (but it's not implemented in the attached patch) is additionally
> adding a ppi_consider_parallel flag to the ParamPathInfo. This would
> be meaningful only for baserels, and would indicate whether the
> ParamPathInfo's ppi_clauses are parallel-safe.
>
> If we're thinking about adding a parallel path to a baserel, we need
> the RelOptInfo to have consider_parallel set and, if there is a
> ParamPathInfo, we need the ParamPathInfo's ppi_consider_parallel flag
> to be set also. That shows that both the rel's baserestrictinfo and
> the paramaterized join clauses are parallel-safe. For a joinrel, we
> can add a path if (1) the joinrel has consider_parallel set and (2)
> the paths being joined are parallel-safe. Testing condition (2) will
> require a per-Path flag, so we'll end up with one flag in the
> RelOptInfo, a second in the ParamPathInfo, and a third in the Path.
>

I am already adding a parallel_aware flag in the patch to make seqscan
node parallel_aware, so if you find that patch better compare to partial
seq scan node patch, then I can take care of above in that patch.

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