Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Mar 7, 2017 at 10:28 PM, Robert Haas wrote: >> Apart from this, there was one problem in match_unsorted_outer (in >> v10), Basically, if inner_cheapest_total was not parallel_safe then I >> was always getting parallel safe inner. But, we should not do anything >> if

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Mar 7, 2017 at 11:38 AM, Dilip Kumar wrote: > On Tue, Mar 7, 2017 at 7:47 PM, Robert Haas wrote: >> You're right to be confused, because that seems to be a bug in the >> existing code. There seems to be no guarantee that the cheapest >>

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Mar 7, 2017 at 7:47 PM, Robert Haas wrote: > You're right to be confused, because that seems to be a bug in the > existing code. There seems to be no guarantee that the cheapest > parallel-safe path will be in the cheapest_parameterized_paths list. > I'll go fix

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Mar 7, 2017 at 5:16 AM, Dilip Kumar wrote: > I am confused about whether to call > "get_cheapest_parallel_safe_total_inner" with > innerrel->cheapest_parameterized_paths like we do in case of > hash_inner_and_outer or with > innerrel->pathlist. The reason behind I

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Mar 7, 2017 at 5:21 AM, Robert Haas wrote: > +/* Can't do anything else if inner path needs to be unique'd */ > +if (save_jointype == JOIN_UNIQUE_INNER) > +return; > > Right after this, you should try_partial_mergejoin_path() with the > result of

Re: [HACKERS] Proposal : Parallel Merge Join

On Mon, Mar 6, 2017 at 8:15 AM, Dilip Kumar wrote: > On Fri, Mar 3, 2017 at 3:57 PM, Robert Haas wrote: >> I'm not happy with the way this patch can just happen to latch on to a >> path that's not parallel-safe rather than one that is and then just

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Mar 3, 2017 at 3:57 PM, Robert Haas wrote: > I'm not happy with the way this patch can just happen to latch on to a > path that's not parallel-safe rather than one that is and then just > give up on a merge join in that case. I already made this argument in >

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Mar 3, 2017 at 9:47 PM, Dilip Kumar wrote: > On Fri, Mar 3, 2017 at 3:57 PM, Robert Haas wrote: >> I'm not happy with the way this patch can just happen to latch on to a >> path that's not parallel-safe rather than one that is and then just

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Mar 3, 2017 at 3:57 PM, Robert Haas wrote: > I'm not happy with the way this patch can just happen to latch on to a > path that's not parallel-safe rather than one that is and then just > give up on a merge join in that case. I already made this argument in >

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Mar 1, 2017 at 12:01 PM, Amit Kapila wrote: > On Wed, Mar 1, 2017 at 11:24 AM, Dilip Kumar wrote: >> On Wed, Mar 1, 2017 at 11:13 AM, Amit Kapila wrote: >>> I think for now we can keep the parallel safety check for

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Mar 1, 2017 at 11:24 AM, Dilip Kumar wrote: > On Wed, Mar 1, 2017 at 11:13 AM, Amit Kapila wrote: >> I think for now we can keep the parallel safety check for cheapest >> inner path, though it will be of use only for the very first time we

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Mar 1, 2017 at 11:13 AM, Amit Kapila wrote: > I think for now we can keep the parallel safety check for cheapest > inner path, though it will be of use only for the very first time we > compare the paths in that loop. I am not sure if there is any other > better

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Feb 28, 2017 at 9:28 PM, Dilip Kumar wrote: > On Mon, Feb 27, 2017 at 10:27 AM, Amit Kapila wrote: >> Okay, but in that case don't you think it is better to consider the >> parallel safety of cheapest_total_inner only when we don't find any

Re: [HACKERS] Proposal : Parallel Merge Join

On Mon, Feb 27, 2017 at 10:27 AM, Amit Kapila wrote: > Okay, but in that case don't you think it is better to consider the > parallel safety of cheapest_total_inner only when we don't find any > cheap parallel_safe innerpath by reducing the sort keys? Well, we can do

Re: [HACKERS] Proposal : Parallel Merge Join

On Sun, Feb 26, 2017 at 12:01 PM, Robert Haas wrote: > On Fri, Feb 24, 2017 at 3:54 PM, Amit Kapila wrote: >> I agree in some cases it could be better, but I think benefits are not >> completely clear, so probably we can leave it as of now and if

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Feb 24, 2017 at 3:54 PM, Amit Kapila wrote: > I agree in some cases it could be better, but I think benefits are not > completely clear, so probably we can leave it as of now and if later > any one comes with a clear use case or can see the benefits of such > path

Re: [HACKERS] Proposal : Parallel Merge Join

On Sat, Feb 25, 2017 at 3:22 PM, Dilip Kumar wrote: > On Sat, Feb 25, 2017 at 11:29 AM, Amit Kapila wrote: >> It seems you have forgotten to change in the below check: >> >> + if (innerpath != NULL && >> + innerpath->parallel_safe && >> +

Re: [HACKERS] Proposal : Parallel Merge Join

On Sat, Feb 25, 2017 at 11:29 AM, Amit Kapila wrote: > It seems you have forgotten to change in the below check: > > + if (innerpath != NULL && > + innerpath->parallel_safe && > + (cheapest_startup_inner == NULL || > + cheapest_startup_inner->parallel_safe == false || > +

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Feb 24, 2017 at 4:49 PM, Dilip Kumar wrote: > On Fri, Feb 24, 2017 at 3:04 PM, Amit Kapila wrote: >> What advantage do you see for considering such a path when the cost of >> innerpath is more than cheapest_total_inner? Remember the more

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Feb 24, 2017 at 3:04 PM, Amit Kapila wrote: > What advantage do you see for considering such a path when the cost of > innerpath is more than cheapest_total_inner? Remember the more paths > we try to consider, the more time we spend in the planner. By any >

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Feb 24, 2017 at 3:42 PM, Dilip Kumar wrote: > On Fri, Feb 24, 2017 at 3:04 PM, Amit Kapila wrote: > > Thanks for the comments. > >> What advantage do you see for considering such a path when the cost of >> innerpath is more than

Re: [HACKERS] Proposal : Parallel Merge Join

On Fri, Feb 24, 2017 at 3:04 PM, Amit Kapila wrote: Thanks for the comments. > What advantage do you see for considering such a path when the cost of > innerpath is more than cheapest_total_inner? Remember the more paths > we try to consider, the more time we spend in

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Feb 14, 2017 at 5:22 PM, Dilip Kumar wrote: > On Tue, Feb 14, 2017 at 12:25 PM, Amit Kapila wrote: > Apart from this, there was one small problem in an earlier version > which I have corrected in this. > > + /* Consider only parallel safe

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Feb 14, 2017 at 12:25 PM, Amit Kapila 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

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Jan 4, 2017 at 9:27 PM, Dilip Kumar wrote: > On Wed, Jan 4, 2017 at 12:02 PM, Amit Kapila wrote: >> 3. >> + /* >> + * See comments in try_partial_nestloop_path(). >> + */ >> >> This same code exists in try_partial_nestloop_path() and >>

Re: [HACKERS] Proposal : Parallel Merge Join

On Thu, Jan 5, 2017 at 12:57 AM, Dilip Kumar wrote: > On Wed, Jan 4, 2017 at 12:02 PM, Amit Kapila wrote: >> Review comments: >> 1. >> + bool is_partial); >> + >> >> Seems additional new line is not required. > Fixed This patch has a patch, no new

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Jan 4, 2017 at 12:02 PM, Amit Kapila wrote: > Review comments: > 1. > + bool is_partial); > + > > Seems additional new line is not required. Fixed > > 2. > + * try_partial_mergejoin_path > + * Consider a partial merge join path; if it appears useful, > push it

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Dec 21, 2016 at 9:23 PM, Dilip Kumar wrote: > On Wed, Dec 21, 2016 at 8:39 PM, Robert Haas wrote: >> Committed the refactoring patch with some mild cosmetic adjustments. > > Thanks.. >> >> As to the second patch: >> >> +if (jointype

Re: [HACKERS] Proposal : Parallel Merge Join

On Thu, Dec 29, 2016 at 3:15 AM, Tomas Vondra wrote: > FWIW, I've done quite a bit of testing on this patch, and also on the other > patches adding parallel index scans and bitmap heap scan. I've been running > TPC-H and TPC-DS on 16GB data sets with each patch,

Re: [HACKERS] Proposal : Parallel Merge Join

Hi, On 12/21/2016 04:53 PM, Dilip Kumar wrote: On Wed, Dec 21, 2016 at 8:39 PM, Robert Haas wrote: Committed the refactoring patch with some mild cosmetic adjustments. Thanks.. As to the second patch: +if (jointype == JOIN_UNIQUE_INNER) +

Re: [HACKERS] Proposal : Parallel Merge Join

On Wed, Dec 21, 2016 at 8:39 PM, Robert Haas wrote: > Committed the refactoring patch with some mild cosmetic adjustments. Thanks.. > > As to the second patch: > > +if (jointype == JOIN_UNIQUE_INNER) > +jointype = JOIN_INNER; > > Isn't this dead code?

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Dec 13, 2016 at 10:04 AM, Dilip Kumar wrote: > On Tue, Dec 13, 2016 at 6:42 AM, Dilip Kumar wrote: >>> This patch is hard to read because it is reorganizing a bunch of code >>> as well as adding new functionality. Perhaps you could separate

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Dec 13, 2016 at 8:34 PM, Dilip Kumar wrote: > I have created two patches as per the suggestion. > > 1. mergejoin_refactoring_v2.patch --> Move functionality of > considering various merge join path into new function. > 2. parallel_mergejoin_v2.patch -> This adds the

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Dec 13, 2016 at 6:42 AM, Dilip Kumar wrote: >> This patch is hard to read because it is reorganizing a bunch of code >> as well as adding new functionality. Perhaps you could separate it >> into two patches, one with the refactoring and then the other with the >>

Re: [HACKERS] Proposal : Parallel Merge Join

On Tue, Dec 13, 2016 at 12:11 AM, Robert Haas wrote: > > Hmm, so it seems my initial guess that we didn't need to bother > generating such paths was wrong. Oops. > > This patch is hard to read because it is reorganizing a bunch of code > as well as adding new

Re: [HACKERS] Proposal : Parallel Merge Join

On Sat, Dec 10, 2016 at 7:59 AM, Dilip Kumar wrote: > I would like to propose a patch for parallelizing merge join path. > This idea is derived by analyzing TPCH results. > > I have done this analysis along with my colleagues Rafia sabih and Amit > kaplia. > > Currently we

Re: [HACKERS] Proposal : Parallel Merge Join

On Sun, Dec 11, 2016 at 8:14 AM, Peter Geoghegan wrote: > I noticed that the partially parallelized Merge Join EXPLAIN ANALYZE > that you attach has a big misestimation: > > -> Merge Join (cost=3405052.45..3676948.66 rows=320 width=32) > (actual time=21165.849..37814.551

Re: [HACKERS] Proposal : Parallel Merge Join

On Sat, Dec 10, 2016 at 4:59 AM, Dilip Kumar wrote: > 3. 20_patch.out : Explain analyze output with patch (median of 3 runs) I noticed that the partially parallelized Merge Join EXPLAIN ANALYZE that you attach has a big misestimation: -> Merge Join

[HACKERS] Proposal : Parallel Merge Join

I would like to propose a patch for parallelizing merge join path. This idea is derived by analyzing TPCH results. I have done this analysis along with my colleagues Rafia sabih and Amit kaplia. Currently we already have infrastructure for executing parallel join, so we don't need any change at