Re: [HACKERS] Partition-wise aggregation/grouping

> Regarding apply_scanjoin_target_to_paths in 0001 and 0007, it seems > like what happens is: we first build an Append path for the topmost > scan/join rel. That uses paths from the individual relations that > don't necessarily produce the final scan/join target. Then we mutate > those

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Apr 10, 2018 at 7:30 PM, David Steele wrote: > Hi Jeevan, > > On 4/2/18 10:57 AM, Robert Haas wrote: > > On Thu, Mar 29, 2018 at 9:02 AM, Jeevan Chalke > > wrote: > >> Yep, I see the risk. > > > > Committed 0001 last week and 0002

Re: [HACKERS] Partition-wise aggregation/grouping

Hi Jeevan, On 4/2/18 10:57 AM, Robert Haas wrote: > On Thu, Mar 29, 2018 at 9:02 AM, Jeevan Chalke > wrote: >> Yep, I see the risk. > > Committed 0001 last week and 0002 just now. I don't really see 0003 a > a critical need. If somebody demonstrates that this

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 29, 2018 at 9:02 AM, Jeevan Chalke wrote: > Yep, I see the risk. Committed 0001 last week and 0002 just now. I don't really see 0003 a a critical need. If somebody demonstrates that this saves a meaningful amount of planning time, we can consider

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 29, 2018 at 4:13 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Mar 28, 2018 at 7:21 PM, Ashutosh Bapat > wrote: > > > > Ah sorry, I was wrong about remote_conds. remote_conds and local_conds > > are basically the conditions on

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 28, 2018 at 7:21 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Tue, Mar 27, 2018 at 2:43 PM, Jeevan Chalke > wrote: > > > I am not sure on what we should Assetrt here. Note that we end-up here > only > > when doing grouping, and

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 28, 2018 at 7:21 PM, Ashutosh Bapat wrote: > > Ah sorry, I was wrong about remote_conds. remote_conds and local_conds > are basically the conditions on the relation being pushed down. > havingQuals are conditions on a grouped relation so treating them

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 27, 2018 at 2:43 PM, Jeevan Chalke wrote: >> else if (IS_UPPER_REL(foreignrel)) >> { >> PgFdwRelationInfo *ofpinfo; >> -PathTarget *ptarget = >> root->upper_targets[UPPERREL_GROUP_AGG]; >> +

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 26, 2018 at 5:24 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Fri, Mar 23, 2018 at 4:35 PM, Jeevan Chalke > wrote: > > > > Changes related to postgres_fdw which allows pushing aggregate on the > > foreign server is not yet

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 27, 2018 at 3:33 AM, Andres Freund wrote: > Hi, > > On 2018-03-23 17:01:54 +0530, Jeevan Chalke wrote: > > Attached patch which fixes that. > > Thanks, will push. For the future, I'd be more likely to notice if you > CC me ;) > Sure. Thanks. > > > > However, I

Re: [HACKERS] Partition-wise aggregation/grouping

Hi, On 2018-03-23 17:01:54 +0530, Jeevan Chalke wrote: > Attached patch which fixes that. Thanks, will push. For the future, I'd be more likely to notice if you CC me ;) > However, I am not sure whether it is expected to have stable regression run > with installcheck having local settings. >

Re: [HACKERS] Partition-wise aggregation/grouping

Hi Robert, On pgsql-committers Andres reported one concern about test case failure with installcheck with local settings. (Sorry, I have not subscribed to that mailing list and thus not able to reply there). Attached patch which fixes that. However, I am not sure whether it is expected to have

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 10:28 PM, Robert Haas wrote: > On Thu, Mar 22, 2018 at 6:15 AM, Jeevan Chalke > wrote: > > Leeks cleaner now. Thanks for refactoring it. > > > > I have merged these changes and flatten all previuos changes into the >

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 6:15 AM, Jeevan Chalke wrote: > Leeks cleaner now. Thanks for refactoring it. > > I have merged these changes and flatten all previuos changes into the main > patch. Committed 0001-0005. I made a few further modifications. These were

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 10:06 AM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas > wrote: > > > > Is there a good reason not to use input_rel->relids as the input to > > fetch_upper_rel() in all cases, rather than

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas wrote: > On Wed, Mar 21, 2018 at 11:33 AM, Jeevan Chalke > wrote: > > Let me try to explain this: > > 1. GROUPING_CAN_PARTITIONWISE_AGG > > 2. extra->is_partial_aggregation > > 3.

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 22, 2018 at 3:26 AM, Robert Haas wrote: > > Is there a good reason not to use input_rel->relids as the input to > fetch_upper_rel() in all cases, rather than just at subordinate > levels? > That would simplify some code in these patches. We have set

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 21, 2018 at 11:33 AM, Jeevan Chalke wrote: > Let me try to explain this: > 1. GROUPING_CAN_PARTITIONWISE_AGG > 2. extra->is_partial_aggregation > 3. extra->perform_partial_partitionwise_aggregation Please find attached an incremental patch that

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 21, 2018 at 7:46 PM, Robert Haas wrote: > On Wed, Mar 21, 2018 at 8:01 AM, Jeevan Chalke > wrote: > >> In the patch as proposed, create_partial_grouping_paths() can get > >> called even if GROUPING_CAN_PARTIAL_AGG is not set. I

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 21, 2018 at 8:01 AM, Jeevan Chalke wrote: >> In the patch as proposed, create_partial_grouping_paths() can get >> called even if GROUPING_CAN_PARTIAL_AGG is not set. I think that's >> wrong. > > I don't think so. For parallel case, we do check that.

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 21, 2018 at 2:04 AM, Robert Haas wrote: > On Tue, Mar 20, 2018 at 10:46 AM, Jeevan Chalke > wrote: > > I have added all these three patches in the attached patch-set and > rebased > > my changes over it. > > > > However, I have

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 20, 2018 at 10:46 AM, Jeevan Chalke wrote: > I have added all these three patches in the attached patch-set and rebased > my changes over it. > > However, I have not yet made this patch-set dependednt on UPPERREL_TLIST > changes you have proposed on

Re: [HACKERS] Partition-wise aggregation/grouping

Hi, On Mon, Mar 19, 2018 at 10:56 PM, Robert Haas wrote: > On Fri, Mar 16, 2018 at 1:50 PM, Ashutosh Bapat > wrote: > > Ok. That looks good. > > Here's an updated version. In this version, based on a voice > discussion with Ashutosh and

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 19, 2018 at 11:15 PM, Robert Haas wrote: > On Fri, Mar 16, 2018 at 1:50 PM, Ashutosh Bapat > wrote: >>> This patch also renames can_parallel_agg to >>> can_partial_agg and removes the parallelism-specific bits from it. >> >> I

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 16, 2018 at 1:50 PM, Ashutosh Bapat wrote: >> This patch also renames can_parallel_agg to >> can_partial_agg and removes the parallelism-specific bits from it. > > I think we need to update the comments in this function to use phrase > "partial

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 16, 2018 at 1:50 PM, Ashutosh Bapat wrote: > Ok. That looks good. Here's an updated version. In this version, based on a voice discussion with Ashutosh and Jeevan, I adjusted 0001 to combine it with an earlier idea of splitting Gather/Gather Merge

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 16, 2018 at 3:19 AM, Robert Haas wrote: > On Thu, Mar 15, 2018 at 2:46 PM, Robert Haas wrote: >> I wonder if we could simplify things by copying more information from >> the parent grouping rel to the child grouping rels. > > On further

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 16, 2018 at 12:16 AM, Robert Haas wrote: > > - partial_costs_set. The comment in compute_group_path_extra_data > doesn't look good. It says "Set partial aggregation costs if we are > going to calculate partial aggregates in make_grouping_rels()", but > what it

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 9:19 PM, Robert Haas wrote: > On Thu, Mar 15, 2018 at 9:46 AM, Jeevan Chalke > wrote: >> Hmm.. you are right. Done. > > I don't see a reason to hold off on committing 0002 and 0003, so I've > done that now; since they

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 7:46 PM, Robert Haas wrote: > On Thu, Mar 15, 2018 at 6:08 AM, Ashutosh Bapat > wrote: >> In current create_grouping_paths() (without any of your patches >> applied) we first create partial paths in partially grouped

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 2:46 PM, Robert Haas wrote: > I wonder if we could simplify things by copying more information from > the parent grouping rel to the child grouping rels. On further review, it seems like a better idea is to generate the partial grouping relations

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 11:49 AM, Robert Haas wrote: > I'm going to go spend some time looking at 0005 next. It looks to me > like it's generally going in a very promising direction, but I need to > study the details more. On further study this patch is doing a number of

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 9:46 AM, Jeevan Chalke wrote: > Hmm.. you are right. Done. I don't see a reason to hold off on committing 0002 and 0003, so I've done that now; since they are closely related changes, I pushed them as a single commit. It probably could've

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 6:08 AM, Ashutosh Bapat wrote: > In current create_grouping_paths() (without any of your patches > applied) we first create partial paths in partially grouped rel and > then add parallel path to grouped rel using those partial paths. Then >

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 15, 2018 at 3:38 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > The patchset needs rebase. I have rebased those on the latest head > and made following changes. > > Argument force_partial_agg is added after output arguments to > make_grouping_rels(). Moved it before

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 14, 2018 at 7:51 PM, Jeevan Chalke wrote: > > > On Tue, Mar 13, 2018 at 7:43 PM, Jeevan Chalke > wrote: >> >> Hi, >> >> The patch-set is complete now. But still, there is a scope of some comment >> improvements due to

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 13, 2018 at 7:43 PM, Jeevan Chalke < jeevan.cha...@enterprisedb.com> wrote: > Hi, > > The patch-set is complete now. But still, there is a scope of some comment > improvements due to all these refactorings. I will work on it. Also, need > to update few documentations and indentations

Re: [HACKERS] Partition-wise aggregation/grouping

Hi, I have resolved all the comments/issues reported in this new patch-set. Changes done by Ashutosh Bapat for splitting out create_grouping_paths() into separate functions so that partitionwise aggregate code will use them were based on my partitionwise aggregate changes. Those were like

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 12, 2018 at 7:49 PM, Jeevan Chalke wrote: > > > On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat > wrote: >> >> On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat >> >> GroupPathExtraData now completely absorbs members from

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat > > GroupPathExtraData now completely absorbs members from and replaces > OtherUpperPathExtraData. This means that we have to consider a way to > pass

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 12, 2018 at 6:07 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat > wrote: > > On Thu, Mar 8, 2018 at 7:31 PM, Robert Haas > wrote: > >> > >> This kind of goes along

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 9, 2018 at 4:21 PM, Ashutosh Bapat wrote: > On Thu, Mar 8, 2018 at 7:31 PM, Robert Haas wrote: >> >> This kind of goes along with the suggestion I made yesterday to >> introduce a new function, which at the time I proposed

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 7:31 PM, Robert Haas wrote: > > This kind of goes along with the suggestion I made yesterday to > introduce a new function, which at the time I proposed calling > initialize_grouping_rel(), to set up new grouped or partially grouped > relations. By

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 7:49 PM, Robert Haas wrote: > On Thu, Mar 8, 2018 at 9:15 AM, Jeevan Chalke > wrote: > > I am not sure why we don't set reltarget into the grouped_rel too. > > > > But if we do so like we did in partially_grouped_rel,

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 9:15 AM, Jeevan Chalke wrote: > I am not sure why we don't set reltarget into the grouped_rel too. > > But if we do so like we did in partially_grouped_rel, then it will be lot > easier for partitionwise aggregate as then we don't have to

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 1:15 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > On Wed, Mar 7, 2018 at 8:07 PM, Jeevan Chalke > wrote: > Here are some more review comments esp. on > try_partitionwise_grouping() function. BTW name of that function >

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 8, 2018 at 2:45 AM, Ashutosh Bapat wrote: > +grouped_rel->part_scheme = input_rel->part_scheme; > +grouped_rel->nparts = nparts; > +grouped_rel->boundinfo = input_rel->boundinfo; > +grouped_rel->part_rels = part_rels; >

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 7, 2018 at 8:07 PM, Jeevan Chalke wrote: Here are some more review comments esp. on try_partitionwise_grouping() function. BTW name of that function doesn't go in sync with enable_partitionwise_aggregation (which btw is in sync with enable_fooagg GUCs).

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Mar 7, 2018 at 1:45 AM, Robert Haas wrote: > On Tue, Mar 6, 2018 at 5:31 AM, Jeevan Chalke > wrote: > > This is in-lined with enable_hashagg GUC. Do you think > > enable_partitionwise_aggregate seems better? But it will be not >

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 6, 2018 at 4:59 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > Hi Jeevan, > I am back reviewing this. Here are some comments. > > @@ -1415,7 +1413,8 @@ add_paths_to_append_rel(PlannerInfo *root, > RelOptInfo *rel, > * the unparameterized Append path we are

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 6, 2018 at 7:52 PM, Jeevan Chalke wrote: > > > Changes look good to me and refactoring will be useful for partitionwise > patches. > > However, will it be good if we add agg_costs into the GroupPathExtraData > too? > Also can we pass this to the

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 6, 2018 at 5:31 AM, Jeevan Chalke wrote: > This is in-lined with enable_hashagg GUC. Do you think > enable_partitionwise_aggregate seems better? But it will be not consistent > with other GUC names like enable_hashagg then. Well, if I had my way,

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 6, 2018 at 4:59 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > Hi Jeevan, > I am back reviewing this. Here are some comments. > > @@ -1415,7 +1413,8 @@ add_paths_to_append_rel(PlannerInfo *root, > RelOptInfo *rel, > * the unparameterized Append path we are

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Mar 6, 2018 at 2:29 AM, Robert Haas wrote: > On Mon, Mar 5, 2018 at 3:56 AM, Jeevan Chalke > wrote: > > However, to perform Gather or Gather Merge once we have all partial paths > > ready, and to avoid too many existing code

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 5, 2018 at 3:56 AM, Jeevan Chalke wrote: > However, to perform Gather or Gather Merge once we have all partial paths > ready, and to avoid too many existing code rearrangement, I am calling > try_partitionwise_grouping() before we do any

Re: [HACKERS] Partition-wise aggregation/grouping

On Sat, Mar 3, 2018 at 12:12 AM, Robert Haas wrote: > On Thu, Mar 1, 2018 at 4:52 PM, Robert Haas wrote: > > This is not a full review, but I'm out of time for right now. > > Another thing I see here now is that create_grouping_paths() and >

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 2, 2018 at 3:22 AM, Robert Haas wrote: > On Thu, Mar 1, 2018 at 5:34 AM, Jeevan Chalke > wrote: > > Attached new patchset after rebasing my changes over these changes and on > > latest HEAD. > > +* We have already

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Mar 1, 2018 at 4:52 PM, Robert Haas wrote: > This is not a full review, but I'm out of time for right now. Another thing I see here now is that create_grouping_paths() and create_child_grouping_paths() are extremely similar. Isn't there some way we can refactor

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Feb 26, 2018 at 8:03 PM, Robert Haas wrote: > > Committed after incorporating your other fixes and updating the > optimizer README. > Attached new patchset after rebasing my changes over these changes and on latest HEAD. > > -- > Robert Haas > EnterpriseDB:

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Feb 27, 2018 at 4:29 AM, Jeevan Chalke wrote: > Hi Robert, > I had a look over his provided testcase and observed that when we create a > Gather Merge path over a cheapest partial path by sorting it explicitly as > generate_gather_paths won't consider it,

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Feb 26, 2018 at 6:38 AM, Jeevan Chalke wrote: > One idea I thought about is to memcpy the struct once we have set all > required fields for grouped_rel so that we don't have to do similar stuff > for partially_grouped_rel. I think that would be a poor

Re: [HACKERS] Partition-wise aggregation/grouping

Hi Robert, On Fri, Feb 23, 2018 at 2:53 AM, Robert Haas wrote: > On Thu, Feb 8, 2018 at 8:05 AM, Jeevan Chalke > wrote: > > In this attached version, I have rebased my changes over new design of > > partially_grouped_rel. The preparatory

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Feb 8, 2018 at 8:05 AM, Jeevan Chalke wrote: > In this attached version, I have rebased my changes over new design of > partially_grouped_rel. The preparatory changes of adding > partially_grouped_rel are in 0001. I spent today hacking in 0001; results

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Feb 8, 2018 at 8:05 AM, Jeevan Chalke wrote: > 0003 - 0006 are refactoring patches as before. I have committed 0006 with some modifications. In particular, [1] I revised the comments and formatting; [2] I made cost_merge_append() add cpu_tuple_cost *

Re: [HACKERS] Partition-wise aggregation/grouping

Commit 2fb1abaeb016aeb45b9e6d0b81b7a7e92bb251b9, changed enable_partition_wise_join to enable_partitionwise_join. This patch too should use enable_partitionwise_agg instead of enable_partition_wise_agg.

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Feb 14, 2018 at 12:17 PM, Rafia Sabih wrote: > On Tue, Feb 13, 2018 at 6:21 PM, Jeevan Chalke < > jeevan.cha...@enterprisedb.com> wrote: > >> >> I see that partition-wise aggregate plan too uses parallel index, am I >> missing something? >> >> > You're

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Feb 13, 2018 at 6:21 PM, Jeevan Chalke < jeevan.cha...@enterprisedb.com> wrote: > > I see that partition-wise aggregate plan too uses parallel index, am I > missing something? > > You're right, I missed that, oops. > >> Q18 takes some 390 secs with patch and some 147 secs without it. >>

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Feb 13, 2018 at 12:37 PM, Rafia Sabih wrote: > > I was testing this patch for TPC-H benchmarking and came across following > results, > Thanks Rafia for testing this with TPC-H benchmarking. > > Q1 completes in 229 secs with patch and in 66 secs without

Re: [HACKERS] Partition-wise aggregation/grouping

Hi, In this attached version, I have rebased my changes over new design of partially_grouped_rel. The preparatory changes of adding partially_grouped_rel are in 0001. Also to minimize finalization code duplication, I have refactored them into two separate functions,

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Feb 2, 2018 at 8:25 AM, Jeevan Chalke wrote: >> The problem is that create_partition_agg_paths() is doing *exactly* >> same thing that add_paths_to_grouping_rel() is already doing inside >> the blocks that say if (grouped_rel->partial_pathlist). We don't

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Feb 2, 2018 at 1:41 AM, Robert Haas wrote: > On Thu, Feb 1, 2018 at 8:59 AM, Jeevan Chalke > wrote: > > I wrote a patch for this (on current HEAD) and attached separately here. > > Please have a look. > > Yes, this is approximately

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Feb 1, 2018 at 8:59 AM, Jeevan Chalke wrote: > I wrote a patch for this (on current HEAD) and attached separately here. > Please have a look. Yes, this is approximately what I had in mind, though it needs more work (e.g. it doesn't removing the clearing of

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Feb 1, 2018 at 1:11 AM, Robert Haas wrote: > On Mon, Jan 29, 2018 at 3:42 AM, Jeevan Chalke > wrote: > > Attached new patch set and rebased it on latest HEAD. > > I strongly dislike add_single_path_to_append_rel. It adds branches >

Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Jan 29, 2018 at 3:42 AM, Jeevan Chalke wrote: > Attached new patch set and rebased it on latest HEAD. I strongly dislike add_single_path_to_append_rel. It adds branches and complexity to code that is already very complex. Most importantly, why are we

Re: [HACKERS] Partition-wise aggregation/grouping

On Sat, Jan 27, 2018 at 1:35 AM, Robert Haas wrote: > On Thu, Jan 18, 2018 at 8:55 AM, Jeevan Chalke > wrote: > > Attached patch with other review points fixed. > > Committed 0001 and 0002 together, with some cosmetic changes, > including

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Jan 18, 2018 at 8:55 AM, Jeevan Chalke wrote: > Attached patch with other review points fixed. Committed 0001 and 0002 together, with some cosmetic changes, including fixing pgindent damage. Please pgindent your patches before submitting. -- Robert Haas

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Jan 16, 2018 at 3:56 AM, Jeevan Chalke wrote: > I will make your suggested changes that is merge create_sort_agg_path() and > create_hash_agg_path(). Will name that function as > create_sort_and_hash_agg_paths(). I suggest add_paths_to_grouping_rel() and

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Jan 16, 2018 at 3:41 AM, Robert Haas wrote: > On Thu, Jan 11, 2018 at 6:00 AM, Jeevan Chalke > wrote: > > Attached new set of patches adding this. Only patch 0007 (main patch) > and > > 0008 (testcase patch) has changed. > > > >

Re: [HACKERS] Partition-wise aggregation/grouping

> On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat < > ashutosh.ba...@enterprisedb.com> wrote: > >> >> + >> +-- Partial aggregation as GROUP BY clause does not match with PARTITION >> KEY >> +EXPLAIN (COSTS OFF) >> +SELECT b, sum(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1, 2, 3; >> +

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > Sure no problem. Take your time. Here's set of comments for 0008. That > ends the first read of all the patches (2nd reading for the core > changes) > > +-- Also, disable parallel paths. > +SET

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > Here are review comments for 0009 > Only full aggregation is pushed on the remote server. > > I think we can live with that for a while but we need to be able to push > down > partial aggregates to the

Re: [HACKERS] Partition-wise aggregation/grouping

Attached patchset with all the review comments reported so far. On Fri, Dec 1, 2017 at 6:11 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > Continuing with review of 0007. > > + > +/* Copy input rels's relids to grouped rel */ > +grouped_rel->relids = input_rel->relids; >

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat wrote: > + > +-- Test when input relation for grouping is dummy > +EXPLAIN (COSTS OFF) > +SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c; > + QUERY PLAN > + > +

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat wrote: > > + > +EXPLAIN (COSTS OFF) > +SELECT a FROM pagg_tab GROUP BY a ORDER BY 1; > + QUERY PLAN > +- > + Group > + Group Key: pagg_tab_p1.a > +

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Dec 13, 2017 at 6:37 PM, Jeevan Chalke wrote: > > > On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat > wrote: >> >> Here are review comments for 0009 > > > Thank you, Ashutosh for the detailed review so far. > > I am working

Re: [HACKERS] Partition-wise aggregation/grouping

Here are review comments for 0009 Only full aggregation is pushed on the remote server. I think we can live with that for a while but we need to be able to push down partial aggregates to the foreign server. I agree that it needs some infrastructure to serialized and deserialize the partial

Re: [HACKERS] Partition-wise aggregation/grouping

On Sat, Dec 2, 2017 at 4:08 AM, Robert Haas wrote: > On Fri, Dec 1, 2017 at 7:41 AM, Ashutosh Bapat > wrote: >> This code creates plans where there are multiple Gather nodes under an Append >> node. > > We should avoid that. Starting and

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Dec 1, 2017 at 7:41 AM, Ashutosh Bapat wrote: > This code creates plans where there are multiple Gather nodes under an Append > node. We should avoid that. Starting and stopping workers is inefficient, and precludes things like turning the Append into a

Re: [HACKERS] Partition-wise aggregation/grouping

Continuing with review of 0007. + +/* Copy input rels's relids to grouped rel */ +grouped_rel->relids = input_rel->relids; Isn't this done in fetch_upper_rel()? Why do we need it here? There's also a similar hunk in create_grouping_paths() which doesn't look appropriate. I guess, you

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Nov 28, 2017 at 5:50 PM, Jeevan Chalke wrote: > [snip] This is still a hot topic so I am moving it to next CF. -- Michael

Re: [HACKERS] Partition-wise aggregation/grouping

On Tue, Nov 28, 2017 at 12:37 PM, Rajkumar Raghuwanshi < rajkumar.raghuwan...@enterprisedb.com> wrote: > On Thu, Nov 23, 2017 at 6:38 PM, Jeevan Chalke > wrote: > > Let me know if I missed any comment to be fixed. > > Hi, > > I have applied v8 patches on commit id

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Nov 23, 2017 at 6:38 PM, Jeevan Chalke wrote: > Let me know if I missed any comment to be fixed. Hi, I have applied v8 patches on commit id 8735978e7aebfbc499843630131c18d1f7346c79, and getting below observation, please take a look. Observation: "when

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Nov 23, 2017 at 6:38 PM, Jeevan Chalke wrote: > > > Agree. However, there was ab existing comment in create_append_path() saying > "We don't bother with inventing a cost_append(), but just do it here", which > implies at sometime in future we may need it;

Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Nov 17, 2017 at 7:24 AM, Ashutosh Bapat wrote: > + * this value should be multiplied with cpu_tuple_cost wherever applicable. > + */ > +#define DEFAULT_APPEND_COST_FACTOR 0.5 > > I am wondering whether we should just define > #define APPEND_TUPLE_COST

Re: [HACKERS] Partition-wise aggregation/grouping

On Wed, Nov 15, 2017 at 5:31 PM, Jeevan Chalke wrote: > > OK. Done in the attached patch set. > > I have rebased all my patches on latest HEAD which is at > 7518049980be1d90264addab003476ae105f70d4 > > Thanks These are review comments for the last set and I think

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Nov 16, 2017 at 6:02 AM, David Rowley wrote: > On 16 November 2017 at 05:57, Konstantin Knizhnik > wrote: >> The main problem IMHO is that there are a lot of different threads and >> patches related with this topic:( >> And it is

Re: [HACKERS] Partition-wise aggregation/grouping

On 16 November 2017 at 05:57, Konstantin Knizhnik wrote: > The main problem IMHO is that there are a lot of different threads and > patches related with this topic:( > And it is very difficult to combine all of them together to achieve the > final goal: efficient

Re: [HACKERS] Partition-wise aggregation/grouping

On 15.11.2017 13:35, Jeevan Chalke wrote: As explained by Ashutosh Bapat in reply https://www.postgresql.org/message-id/CAFjFpRdpeMTd8kYbM_x0769V-aEKst5Nkg3+coG=8ki7s8z...@mail.gmail.com we cannot rely on just aggtype==aggtranstype. Obviously this check (aggtype==aggtranstype) is not

Re: [HACKERS] Partition-wise aggregation/grouping

On Thu, Nov 2, 2017 at 7:36 AM, Robert Haas wrote: > On Wed, Nov 1, 2017 at 6:20 PM, Jeevan Chalke > wrote: > > Yep. > > But as David reported earlier, if we remove the first part i.e. adding > > cpu_operator_cost per tuple, Merge Append

Re: [HACKERS] Partition-wise aggregation/grouping

On Sun, Nov 12, 2017 at 1:59 AM, Konstantin Knizhnik < k.knizh...@postgrespro.ru> wrote: > On 10/27/2017 02:01 PM, Jeevan Chalke wrote: > >> Hi, >> >> Attached new patch-set here. Changes include: >> >> 1. Added separate patch for costing Append node as discussed up-front in >> the >> patch-set.

  1   2   >