Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Feb 14, 2017 at 12:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few review comments on the latest version of the patch:
> 1.
> - if (joinrel->consider_parallel && nestjoinOK &&
> - save_jointype != JOIN_UNIQUE_OUTER &&
> - bms_is_empty(joinrel->lateral_relids))
> + if (outerrel->partial_pathlist == NIL ||
> + !joinrel->consider_parallel ||
> + save_jointype == JOIN_UNIQUE_OUTER ||
> + !bms_is_empty(joinrel->lateral_relids) ||
> + jointype == JOIN_FULL ||
> + jointype == JOIN_RIGHT)
> + return;
>
>
> For the matter of consistency, I think the above checks should be in
> same order as they are in function hash_inner_and_outer().  I wonder
> why are you using jointype in above check instead of save_jointype, it
> seems to me we can use save_jointype for all the cases.

Done

>
> 2.
> + generate_mergejoin_paths(root, joinrel, innerrel, outerpath,
> +
> jointype, extra, false,
> +
> inner_cheapest_total, merge_pathkeys,
> +
> true);
>
> IIUC, the reason of passing a 7th parameter (useallclauses) as false
> is that it can be true only for Right and Full joins and for both we
> don't generate partial merge join paths.  If so, then I think it is
> better to add a comment about such an assumption, so that we don't
> forget to update this code in future if we need to useallclauses for
> something else.  Another way to deal with this is that you can pass
> the value of useallclauses to consider_parallel_mergejoin() and then
> to generate_mergejoin_paths().  I feel second way is better, but if
> for some reason you don't find it appropriate then at least add a
> comment.

After fixing 3rd comments this will not be needed.
>
> 3.
> generate_mergejoin_paths()
> {

>
> The above and similar changes in generate_mergejoin_paths() doesn't
> look good and in some cases when innerpath is *parallel-unsafe*, you
> need to perform some extra computation which is not required.  How
> about writing a separate function generate_partial_mergejoin_paths()?
> I think you can save some extra computation and it will look neat.  I
> understand that there will be some code duplication, but not sure if
> there is any other better way.

Okay, I have done as suggested.


Apart from this, there was one small problem in an earlier version
which I have corrected in this.

+ /* Consider only parallel safe inner path */
+ if (innerpath != NULL &&
+ innerpath->parallel_safe &&
+ (cheapest_total_inner == NULL ||
+ cheapest_total_inner->parallel_safe == false ||
+ compare_path_costs(innerpath, cheapest_total_inner,
+ TOTAL_COST) < 0))

In this comparison, we were only checking if innerpath is cheaper than
the cheapest_total_inner then generate path with this new inner path
as well. Now, I have added one more check if cheapest_total_inner was
not parallel safe then also consider a path with this new inner
(provided this inner is parallel safe).


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers