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