Re: [HACKERS] Partition-wise aggregation/grouping

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.

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.

- 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?

/*
+ /*
+ * 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.

/* Grouping information */
List *grouped_tlist;
+ PathTarget *grouped_target;

We should use the target stored in the grouped rel directly.

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

/*
* 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.

- 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?

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