Re: [HACKERS] Partition-wise aggregation/grouping

On Fri, Mar 23, 2018 at 4:35 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.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