> 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 relations
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 just now. I don't really see 0003 a
> > a critical need.
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 saves a
> meaningful amount of pla
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 that part for a
future release.
--
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 the relation being pushed down.
>
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 thus I don't think we need any Asser
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 like
> baserestrictinfo or join co
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];
>> +PathTarget *ptarget = fpinfo->grouped_ta
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 committed. Due to this, we will end up ge
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 am not sure whether i
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.
> Fo
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 committed. Due to this, we will end up getting an
> error when we have foreign partitions + aggregation.
>
> Attached 0001 patch here (0006 fr
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 s
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
> main
> > patch.
>
> Committed 0001-0005.
Thanks Rober
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
mostly cosmetic, but with two excepti
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 just at subordinate
> > le
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. extra->perform_partial_partitionwise_aggregation
>
> Please find attach
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
upper_rel->relids to NULL for non-o
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 attempts to refactor
this logic into a s
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 think that's
> >> wrong.
> >
> > I don't think so. For
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. And for partitionwise
> aggregation
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 not yet made this patch-set dependednt on UPPERREL_TLIST
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 another mail-thread and thus it has
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 Jeevan, I adjusted 0001 to combine it
> with an earlier i
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 think we need to update the comments in this function to u
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 aggregation" instead of "parallel aggrega
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 path generation
out of the function
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 review, it seems like a better idea is to genera
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 means is something more
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 are closely related changes, I pushed them
> as a singl
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 rel and
>> then add parallel path to grouped rel using t
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 from the grouping relation
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 things, some of which
se
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 just been included in the
main pa
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
> we hand over this to FDW and exte
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
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 all these refactorings. I will work on it. Also, need to
>> update
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 e
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
refacto
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 and replaces
>> OtherUpperPathExtraData. This means that we have t
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 GroupPat
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 with the suggestion I made yesterday to
> >> introduce a
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 calling
>> initialize_grouping_rel(), to set up new grouped or
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 doing that it would be ea
On Thu, Mar 8, 2018 at 8:02 PM, Jeevan Chalke
wrote:
>
>
> 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_g
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, then it will be lot
> > easier for partitionwise aggreg
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 pass target to
> functions creating
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
> doesn't go in sync with enable_partit
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;
>
> You need to set the part_exprs w
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). But it goes in sync with
create_
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
> consistent
> > with other GUC names like enable_hashagg the
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 con
On Wed, Mar 7, 2018 at 10:04 AM, Ashutosh Bapat
wrote:
> 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?
>>
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 add_partial_paths_to_grouping_rel() and
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, enable_hashagg would be spelled
enable
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 con
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 constructing for the parent.
* If not, there's no workable unparameterized path.
*/
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 rearrangement, I am calling
> > try_partitionwise_grouping() be
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 aggregation/grouping on whole
> relation. By doi
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
> create_child_grouping_paths() are extremely similar.
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 created a Gather or Gather Merge path atop
> cheapest
> +
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 things so that we can reus
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 created a Gather or Gather Merge path atop cheapest
+* partial path. Thus the partial path referenced by the
Gather no
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: http://www.enterprisedb.com
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, we accidentally used cheapest
> pa
Hi Robert,
On Mon, Feb 26, 2018 at 8:03 PM, Robert Haas wrote:
>
> Committed after incorporating your other fixes and updating the
> optimizer README.
>
Thanks Robert.
Off-list Rajkumar has reported an issue. When we have enable_hashagg set to
false, and Gather Merge path is chosen, it ended-
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 idea. We want to copy a few specific
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 changes of adding
> > partially_grouped_rel are in 0001.
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 attached. The big change from
your
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 * APPEND_CPU_COST_MULTIPLIER in lieu of
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.
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 right, I missed that, oops.
>
>>
>>>
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.
>>
>
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 it. It looks
> like with this pa
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, finalize_sorted_partial_agg_pa
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 need
>> two copies of that code.
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 what I had in mind, though it needs more
> work (e.g. it
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 the grouped_rel's
partial_pathli
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
> and complexity to code that is already very complex. M
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 adding paths to fields in
OtherUpper
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 fixing pgindent damage.
Thanks Robert.
> Please pg
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
EnterpriseDB: http://www.enterpr
On Wed, Jan 17, 2018 at 1:18 AM, Robert Haas wrote:
> 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 sugge
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
add_partial_paths_to_grouping_rel(
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.
> >
> > Please have a look and let me know if I missed any.
>
> I
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.
>
> Please have a look and let me know if I missed any.
I spent a little time studying 0001 and 0002 today, as well as their
rela
> 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;
>> +
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 max_parall
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 fore
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;
>
>
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
> +
> + HashAggregate
> + Group Key: pagg_tab.
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
> + -> Merge Append
> + Sor
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 on your reviews but since parallel Append is now committed,
> I n
On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:
> Here are review comments for 0009
>
Thank you, Ashutosh for the detailed review so far.
I am working on your reviews but since parallel Append is now committed,
I need to re-base my changes over it and ne
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 aggreg
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 stopping workers is inefficient,
> and precludes things li
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 Parallel Append.
> AFAIU, the wor
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 need
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
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 8735978e7aebfbc499843630131c18
>
1 - 100 of 110 matches
Mail list logo
