Re: [sqlsmith] Failed assertion in create_gather_path

On Mon, Apr 9, 2018 at 5:52 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Sun, Apr 8, 2018 at 1:04 PM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> > Hi,
> >
> > At some places, I have observed that we are adding a partial path even
> when
> > rel's consider_parallel is false. Due to this, the partial path added has
> > parallel_safe set to false and then later in create_gather_path()
> assertion
> > fails.
> >
>
> Few Comments:
> 1.
> @@ -2196,6 +2196,10 @@ set_subquery_pathlist(PlannerInfo *root,
> RelOptInfo *rel,
> pathkeys, required_outer));
> }
>
> + /* If parallelism is not possible, return. */
> + if (!rel->consider_parallel || !bms_is_empty(required_outer))
> + return;
>
> In this case shouldn't we set the rel's consider_parallel flag
> correctly rather than avoiding adding the path to it as we do in
> recurse_set_operations?
>

In recurse_set_operations() we are building a new rel and setting its
properties from the final_rel. consider_parallel there is just copied from
the final_rel.
However, in set_subquery_pathlist(), rel is the input parameter here and we
are trying to add a partial path to it without looking at its
consider_parallel field. This patch does that.

And if we want to set consider_parallel for the rel, then it should have
been done prior to this function itself. And I am not sure where that
exactly but not in this function I guess.

> 2.
> @@ -331,7 +331,7 @@ recurse_set_operations(Node *setOp, PlannerInfo *root,
> * to build a partial path for this relation. But there's no point in
> * considering any path but the cheapest.
> */
> - if (final_rel->partial_pathlist != NIL)
> + if (final_rel->consider_parallel && final_rel->partial_pathlist != NIL)
>
> What problem did you see here or is it just for additional safety?
> Ideally, if the consider_parallel is false for a rel, it's partial
> path list should also be NULL.
>

I actually wanted to have rel->consider_parallel in the condition (yes, for
additional safety) as we are adding a partial path into rel. But then
observed that it is same as that of final_rel->consider_parallel and thus
used it along with other condition.

I have observed at many places that we do check consider_parallel flag
before adding a partial path to it. Thus for consistency added here too,
but yes, it just adds an additional safety here.

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

--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company