Re: [sqlsmith] Failed assertion in create_gather_path

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?

  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.

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);

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,

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