Re: [sqlsmith] Failed assertion in create_gather_path

On Tue, Apr 10, 2018 at 11:14 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)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