Re: [sqlsmith] Failed assertion in create_gather_path

On Tue, Apr 10, 2018 at 11:14 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
> > 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.
>
> Thanks to Andreas for reporting this issue and to Jeevan for fixing
> it.  My commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 seems clearly
> to blame.
>
> The change to set_subquery_pathlist() looks correct, but can we add a
> simple test case?


I have tried adding simple testcase in this version of the patch. This test
hits the Assertion added in add_partial_path() like you have tried.


>   Right now if I keep the new Assert() in
> add_partial_path() and leave out the rest of the changes, the
> regression tests still pass.  That's not so good.  Also, I think I
> would be inclined to wrap the if-statement around the rest of the
> function instead of returning early.
>

OK.
Wrapped if block instead of returning mid-way.


>
> The new Assert() in add_partial_path() is an excellent idea.  I had
> the same thought before, but I didn't do anything about it.  That was
> a bad idea; your plan is better. In fact, I suggest an additional
> Assert() that any relation to which we're adding a partial path is
> marked consider_parallel, like this:
>
> +    /* Path to be added must be parallel safe. */
> +    Assert(new_path->parallel_safe);
> +
> +    /* Relation should be OK for parallelism, too. */
> +    Assert(parent_rel->consider_parallel);
>

Yep.
Added this new one too.


>
> Regarding recurse_set_operations, since the rel->consider_parallel is
> always the same as final_rel->consider_parallel there's really no
> value in testing it.  If it were possible for rel->consider_parallel
> to be false even when final_rel->consider_parallel were true then the
> test would be necessary for correctness.  That might or might not
> happen in the future, so I guess it wouldn't hurt to include this for
> future-proofing,


In that case, we should have rel in a condition rather than final_rel as we
are adding a path into rel. For future-proofing added check on
lateral_relids too.


> but it's not actually a bug fix, so it also wouldn't
> hurt to leave it out.  I could go either way, I guess.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>



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