Re: [HACKERS] Gather Merge

On Tue, Mar 14, 2017 at 9:56 AM, Tom Lane wrote: > Robert Haas writes: >> Cool, thanks for the review. I'm not quite confident that we've found >> all of the bugs here yet, but I think we're moving in the right >> direction. > > I guess the real question here is why isn't Gather Merge more like

Re: [HACKERS] Gather Merge

Robert Haas writes: > Cool, thanks for the review. I'm not quite confident that we've found > all of the bugs here yet, but I think we're moving in the right > direction. I guess the real question here is why isn't Gather Merge more like Append and MergeAppend? That is, why did you complicate m

Re: [HACKERS] Gather Merge

On Tue, Mar 14, 2017 at 5:47 AM, Rushabh Lathia wrote: > Thanks Robert for the patch and the explanation. > > I studied the patch and that look right to me. I performed manual testing, > run the scripts which I created during the gather merge patch also run > the tpch queries to make sure that it

Re: [HACKERS] Gather Merge

On Mon, Mar 13, 2017 at 10:56 PM, Robert Haas wrote: > On Fri, Mar 10, 2017 at 7:59 AM, Rushabh Lathia > wrote: > > Error coming from create_gather_merge_plan() from below condition: > > > > if (memcmp(sortColIdx, gm_plan->sortColIdx, > >numsortkeys * sizeof(AttrNumber)) != 0

Re: [HACKERS] Gather Merge

On Fri, Mar 10, 2017 at 7:59 AM, Rushabh Lathia wrote: > Error coming from create_gather_merge_plan() from below condition: > > if (memcmp(sortColIdx, gm_plan->sortColIdx, >numsortkeys * sizeof(AttrNumber)) != 0) > elog(ERROR, "GatherMerge child's targetlist doesn't mat

Re: [HACKERS] Gather Merge

Error coming from create_gather_merge_plan() from below condition: if (memcmp(sortColIdx, gm_plan->sortColIdx, numsortkeys * sizeof(AttrNumber)) != 0) elog(ERROR, "GatherMerge child's targetlist doesn't match GatherMerge"); Above condition checks the sort column numbers

Re: [HACKERS] Gather Merge

On Fri, Mar 10, 2017 at 4:09 PM, Kuntal Ghosh wrote: > On Fri, Mar 10, 2017 at 3:04 PM, Rushabh Lathia > wrote: > > > > > > On Fri, Mar 10, 2017 at 2:42 PM, Andreas Joseph Krogh < > andr...@visena.com> > > wrote: > >> > >> På fredag 10. mars 2017 kl. 10:09:22, skrev Rushabh Lathia > >> : > >> >

Re: [HACKERS] Gather Merge

På fredag 10. mars 2017 kl. 10:34:48, skrev Rushabh Lathia < rushabh.lat...@gmail.com >:     On Fri, Mar 10, 2017 at 2:42 PM, Andreas Joseph Krogh mailto:andr...@visena.com>> wrote: På fredag 10. mars 2017 kl. 10:09:22, skrev Rushabh Lathia mailto:rushabh.lat...@gm

Re: [HACKERS] Gather Merge

On Fri, Mar 10, 2017 at 2:42 PM, Andreas Joseph Krogh wrote: > På fredag 10. mars 2017 kl. 10:09:22, skrev Rushabh Lathia < > rushabh.lat...@gmail.com>: > > > > On Fri, Mar 10, 2017 at 2:33 PM, Andreas Joseph Krogh > wrote: >> >> [...] >> The execution-plan seems (unsurprisingly) to depend on da

Re: [HACKERS] Gather Merge

På fredag 10. mars 2017 kl. 10:09:22, skrev Rushabh Lathia < rushabh.lat...@gmail.com >:     On Fri, Mar 10, 2017 at 2:33 PM, Andreas Joseph Krogh mailto:andr...@visena.com>> wrote: [...] The execution-plan seems (unsurprisingly) to depend on data-distribution, so

Re: [HACKERS] Gather Merge

On Fri, Mar 10, 2017 at 2:33 PM, Andreas Joseph Krogh wrote: > På fredag 10. mars 2017 kl. 09:53:47, skrev Rushabh Lathia < > rushabh.lat...@gmail.com>: > > > > On Fri, Mar 10, 2017 at 1:44 PM, Andreas Joseph Krogh > wrote: >> >> På torsdag 09. mars 2017 kl. 18:09:45, skrev Robert Haas < >> robe

Re: [HACKERS] Gather Merge

På fredag 10. mars 2017 kl. 09:53:47, skrev Rushabh Lathia < rushabh.lat...@gmail.com >:     On Fri, Mar 10, 2017 at 1:44 PM, Andreas Joseph Krogh mailto:andr...@visena.com>> wrote: På torsdag 09. mars 2017 kl. 18:09:45, skrev Robert Haas mailto:robertmh...@gmail.c

Re: [HACKERS] Gather Merge

On Fri, Mar 10, 2017 at 1:44 PM, Andreas Joseph Krogh wrote: > På torsdag 09. mars 2017 kl. 18:09:45, skrev Robert Haas < > robertmh...@gmail.com>: > > On Thu, Mar 9, 2017 at 11:25 AM, Rushabh Lathia > wrote: > > I don't see this failure with the patch. Even I forced the gather merge > > in the

Re: [HACKERS] Gather Merge

På torsdag 09. mars 2017 kl. 18:09:45, skrev Robert Haas mailto:robertmh...@gmail.com>>: On Thu, Mar 9, 2017 at 11:25 AM, Rushabh Lathia wrote: > I don't see this failure with the patch. Even I forced the gather merge > in the above query and that just working fine. > > Attaching patch, with

Re: [HACKERS] Gather Merge

On Thu, Mar 9, 2017 at 11:25 AM, Rushabh Lathia wrote: > I don't see this failure with the patch. Even I forced the gather merge > in the above query and that just working fine. > > Attaching patch, with the discussed changes. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Gather Merge

On Thu, Mar 9, 2017 at 9:42 PM, Robert Haas wrote: > On Thu, Mar 9, 2017 at 8:21 AM, Rushabh Lathia > wrote: > > Thanks Robert for committing this. > > > > My colleague Neha Sharma found one regression with the patch. I was about > > to send this mail and noticed that you committed the patch. >

Re: [HACKERS] Gather Merge

On Thu, Mar 9, 2017 at 8:21 AM, Rushabh Lathia wrote: > Thanks Robert for committing this. > > My colleague Neha Sharma found one regression with the patch. I was about > to send this mail and noticed that you committed the patch. Oops. Bad timing. > postgres=# explain select aid from pgbench_a

Re: [HACKERS] Gather Merge

On Thu, Mar 9, 2017 at 6:19 PM, Robert Haas wrote: > On Wed, Mar 8, 2017 at 11:59 PM, Rushabh Lathia > wrote: > > Here is another version of patch with the suggested changes. > > Committed. > > Thanks Robert for committing this. My colleague Neha Sharma found one regression with the patch. I wa

Re: [HACKERS] Gather Merge

On Wed, Mar 8, 2017 at 11:59 PM, Rushabh Lathia wrote: > Here is another version of patch with the suggested changes. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To

Re: [HACKERS] Gather Merge

On Thu, Mar 9, 2017 at 8:40 AM, Robert Haas wrote: > On Mon, Feb 20, 2017 at 1:35 AM, Rushabh Lathia > wrote: > > Thanks Amit for raising this point. I was not at all aware of > mark/restore. > > I tried to come up with the case, but haven't found such case. > > > > For now here is the patch wit

Re: [HACKERS] Gather Merge

On Mon, Feb 20, 2017 at 1:35 AM, Rushabh Lathia wrote: > Thanks Amit for raising this point. I was not at all aware of mark/restore. > I tried to come up with the case, but haven't found such case. > > For now here is the patch with comment update. Sorry for the delay in getting back to this. Th

Re: [HACKERS] Gather Merge

On Mon, Feb 20, 2017 at 1:58 PM, Dilip Kumar wrote: > On Mon, Feb 20, 2017 at 12:05 PM, Rushabh Lathia > wrote: >> Thanks Amit for raising this point. I was not at all aware of mark/restore. >> I tried to come up with the case, but haven't found such case. >> >> For now here is the patch with com

Re: [HACKERS] Gather Merge

On Mon, Feb 20, 2017 at 12:05 PM, Rushabh Lathia wrote: > Thanks Amit for raising this point. I was not at all aware of mark/restore. > I tried to come up with the case, but haven't found such case. > > For now here is the patch with comment update. I think for reproducing this you need plan some

Re: [HACKERS] Gather Merge

Thanks Amit for raising this point. I was not at all aware of mark/restore. I tried to come up with the case, but haven't found such case. For now here is the patch with comment update. Thanks, On Sun, Feb 19, 2017 at 7:30 PM, Amit Kapila wrote: > On Sun, Feb 19, 2017 at 2:22 PM, Robert Haas

Re: [HACKERS] Gather Merge

On Sun, Feb 19, 2017 at 2:22 PM, Robert Haas wrote: > On Sat, Feb 18, 2017 at 6:43 PM, Amit Kapila wrote: >> I think there is a value in supporting mark/restore position for any >> node which produces sorted results, however, if you don't want to >> support it, then I think we should update above

Re: [HACKERS] Gather Merge

On Sat, Feb 18, 2017 at 6:43 PM, Amit Kapila wrote: > I think there is a value in supporting mark/restore position for any > node which produces sorted results, however, if you don't want to > support it, then I think we should update above comment in the code to > note this fact. Also, you might

Re: [HACKERS] Gather Merge

On Fri, Feb 17, 2017 at 6:27 PM, Rushabh Lathia wrote: > On Fri, Feb 17, 2017 at 4:47 PM, Amit Kapila > wrote: >> >> Are you suggesting that commit 0c2070ce has helped to improve >> performance if so, I don't think that has been proved? I guess the >> numbers are different either due to differen

Re: [HACKERS] Gather Merge

On Fri, Feb 17, 2017 at 4:47 PM, Amit Kapila wrote: > On Fri, Feb 17, 2017 at 3:59 PM, Thomas Munro > wrote: > > On Thu, Feb 2, 2017 at 2:32 AM, Rushabh Lathia > wrote: > >> Please find attached latest patch. > > > > The latest patch still applies (with some fuzz), builds and the > > regression

Re: [HACKERS] Gather Merge

On Fri, Feb 17, 2017 at 3:59 PM, Thomas Munro wrote: > On Thu, Feb 2, 2017 at 2:32 AM, Rushabh Lathia > wrote: > > Please find attached latest patch. > > The latest patch still applies (with some fuzz), builds and the > regression tests pass. > > Attached latest patch, which applies cleanly on l

Re: [HACKERS] Gather Merge

On Fri, Feb 17, 2017 at 3:59 PM, Thomas Munro wrote: > On Thu, Feb 2, 2017 at 2:32 AM, Rushabh Lathia > wrote: >> Please find attached latest patch. > > The latest patch still applies (with some fuzz), builds and the > regression tests pass. > > I see that Robert made a number of changes and pos

Re: [HACKERS] Gather Merge

On Thu, Feb 2, 2017 at 2:32 AM, Rushabh Lathia wrote: > Please find attached latest patch. The latest patch still applies (with some fuzz), builds and the regression tests pass. I see that Robert made a number of changes and posted a v6 along with some numbers which he described as lacklustre, b

Re: [HACKERS] Gather Merge

Hi, I have done some testing with the latest patch 1)./pgbench postgres -i -F 100 -s 20 2) update pgbench_accounts set filler = 'foo' where aid%10 = 0; 3) vacuum analyze pgbench_accounts; 4) set max_parallel_workers_per_gather = 4; 5) set max_parallel_workers = 4; *Machine Configuration :-* RAM

Re: [HACKERS] Gather Merge

Due to recent below commit, patch not getting apply cleanly on master branch. commit d002f16c6ec38f76d1ee97367ba6af3000d441d0 Author: Tom Lane Date: Mon Jan 30 17:15:42 2017 -0500 Add a regression test script dedicated to exercising system views. Please find attached latest patch. On W

Re: [HACKERS] Gather Merge

I am sorry for the delay, here is the latest re-based patch. my colleague Neha Sharma, reported one regression with the patch, where explain output for the Sort node under GatherMerge was always showing cost as zero: explain analyze select '' AS "xxx" from pgbench_accounts where filler like '%fo

Re: [HACKERS] Gather Merge

On Mon, Jan 23, 2017 at 6:51 PM, Kuntal Ghosh wrote: > On Wed, Jan 18, 2017 at 11:31 AM, Rushabh Lathia > wrote: >> > The patch needs a rebase after the commit 69f4b9c85f168ae006929eec4. Is an update going to be provided? I have moved this patch to next CF with "waiting on author" as status. --

Re: [HACKERS] Gather Merge

On Wed, Jan 18, 2017 at 11:31 AM, Rushabh Lathia wrote: > The patch needs a rebase after the commit 69f4b9c85f168ae006929eec4. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to y

Re: [HACKERS] Gather Merge

On Tue, Jan 17, 2017 at 6:44 PM, Amit Kapila wrote: > On Tue, Jan 17, 2017 at 5:19 PM, Rushabh Lathia > wrote: > > > > > > > > Here is the benchmark number which I got with the latest (v6) patch: > > > > - max_worker_processes = DEFAULT (8) > > - max_parallel_workers_per_gather = 4 > > - Cold ca

Re: [HACKERS] Gather Merge

On Tue, Jan 17, 2017 at 4:26 AM, Rushabh Lathia wrote: > Another observation is, HashAggregate (case 1) is performs better compare to > GroupAggregate (case 2), but still it doesn't justify the cost difference of > those two. It may not be the only issue, or even the main issue, but I'm fairly su

Re: [HACKERS] Gather Merge

On Tue, Jan 17, 2017 at 5:19 PM, Rushabh Lathia wrote: > > > > Here is the benchmark number which I got with the latest (v6) patch: > > - max_worker_processes = DEFAULT (8) > - max_parallel_workers_per_gather = 4 > - Cold cache environment is ensured. With every query execution - server is > sto

Re: [HACKERS] Gather Merge

On Tue, Jan 17, 2017 at 5:19 PM, Rushabh Lathia wrote: > > > On Fri, Jan 13, 2017 at 10:52 AM, Rushabh Lathia > wrote: > >> >> >> On Thu, Jan 12, 2017 at 8:50 AM, Robert Haas >> wrote: >> >>> On Sun, Dec 4, 2016 at 7:36 PM, Haribabu Kommi >>> wrote: >>> > On Thu, Nov 24, 2016 at 11:12 PM, Rush

Re: [HACKERS] Gather Merge

On Fri, Jan 13, 2017 at 10:52 AM, Rushabh Lathia wrote: > > > On Thu, Jan 12, 2017 at 8:50 AM, Robert Haas > wrote: > >> On Sun, Dec 4, 2016 at 7:36 PM, Haribabu Kommi >> wrote: >> > On Thu, Nov 24, 2016 at 11:12 PM, Rushabh Lathia < >> rushabh.lat...@gmail.com> >> > wrote: >> >> PFA latest pat

Re: [HACKERS] Gather Merge

On Thu, Jan 12, 2017 at 8:50 AM, Robert Haas wrote: > On Sun, Dec 4, 2016 at 7:36 PM, Haribabu Kommi > wrote: > > On Thu, Nov 24, 2016 at 11:12 PM, Rushabh Lathia < > rushabh.lat...@gmail.com> > > wrote: > >> PFA latest patch with fix as well as few cosmetic changes. > > > > Moved to next CF wit

Re: [HACKERS] Gather Merge

On Sun, Dec 4, 2016 at 7:36 PM, Haribabu Kommi wrote: > On Thu, Nov 24, 2016 at 11:12 PM, Rushabh Lathia > wrote: >> PFA latest patch with fix as well as few cosmetic changes. > > Moved to next CF with "needs review" status. I spent quite a bit of time on this patch over the last couple of days.

Re: [HACKERS] Gather Merge

On Thu, Nov 24, 2016 at 11:12 PM, Rushabh Lathia wrote: > > PFA latest patch with fix as well as few cosmetic changes. > > Moved to next CF with "needs review" status. Regards, Hari Babu Fujitsu Australia

Re: [HACKERS] Gather Merge

On Wed, Nov 16, 2016 at 3:10 PM, Rushabh Lathia wrote: > > > On Mon, Nov 14, 2016 at 3:51 PM, Thomas Munro < > thomas.mu...@enterprisedb.com> wrote: > >> On Sat, Nov 12, 2016 at 1:56 AM, Rushabh Lathia >> wrote: >> > On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro < >> thomas.mu...@enterprisedb.com

Re: [HACKERS] Gather Merge

On Mon, Nov 14, 2016 at 3:51 PM, Thomas Munro wrote: > On Sat, Nov 12, 2016 at 1:56 AM, Rushabh Lathia > wrote: > > On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro < > thomas.mu...@enterprisedb.com> > > wrote: > >> + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development > Group > >> + *

Re: [HACKERS] Gather Merge

On Sat, Nov 12, 2016 at 1:56 AM, Rushabh Lathia wrote: > On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro > wrote: >> + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group >> + * Portions Copyright (c) 1994, Regents of the University of California >> >> Shouldn't this say just "(c

Re: [HACKERS] Gather Merge

Oops forgot to attach latest patch in the earlier mail. On Fri, Nov 11, 2016 at 6:26 PM, Rushabh Lathia wrote: > > > On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro < > thomas.mu...@enterprisedb.com> wrote: > >> On Thu, Oct 27, 2016 at 10:50 PM, Rushabh Lathia >> wrote: >> > Please find attached

Re: [HACKERS] Gather Merge

On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro wrote: > On Thu, Oct 27, 2016 at 10:50 PM, Rushabh Lathia > wrote: > > Please find attached latest patch which fix the review point as well as > > additional clean-up. > > I've signed up to review this patch and I'm planning to do some > testing. Her

Re: [HACKERS] Gather Merge

On Fri, Nov 4, 2016 at 8:30 AM, Thomas Munro wrote: > On Thu, Oct 27, 2016 at 10:50 PM, Rushabh Lathia > wrote: >> Please find attached latest patch which fix the review point as well as >> additional clean-up. > > +/* > + * Read the tuple for given reader into nowait mode, and form the tuple >

Re: [HACKERS] Gather Merge

On Sat, Nov 5, 2016 at 2:42 AM, Tom Lane wrote: > Michael Paquier writes: >> On Fri, Nov 4, 2016 at 12:00 PM, Thomas Munro >> wrote: >>> Shouldn't this say just "(c) 2016, PostgreSQL Global Development >>> Group"? Are we supposed to be blaming the University of California >>> for new files? > >

Re: [HACKERS] Gather Merge

On Sat, Nov 5, 2016 at 1:55 AM, Robert Haas wrote: > On Thu, Nov 3, 2016 at 11:00 PM, Thomas Munro > wrote: >> + /* >> + * Avoid log(0)... >> + */ >> + N = (path->num_workers < 2) ? 2.0 : (double) path->num_workers; >> + logN = LOG2(N); >> ... >> + /* Per-tuple heap maintenance cost */ >> + run_c

Re: [HACKERS] Gather Merge

Michael Paquier writes: > On Fri, Nov 4, 2016 at 12:00 PM, Thomas Munro > wrote: >> Shouldn't this say just "(c) 2016, PostgreSQL Global Development >> Group"? Are we supposed to be blaming the University of California >> for new files? > If the new file contains a portion of code from this age

Re: [HACKERS] Gather Merge

On Thu, Nov 3, 2016 at 11:00 PM, Thomas Munro wrote: > + /* > + * Avoid log(0)... > + */ > + N = (path->num_workers < 2) ? 2.0 : (double) path->num_workers; > + logN = LOG2(N); > ... > + /* Per-tuple heap maintenance cost */ > + run_cost += path->path.rows * comparison_cost * 2.0 * logN; > > Why m

Re: [HACKERS] Gather Merge

On Fri, Nov 4, 2016 at 12:00 PM, Thomas Munro wrote: > + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group > + * Portions Copyright (c) 1994, Regents of the University of California > > Shouldn't this say just "(c) 2016, PostgreSQL Global Development > Group"? Are we suppose

Re: [HACKERS] Gather Merge

On Thu, Oct 27, 2016 at 10:50 PM, Rushabh Lathia wrote: > Please find attached latest patch which fix the review point as well as > additional clean-up. I've signed up to review this patch and I'm planning to do some testing. Here's some initial feedback after a quick read-through: + if (gather

Re: [HACKERS] Gather Merge

Please find attached latest patch which fix the review point as well as additional clean-up. - Get rid of funnel_slot as its not needed for the Gather Merge - renamed fill_tuple_array to form_tuple_array - Fix possible infinite loop into gather_merge_init (Reported by Amit Kaplia) - Fix tqueue.c m

Re: [HACKERS] Gather Merge

On Thu, Oct 20, 2016 at 1:12 PM, Amit Kapila wrote: > On Tue, Oct 18, 2016 at 5:29 PM, Rushabh Lathia > wrote: > > On Mon, Oct 17, 2016 at 2:26 PM, Amit Kapila > > wrote: > >> > >> There is lot of common code between ExecGatherMerge and ExecGather. > >> Do you think it makes sense to have a com

Re: [HACKERS] Gather Merge

On Thu, Oct 20, 2016 at 12:22 AM, Peter Geoghegan wrote: > On Tue, Oct 4, 2016 at 11:05 PM, Rushabh Lathia > wrote: > > Query 4: With GM 7901.480 -> Without GM 9064.776 > > Query 5: With GM 53452.126 -> Without GM 55059.511 > > Query 9: With GM 52613.132 -> Without GM 98206.793 > > Query 15:

Re: [HACKERS] Gather Merge

On Tue, Oct 18, 2016 at 5:29 PM, Rushabh Lathia wrote: > On Mon, Oct 17, 2016 at 2:26 PM, Amit Kapila > wrote: >> >> There is lot of common code between ExecGatherMerge and ExecGather. >> Do you think it makes sense to have a common function to avoid the >> duplicity? >> >> I see there are small

Re: [HACKERS] Gather Merge

On Tue, Oct 4, 2016 at 11:05 PM, Rushabh Lathia wrote: > Query 4: With GM 7901.480 -> Without GM 9064.776 > Query 5: With GM 53452.126 -> Without GM 55059.511 > Query 9: With GM 52613.132 -> Without GM 98206.793 > Query 15: With GM 68051.058 -> Without GM 68918.378 > Query 17: With GM 129236.07

Re: [HACKERS] Gather Merge

Thanks Amit for reviewing this patch. On Mon, Oct 17, 2016 at 2:26 PM, Amit Kapila wrote: > On Wed, Oct 5, 2016 at 11:35 AM, Rushabh Lathia > wrote: > > Hi hackers, > > > > Attached is the patch to implement Gather Merge. > > > > Couple of review comments: > > 1. > ExecGatherMerge() > { > .. >

Re: [HACKERS] Gather Merge

On Mon, Oct 17, 2016 at 4:56 AM, Amit Kapila wrote: > + node->nreaders < 2) ... > I see there are small discrepancies in both the codes like I don't see > the use of single_copy flag, as it is present in gather node. single_copy doesn't make sense for GatherMerge, because the whole point is to me

Re: [HACKERS] Gather Merge

On Wed, Oct 5, 2016 at 11:35 AM, Rushabh Lathia wrote: > Hi hackers, > > Attached is the patch to implement Gather Merge. > Couple of review comments: 1. ExecGatherMerge() { .. + /* No workers? Then never mind. */ + if (!got_any_worker || + node->nreaders < 2) + { + ExecShutdownGatherMergeWorker