Horton

Re: [HACKERS] Partition-wise aggregation/grouping

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>

View thread View on postgr.es
From: Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>
To: Jeevan Chalke <jeevan.chalke@enterprisedb.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, David Rowley <david.rowley@2ndquadrant.com>, Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2018-03-26T11:54:13Z
Lists: pgsql-hackers 
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

Commits

  1. postgres_fdw: Push down partition-wise aggregation.

  2. Remove 'target' from GroupPathExtraData.

  3. Implement partition-wise grouping/aggregation.

  4. Don't pass the grouping target around unnecessarily.

  5. Determine grouping strategies in create_grouping_paths.

  6. Defer creation of partially-grouped relation until it's needed.

  7. Split create_grouping_paths into degenerate and non-degenerate cases.

  8. Pass additional arguments to a couple of grouping-related functions.

  9. Fix logic error in add_paths_to_partial_grouping_rel.

  10. Minor cleanup of code related to partially_grouped_rel.

  11. Add a new upper planner relation for partially-aggregated results.

  12. Charge cpu_tuple_cost * 0.5 for Append and MergeAppend nodes.

  13. Rename enable_partition_wise_join to enable_partitionwise_join

  14. Factor some code out of create_grouping_paths.

  15. Pad XLogReaderState's main_data buffer more aggressively.

  16. Prevent int128 from requiring more than MAXALIGN alignment.

  17. Fix DROP SUBSCRIPTION hang

  18. Inject $(ICU_LIBS) regardless of platform.

  19. Some preliminary refactoring towards partitionwise join.