Re: [sqlsmith] Failed assertion in create_gather_path

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? 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