Re: [HACKERS] Partition-wise aggregation/grouping

On Mon, Mar 26, 2018 at 5:24 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Fri, Mar 23, 2018 at 4:35 PM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> 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 from my earlier patch-set) which adds
> support
> > for this and thus will not have any error.
>

I have observed that, target member in GroupPathExtraData is not needed as
we store the target in grouped_rel itself.
Attached 0001 patch to remove that.

>
>
> else if (IS_UPPER_REL(foreignrel))
> {
> PgFdwRelationInfo *ofpinfo;
> - PathTarget *ptarget = root->upper_targets[UPPERREL_
> GROUP_AGG];
> + PathTarget *ptarget = fpinfo->grouped_target;
>
> I think we need an assert there to make sure that the upper relation is a
> grouping relation. That way any future push down will notice it.
>

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 Assert here.
Let me know if I missed anything.

>
> - get_agg_clause_costs(root, (Node *)
> root->parse->havingQual,
> + get_agg_clause_costs(root, fpinfo->havingQual,
> AGGSPLIT_SIMPLE, &aggcosts);
> }
> Should we pass agg costs as well through GroupPathExtraData to avoid
> calculating it again in this function?
>

Adding an extra member in GroupPathExtraData just for FDW does not look
good to me.
But yes, if we do that, then we can save this calculation.
Let me know if its OK to have an extra member for just FDW use, will
prepare a separate patch for that.

>
> /*
> + /*
> + * Store passed-in target and havingQual in fpinfo. If its a foreign
> + * partition, then path target and HAVING quals fetched from the root
> are
> + * not correct as Vars within it won't match with this child relation.
> + * However, server passed them through extra and thus fetch from it.
> + */
> + if (extra)
> + {
> + /* Partial aggregates are not supported. */
> + Assert(extra->patype != PARTITIONWISE_AGGREGATE_PARTIAL);
> +
> + fpinfo->grouped_target = extra->target;
> + fpinfo->havingQual = extra->havingQual;
> + }
> + else
> + {
> + fpinfo->grouped_target = root->upper_targets[UPPERREL_GROUP_AGG];
> + fpinfo->havingQual = parse->havingQual;
> + }
> I think both these cases, extra should always be present whether a child
> relation or a parent relation. Just pick from extra always.
>

Yes.
Done.

>
> /* Grouping information */
> List *grouped_tlist;
> + PathTarget *grouped_target;
>
> We should use the target stored in the grouped rel directly.
>

Yep.

>
> + Node *havingQual;
> I am wondering whether we could use remote_conds member for storing this.
>

This havingQual is later checked for shippability and classified into
pushable and non-pushable quals and stored in remote_conds and local_conds
respectively.
Storing it directly in remote_conds and then splitting it does not look
good to me.
Also, remote_conds is list of RestrictInfo nodes whereas havingQual is not.
So using that for storing havingQual does not make sense. So better to have
a separate member in PgFdwRelationInfo.

> /*
> * Likewise, copy the relids that are represented by this foreign
> scan. An
> - * upper rel doesn't have relids set, but it covers all the base
> relations
> - * participating in the underlying scan, so use root's all_baserels.
> + * upper rel (but not the other upper rel) doesn't have relids set,
> but it
> + * covers all the base relations participating in the underlying
> scan, so
> + * use root's all_baserels.
> */
>
> This is correct only for "other" grouping relations. We are yet to
> decide what to do
> for the other upper relations.
>

I have removed this comment change as existing comments look good after
doing following changes:

>
> - if (IS_UPPER_REL(rel))
> + if (IS_UPPER_REL(rel) && !IS_OTHER_REL(rel))
> I guess, this condition boils down to rel->reloptkind == RELOPT_UPPER_REL.
> Use
> it that way?
>

Done.

Attached 0002 for this.

Thanks

>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>

--
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company