Thread: [HACKERS] Partition-wise aggregation/grouping

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> Declarative partitioning is supported in PostgreSQL 10 and work is already in
> progress to support partition-wise joins. Here is a proposal for partition-wise
> aggregation/grouping. Our initial performance measurement has shown 7 times
> performance when partitions are on foreign servers and approximately 15% when
> partitions are local.
>
> Partition-wise aggregation/grouping computes aggregates for each partition
> separately. If the group clause contains the partition key, all the rows
> belonging to a given group come from one partition, thus allowing aggregates
> to be computed completely for each partition. Otherwise, partial aggregates
> computed for each partition are combined across the partitions to produce the
> final aggregates. This technique improves performance because:

> i. When partitions are located on foreign server, we can push down the
> aggregate to the foreign server.

> ii. If hash table for each partition fits in memory, but that for the whole
> relation does not, each partition-wise aggregate can use an in-memory hash
> table.

> iii. Aggregation at the level of partitions can exploit properties of
> partitions like indexes, their storage etc.

I suspect this overlaps with

https://www.postgresql.org/message-id/29111.1483984605%40localhost

I'm working on the next version of the patch, which will be able to aggregate
the result of both base relation scans and joins. I'm trying hard to make the
next version available before an urgent vacation that I'll have to take at
random date between today and early April. I suggest that we coordinate the
effort, it's lot of work in any case.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

The promised new version of my patch is here:

https://www.postgresql.org/message-id/9666.1491295317%40localhost

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> On Tue, Mar 21, 2017 at 1:47 PM, Antonin Houska <ah@cybertec.at> wrote:
>
>  Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
>
> IIUC, it seems that you are trying to push down the aggregation into the
> joining relations. So basically you are converting
> Agg -> Join -> {scan1, scan2} into
> FinalAgg -> Join -> {PartialAgg -> scan1, PartialAgg -> scan2}.
> In addition to that your patch pushes aggregates on base rel to its children,
> if any.
>
> Where as what I propose here is pushing down aggregation below the append
> node keeping join/scan as is. So basically I am converting
> Agg -> Append-> Join -> {scan1, scan2} into
> Append -> Agg -> Join -> {scan1, scan2}.
> This will require partition-wise join as posted in [1].
> But I am planning to make this work for partitioned relations and not for
> generic inheritance.
>
> I treat these two as separate strategies/paths to be consider while planning.
>
> Our work will overlap when we are pushing down the aggregate on partitioned
> base relation to its children/partitions.
>
> I think you should continue working on pushing down aggregate onto the
> joins/scans where as I will continue my work on pushing down aggregates to
> partitions (joins as well as single table). Once we are done with these task,
> then we may need to find a way to integrate them.
>
> [1]
https://www.postgresql.org/message-id/flat/CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com#CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com

My patch does also create (partial) aggregation paths below the Append node,
but only expects SeqScan as input. Please check if you patch can be based on
this or if there's any conflict.

(I'll probably be unable to respond before Monday 04/17.)

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

Antonin Houska <ah@cybertec.at> wrote:
>
> Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
>
> > Our work will overlap when we are pushing down the aggregate on partitioned
> > base relation to its children/partitions.
> >
> > I think you should continue working on pushing down aggregate onto the
> > joins/scans where as I will continue my work on pushing down aggregates to
> > partitions (joins as well as single table). Once we are done with these task,
> > then we may need to find a way to integrate them.
> >
> > [1]
https://www.postgresql.org/message-id/flat/CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com#CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com
>
> My patch does also create (partial) aggregation paths below the Append node,
> but only expects SeqScan as input. Please check if you patch can be based on
> this or if there's any conflict.

Well, I haven't imposed any explicit restriction on the kind of path to be
aggregated below the Append path. Maybe the only thing to do is to merge my
patch with the "partition-wise join" patch (which I haven't checked yet).

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

Antonin Houska <ah@cybertec.at> wrote:

> Antonin Houska <ah@cybertec.at> wrote:
> >
> > Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:
> >
> > > Our work will overlap when we are pushing down the aggregate on partitioned
> > > base relation to its children/partitions.
> > >
> > > I think you should continue working on pushing down aggregate onto the
> > > joins/scans where as I will continue my work on pushing down aggregates to
> > > partitions (joins as well as single table). Once we are done with these task,
> > > then we may need to find a way to integrate them.
> > >
> > > [1]
https://www.postgresql.org/message-id/flat/CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com#CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com
> >
> > My patch does also create (partial) aggregation paths below the Append node,
> > but only expects SeqScan as input. Please check if you patch can be based on
> > this or if there's any conflict.
>
> Well, I haven't imposed any explicit restriction on the kind of path to be
> aggregated below the Append path. Maybe the only thing to do is to merge my
> patch with the "partition-wise join" patch (which I haven't checked yet).

Attached is a diff that contains both patches merged. This is just to prove my
assumption, details to be elaborated later. The scripts attached produce the
following plan in my environment:

                   QUERY PLAN
------------------------------------------------
 Parallel Finalize HashAggregate
   Group Key: b_1.j
   ->  Append
         ->  Parallel Partial HashAggregate
               Group Key: b_1.j
               ->  Hash Join
                     Hash Cond: (b_1.j = c_1.k)
                     ->  Seq Scan on b_1
                     ->  Hash
                           ->  Seq Scan on c_1
         ->  Parallel Partial HashAggregate
               Group Key: b_2.j
               ->  Hash Join
                     Hash Cond: (b_2.j = c_2.k)
                     ->  Seq Scan on b_2
                     ->  Hash
                           ->  Seq Scan on c_2


Note that I had no better idea how to enforce the plan than hard-wiring zero
costs of the partial aggregation paths. This simulates the use case of partial
aggregation performed on remote node (postgres_fdw). Other use cases may
exist, but I only wanted to prove the concept in terms of coding so far.

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Apr 26, 2017 at 6:28 AM, Antonin Houska <ah@cybertec.at> wrote:
> Attached is a diff that contains both patches merged. This is just to prove my
> assumption, details to be elaborated later. The scripts attached produce the
> following plan in my environment:
>
>                    QUERY PLAN
> ------------------------------------------------
>  Parallel Finalize HashAggregate
>    Group Key: b_1.j
>    ->  Append
>          ->  Parallel Partial HashAggregate
>                Group Key: b_1.j
>                ->  Hash Join
>                      Hash Cond: (b_1.j = c_1.k)
>                      ->  Seq Scan on b_1
>                      ->  Hash
>                            ->  Seq Scan on c_1
>          ->  Parallel Partial HashAggregate
>                Group Key: b_2.j
>                ->  Hash Join
>                      Hash Cond: (b_2.j = c_2.k)
>                      ->  Seq Scan on b_2
>                      ->  Hash
>                            ->  Seq Scan on c_2

Well, I'm confused.  I see that there's a relationship between what
Antonin is trying to do and what Jeevan is trying to do, but I can't
figure out whether one is a subset of the other, whether they're both
orthogonal, or something else.  This plan looks similar to what I
would expect Jeevan's patch to produce, except i have no idea what
"Parallel" would mean in a plan that contains no Gather node.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:

> On Wed, Apr 26, 2017 at 6:28 AM, Antonin Houska <ah@cybertec.at> wrote:
> > Attached is a diff that contains both patches merged. This is just to prove my
> > assumption, details to be elaborated later. The scripts attached produce the
> > following plan in my environment:
> >
> >                    QUERY PLAN
> > ------------------------------------------------
> >  Parallel Finalize HashAggregate
> >    Group Key: b_1.j
> >    ->  Append
> >          ->  Parallel Partial HashAggregate
> >                Group Key: b_1.j
> >                ->  Hash Join
> >                      Hash Cond: (b_1.j = c_1.k)
> >                      ->  Seq Scan on b_1
> >                      ->  Hash
> >                            ->  Seq Scan on c_1
> >          ->  Parallel Partial HashAggregate
> >                Group Key: b_2.j
> >                ->  Hash Join
> >                      Hash Cond: (b_2.j = c_2.k)
> >                      ->  Seq Scan on b_2
> >                      ->  Hash
> >                            ->  Seq Scan on c_2
>
> Well, I'm confused.  I see that there's a relationship between what
> Antonin is trying to do and what Jeevan is trying to do, but I can't
> figure out whether one is a subset of the other, whether they're both
> orthogonal, or something else.  This plan looks similar to what I
> would expect Jeevan's patch to produce,

The point is that the patch Jeevan wanted to work on is actually a subset of
[1] combined with [2].

> except i have no idea what "Parallel" would mean in a plan that contains no
> Gather node.

parallel_aware field was set mistakenly on the AggPath. Fixed patch is
attached below, producing this plan:

                   QUERY PLAN
------------------------------------------------
 Finalize HashAggregate
   Group Key: b_1.j
   ->  Append
         ->  Partial HashAggregate
               Group Key: b_1.j
               ->  Hash Join
                     Hash Cond: (b_1.j = c_1.k)
                     ->  Seq Scan on b_1
                     ->  Hash
                           ->  Seq Scan on c_1
         ->  Partial HashAggregate
               Group Key: b_2.j
               ->  Hash Join
                     Hash Cond: (b_2.j = c_2.k)
                     ->  Seq Scan on b_2
                     ->  Hash
                           ->  Seq Scan on c_2

[1] https://www.postgresql.org/message-id/9666.1491295317%40localhost

[2] https://commitfest.postgresql.org/14/994/

--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Jeevan Chalke <jeevan.chalke@enterprisedb.com> wrote:

> On Thu, Apr 27, 2017 at 4:53 PM, Antonin Houska <ah@cybertec.at> wrote:
>
> > Robert Haas <robertmhaas@gmail.com> wrote:
> > > Well, I'm confused. I see that there's a relationship between what
> > > Antonin is trying to do and what Jeevan is trying to do, but I can't
> > > figure out whether one is a subset of the other, whether they're both
> > > orthogonal, or something else. This plan looks similar to what I
> > > would expect Jeevan's patch to produce,

> >  The point is that the patch Jeevan wanted to work on is actually a subset of
> >  [1] combined with [2].

> Seems like, as you are targeting every relation whether or not it is
> partitioned.

Yes.

> With my patch, I am getting following plan where we push entire
> aggregation below append.
>
> QUERY PLAN
> ------------------------------------------
> Append
> -> HashAggregate
> Group Key: b_1.j
> -> Hash Join
> Hash Cond: (b_1.j = c_1.k)
> -> Seq Scan on b_1
> -> Hash
> -> Seq Scan on c_1
> -> HashAggregate
> Group Key: b_2.j
> -> Hash Join
> Hash Cond: (b_2.j = c_2.k)
> -> Seq Scan on b_2
> -> Hash
> -> Seq Scan on c_2
> (15 rows)

I think this is not generic enough because the result of the Append plan can
be joined to another relation. As such a join can duplicate the
already-aggregated values, the aggregates should not be finalized below the
top-level plan.

> Antonin, I have tried applying your patch on master but it doesn't get
> apply. Can you please provide the HEAD and any other changes required
> to be applied first?

I've lost that information. I'll post a new version to the [1] thread asap.

> How the plan look like when GROUP BY key does not match with the
> partitioning key i.e. GROUP BY b.v ?

EXPLAIN (COSTS false)
SELECT          b.v, avg(b.v + c.v)
FROM            b               JOIN               c ON b.j = c.k
GROUP BY        b.v;
                  QUERY PLAN
------------------------------------------------Finalize HashAggregate  Group Key: b_1.v  ->  Append        ->  Partial
HashAggregate             Group Key: b_1.v              ->  Hash Join                    Hash Cond: (b_1.j = c_1.k)
              ->  Seq Scan on b_1                    ->  Hash                          ->  Seq Scan on c_1        ->
PartialHashAggregate              Group Key: b_2.v              ->  Hash Join                    Hash Cond: (b_2.j =
c_2.k)                   ->  Seq Scan on b_2                    ->  Hash                          ->  Seq Scan on c_2 


> >  [1] https://www.postgresql.org/message-id/9666.1491295317%40localhost
> >
> >  [2] https://commitfest.postgresql.org/14/994/


--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

On Fri, Apr 28, 2017 at 3:03 AM, Antonin Houska <ah@cybertec.at> wrote:
> I think this is not generic enough because the result of the Append plan can
> be joined to another relation. As such a join can duplicate the
> already-aggregated values, the aggregates should not be finalized below the
> top-level plan.

If the grouping key matches the partition key, then it's correct to
push the entire aggregate down, and there's probably a large
performance advantage from avoiding aggregating twice.  If the two
don't match, then pushing the aggregate down necessarily involves a
"partial" and a "finalize" stage, which may or may not be cheaper than
doing the aggregation all at once.  If you have lots of 2-row groups
with 1 row in the first branch of the append and 1 row in the second
branch of the append, breaking the aggregate into two steps is
probably going to be a loser.  If the overall number of groups is
small, it's probably going to win.  But when the grouping key matches
the partition key, so that two-stage aggregation isn't required, I
suspect the pushdown should almost always win.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi Jeevan,
I have started reviewing these patches.

0001 looks fine. There might be some changes that will be needed, but
those will be clear when I review the patch that uses this
refactoring.

0002
+ *
+ * If targetlist is provided, we use it else use targetlist from the root. */static
doubleget_number_of_groups(PlannerInfo*root,                    double path_rows,
 
-                    grouping_sets_data *gd)
+                    grouping_sets_data *gd,
+                    List *tlist){   Query      *parse = root->parse;   double      dNumGroups;
+   List       *targetList = (tlist == NIL) ? parse->targetList : tlist;

May be we should just pass targetlist always. Instead of passing NIL,
pass parse->targetList directly. That would save us one conditional
assignment. May be passing NIL is required for the patches that use
this refactoring, but that's not clear as is in this patch.

0003
In the documenation of enable_partition_wise_aggregate, we should
probably explain why the default is off or like partition_wise_join
GUC, explain the consequences of turning it off. I doubt if we could
accept something like partition_wise_agg_cost_factor looks. But we can
discuss this at a later stage. Mean time it may be worthwhile to fix
the reason why we would require this GUC. If the regular aggregation
has cost lesser than partition-wise aggregation in most of the cases,
then probably we need to fix the cost model.

I will continue reviewing rest of the patches.

On Mon, Sep 18, 2017 at 12:37 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> On Tue, Sep 12, 2017 at 6:21 PM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
>>
>>
>>
>> On Tue, Sep 12, 2017 at 3:24 PM, Rajkumar Raghuwanshi
>> <rajkumar.raghuwanshi@enterprisedb.com> wrote:
>>>
>>>
>>> Hi Jeevan,
>>>
>>> I have started testing partition-wise-aggregate and got one observation,
>>> please take a look.
>>> with the v2 patch, here if I change target list order, query is not
>>> picking full partition-wise-aggregate.
>>
>>
>> Thanks Rajkumar for reporting this.
>>
>> I am looking into this issue and will post updated patch with the fix.
>
>
> Logic for checking whether partition keys lead group by keys needs to be
> updated here. The group by expressions can appear in any order without
> affecting the final result. And thus, the need for partition keys should
> be leading the group by keys to have full aggregation is not mandatory.
> Instead we must ensure that the partition keys are part of the group by
> keys to compute full aggregation on a partition.
>
> Attached, revised patch-set with above fix.
>
> Also, in test-cases, I have removed DROP/ANALYZE commands on child
> relations and also removed VERBOSE from the EXPLAIN.
>
> Notes:
> HEAD: 8edacab209957520423770851351ab4013cb0167
> Partition-wise Join patch-set version: v32
>
> Thanks
>
> --
> Jeevan Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Sep 27, 2017 at 3:42 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Thanks Ashutosh for reviewing.
>
> Attached new patch-set with following changes:
>
> 1. Removed earlier 0007 and 0008 patches which were PoC for supporting
> partial aggregation over fdw. I removed them as it will be a different
> issue altogether and hence I will tackle them separately once this is
> done.
>
> This patch-set now includes support for parallel plans within partitions.
>
> Notes:
> HEAD: 59597e6
> Partition-wise Join Version: 34
>
> (First six patches 0001 - 0006, remains the same functionality-wise)
> 0007 - Refactors partial grouping paths creation into the separate function.
> 0008 - Enables parallelism within the partition-wise aggregation.
>
> This patch also includes a fix for the crash reported by Rajkumar.
> While forcibly applying scan/join target to all the Paths for the scan/join
> rel, earlier I was using apply_projection_to_path() which modifies the path
> in-place which causing this crash as the path finally chosen has been
> updated by partition-wise agg path creation. Now I have used
> create_projection_path() like we do in partial aggregation paths.
>
> Also, fixed issues reported by Ashutosh.

Thanks.

Here are comments on 0004 from last patch set. But most the comments
still apply.

Patch 0001 adds functions create_hash_agg_path() and create_sort_agg_path().
Patch 0004 adds a new argument to those functions for conditions in HAVING
clause. We should move those changes to 0001 and pass parse->havingQual to
these functions in 0001 itself. That will keep all changes to those functions
together and also make 0003 small.

The prologue of try_partition_wise_grouping() mentions a restriction of
partition keys being leading group by clauses. This restriction is not
specified in the prologue of have_grouping_by_partkey(), which actually checks
for this restriction. The requirement per prologue of that function is just to
have partition keys in group clause. I think have_grouping_by_partkey() is
correct, and we should change prologue of try_partition_wise_grouping() to be
in sync with have_grouping_by_partkey(). The prologue explains why
partition-wise aggregation/grouping would be efficient with this restriction,
but it doesn't explain why partial aggregation/grouping per partition would be
efficient. May be it should do that as well. Similar is the case with partial
aggregation/grouping discussion in README.

+    /* Do not handle grouping sets for now. */
Is this a real restriction or just restriction for first cut of this feature?
Can you please add more explanation? May be update README as well?

+    grouped_rel->part_scheme = input_rel->part_scheme;
Is this true even for partial aggregates? I think not. Since group by clause
does not contain partition keys, the rows from multiple partitions participate
in one group and thus the partition keys of input relation do not apply to the
grouped relation. In this case, it seems that the grouped rel will have
part_rels but will not be partitioned.

+        /*
+         * If there is no path for the child relation, then we cannot add
+         * aggregate path too.
+         */
+        if (input_child_rel->pathlist == NIL)
+            return;
When can this happen? I think, similar to partition-wise join it happens when
there is a dummy parent relation. See [1]. If that's the case, you may want to
do things similar to what partition-wise join is doing. If there's some other
reason for doing this, returing from here half-way is actually waste of memory
and planning time. Instead, we may want to loop over the part_rels to find if
any of those have empty pathlist and return from there before doing any actual
work.

+        extra.pathTarget = child_target;
+        extra.inputRows = input_child_rel->cheapest_startup_path->rows;
+        extra.havingQual = (Node *) adjust_appendrel_attrs(root,
+                                                           (Node *)
query->havingQual,
+                                                           nappinfos,
+                                                           appinfos);
These lines are updating some fields of "extra" structure in every loop. The
structure is passed to create_child_grouping_paths() in the loop and to
add_paths_to_append_rel() outside the loop. Thus add_paths_to_append_rel() only
gets some member values for the last child. Is that right? Should we split
extra into two structures one to be used within the loop and one outside? Or
may be send the members being updated within the loop separately?

+        /*
+         * Forcibly apply scan/join target to all the Paths for the scan/join
+         * rel.
+         *
[ lines clipped ]
+                if (subpath == input_child_rel->cheapest_total_path)
+                    input_child_rel->cheapest_total_path = path;
+            }
+        }
This code seems to be copied from grouping_planner() almost verbatim. Is there
a way we can refactor it into a function and use it in both the places.

have_grouping_by_partkey() may use match_expr_to_partition_keys() to find
whether a given clause expression matches any of the partition keys. Or you
could use list_intersection() instead of following loop
+        foreach(lc, partexprs)
+        {
+            Expr       *partexpr = lfirst(lc);
+
+            if (list_member(groupexprs, partexpr))
+            {
+                found = true;
+                break;
+            }
+        }
+        /*
+         * If none of the partition key matches with any of the GROUP BY
+         * expression, return false.
+         */
+        if (!found)
+            return false;

create_child_grouping_paths() and create_grouping_paths() has almost similar
code. Is there a way we could refactor the code to extract common code into a
function called by these two functions or reuse create_grouping_paths() for
children as well? I don't think we will be able to do the later.

+    /* Nothing to do if there is an empty pathlist */
+    if (grouped_rel->pathlist == NIL)
+        return false;
When would that happen? Similar condition in case of parent grouped rel throws
an error, so when this code is called, we know for sure that parent had
non-empty pathlist. So, we would expect child to have non-empty pathlist as
well.

+        grouped_rel = fetch_upper_rel(root, UPPERREL_GROUP_AGG,
+                                      input_rel->relids);
+
+        /* Mark this rel as "other upper rel" */
+        grouped_rel->reloptkind = RELOPT_OTHER_UPPER_REL;
I think we need to pass relopkind as an argument to fetch_upper_rel(), now that
we have "upper" relations and "other upper" relations. relids will still be a
"key" to find an upper relation but its reloptkind should match the given
reloptkind.  fetch_upper_rel() is used to create the upper relation if it
doesn't exist. So, with the above code, if some other function calls
fetch_upper_rel() with given relids, it would get an upper rel with
RELOPT_UPPER_REL and then this code would change it to RELOPT_OTHER_UPPER_REL.
That looks odd. May be we should have written build_upper_rel() and
find_upper_rel() similar to build_join_rel() and find_join_rel() instead of
combining both the functionalities in one function.

+        /*
+         * create_append_path() sets the path target from the given relation.
+         * However, in this case grouped_rel doesn't have a target set.  So we
+         * must set the pathtarget to the passed in target.
+         */
+        apath->pathtarget = target;
I think, we should change create_append_path() functions to accept target as an
additional argument. For append rels other than aggregate and grouping, target
will be same as relation's target. For agg/group append rels, we will pass
different targets for partial and non-partial grouping paths.

+        /*
+         * Since Append's output is always unsorted, we'll need to sort,
+         * unless there's no GROUP BY clause or a degenerate (constant) one,
+         * in which case there will only be a single group.
+         */
append path here can be output of either merge append or append. If it's output
of merge append, we don't need to sort it again, do we?

create_partition_agg_paths() creates append paths and then adds finalization
path if necessary. The code to add finalization path seems to be similar to the
code that adds finalization path for parallel query. May be we could take out
common code into a function and call that function in two places. I see this
function as accepting a partial aggregation/grouping path and returning a path
that finalizes partial aggregates/groups.

[1] https://www.postgresql.org/message-id/CAFjFpRd5+zroxY7UMGTR2M=rjBV4aBOCxQg3+1rBmTPLK5mpDg@mail.gmail.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10 October 2017 at 01:10, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Attached new patch set having HEAD at 84ad4b0 with all these review points
> fixed. Let me know if I missed any thanks.

I've only really skimmed over this thread and only opened the code
enough to extract the following:

+ /* Multiply the costs by partition_wise_agg_cost_factor. */
+ apath->startup_cost *= partition_wise_agg_cost_factor;
+ apath->total_cost *= partition_wise_agg_cost_factor;

I've not studied how all the path plumbing is done, but I think
instead of doing this costing magic we should really stop pretending
that Append/MergeAppend nodes are cost-free. I think something like
charging cpu_tuple_cost per row expected through Append/MergeAppend
would be a better approach to this.

If you perform grouping or partial grouping before the Append, then in
most cases the Append will receive less rows, so come out cheaper than
if you perform the grouping after it. I've not learned the
partition-wise join code enough to know if this is going to affect
that too, but for everything else, there should be no plan change,
since there's normally no alternative paths. I see there's even a
comment in create_append_path() which claims the zero cost is a bit
optimistic.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 10, 2017 at 3:15 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> On 10 October 2017 at 01:10, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
>> Attached new patch set having HEAD at 84ad4b0 with all these review points
>> fixed. Let me know if I missed any thanks.
>
> I've only really skimmed over this thread and only opened the code
> enough to extract the following:
>
> + /* Multiply the costs by partition_wise_agg_cost_factor. */
> + apath->startup_cost *= partition_wise_agg_cost_factor;
> + apath->total_cost *= partition_wise_agg_cost_factor;
>
> I've not studied how all the path plumbing is done, but I think
> instead of doing this costing magic we should really stop pretending
> that Append/MergeAppend nodes are cost-free. I think something like
> charging cpu_tuple_cost per row expected through Append/MergeAppend
> would be a better approach to this.
>
> If you perform grouping or partial grouping before the Append, then in
> most cases the Append will receive less rows, so come out cheaper than
> if you perform the grouping after it. I've not learned the
> partition-wise join code enough to know if this is going to affect
> that too, but for everything else, there should be no plan change,
> since there's normally no alternative paths. I see there's even a
> comment in create_append_path() which claims the zero cost is a bit
> optimistic.
>

+1. Partition-wise join will also benefit from costing Append
processing. Number of rows * width of join result compared with the
sum of that measure for joining relations decides whether Append node
processes more data in Append->Join case than Join->Append case.

Append node just returns the result of ExecProcNode(). Charging
cpu_tuple_cost may make it too expensive. In other places where we
charge cpu_tuple_cost there's some processing done to the tuple like
ExecStoreTuple() in SeqNext(). May be we need some other measure for
Append's processing of the tuple.

May be we should try to measure the actual time spent in Append node
as a fraction of say time spent in child seq scans. That might give us
a clue as to how Append processing can be charged in terms of costing.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10 October 2017 at 17:57, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Append node just returns the result of ExecProcNode(). Charging
> cpu_tuple_cost may make it too expensive. In other places where we
> charge cpu_tuple_cost there's some processing done to the tuple like
> ExecStoreTuple() in SeqNext(). May be we need some other measure for
> Append's processing of the tuple.

I don't think there's any need to invent any new GUC. You could just
divide cpu_tuple_cost by something.

I did a quick benchmark on my laptop to see how much Append really
costs, and with the standard costs the actual cost seems to be about
cpu_tuple_cost / 2.4. So probably cpu_tuple_cost / 2 might be
realistic. create_set_projection_path() does something similar and
brincostestimate() does some similar magic and applies 0.1 *
cpu_operator_cost to the total cost.

# create table p (a int, b int);
# create table p1 () inherits (p);
# insert into p1 select generate_series(1,1000000);
# vacuum analyze p1;
# \q
$ echo "select count(*) from p1;" > p1.sql
$ echo "select count(*) from p;" > p.sql
$ pgbench -T 60 -f p1.sql -n

latency average = 58.567 ms

$ pgbench -T 60 -f p.sql -n
latency average = 72.984 ms

$ psql
psql (11devel)
Type "help" for help.

# -- check the cost of the plan.
# explain select count(*) from p1;                           QUERY PLAN
------------------------------------------------------------------Aggregate  (cost=16925.00..16925.01 rows=1 width=8)
-> Seq Scan on p1  (cost=0.00..14425.00 rows=1000000 width=0)
 
(2 rows)

# -- selecting from the parent is the same due to zero Append cost.
# explain select count(*) from p;                              QUERY PLAN
------------------------------------------------------------------------Aggregate  (cost=16925.00..16925.01 rows=1
width=8) ->  Append  (cost=0.00..14425.00 rows=1000001 width=0)        ->  Seq Scan on p  (cost=0.00..0.00 rows=1
width=0)       ->  Seq Scan on p1  (cost=0.00..14425.00 rows=1000000 width=0)
 
(4 rows)

# -- extrapolate the additional time taken for the Append scan and
work out what the planner
# -- should add to the plan's cost, then divide by the number of rows
in p1 to work out the
# -- tuple cost of pulling a row through the append.
# select (16925.01 * (72.984 / 58.567) - 16925.01)  / 1000000;       ?column?
------------------------0.00416630302337493743
(1 row)

# show cpu_tuple_cost;cpu_tuple_cost
----------------0.01
(1 row)

# -- How does that compare to the cpu_tuple_cost?
# select current_Setting('cpu_tuple_cost')::float8 / 0.00416630302337493743;   ?column?
----------------2.400209476818
(1 row)

Maybe it's worth trying with different row counts to see if the
additional cost is consistent, but it's probably not worth being too
critical here.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 10, 2017 at 1:31 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
>
> I don't think there's any need to invent any new GUC. You could just
> divide cpu_tuple_cost by something.
>
> I did a quick benchmark on my laptop to see how much Append really
> costs, and with the standard costs the actual cost seems to be about
> cpu_tuple_cost / 2.4. So probably cpu_tuple_cost / 2 might be
> realistic. create_set_projection_path() does something similar and
> brincostestimate() does some similar magic and applies 0.1 *
> cpu_operator_cost to the total cost.
>
>
> # -- How does that compare to the cpu_tuple_cost?
> # select current_Setting('cpu_tuple_cost')::float8 / 0.00416630302337493743;
>     ?column?
> ----------------
>  2.400209476818
> (1 row)
>
> Maybe it's worth trying with different row counts to see if the
> additional cost is consistent, but it's probably not worth being too
> critical here.
>

This looks good to me. I think it should be a separate, yet very small patch.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 10, 2017 at 6:00 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> This looks good to me. I think it should be a separate, yet very small patch.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 13 October 2017 at 19:36, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> I have tried exactly same tests to get to this factor on my local developer
> machine. And with parallelism enabled I got this number as 7.9. However, if
> I disable the parallelism (and I believe David too disabled that), I get
> this number as 1.8. Whereas for 10000 rows, I get this number to 1.7
>
> -- With Gather
> # select current_Setting('cpu_tuple_cost')::float8 / ((10633.56 * (81.035 /
> 72.450) - 10633.56)  / 1000000);
> 7.9
>
> -- Without Gather
> # select current_Setting('cpu_tuple_cost')::float8 / ((16925.01 * (172.838 /
> 131.400) - 16925.01)  / 1000000);
> 1.8
>
> -- With 10000 rows (so no Gather too)
> # select current_Setting('cpu_tuple_cost')::float8 / ((170.01 * (1.919 /
> 1.424) - 170.01)  / 10000);
> 1.7
>
> So it is not so straight forward to come up the correct heuristic here. Thus
> using 50% of cpu_tuple_cost look good to me here.
>
> As suggested by Ashutosh and Robert, attached separate small WIP patch for
> it.

Good to see it stays fairly consistent at different tuple counts, and
is not too far away from what I got on this machine.

I looked over the patch and saw this:

@@ -1800,6 +1827,9 @@ cost_merge_append(Path *path, PlannerInfo *root, */ run_cost += cpu_operator_cost * tuples;

+ /* Add MergeAppend node overhead like we do it for the Append node */
+ run_cost += cpu_tuple_cost * DEFAULT_APPEND_COST_FACTOR * tuples;
+ path->startup_cost = startup_cost + input_startup_cost; path->total_cost = startup_cost + run_cost +
input_total_cost;}

You're doing that right after a comment that says we don't do that. It
also does look like the "run_cost += cpu_operator_cost * tuples" is
trying to do the same thing, so perhaps it's worth just replacing
that, which by default will double that additional cost, although
doing so would have the planner slightly prefer a MergeAppend to an
Append than previously.

+#define DEFAULT_APPEND_COST_FACTOR 0.5

I don't really think the DEFAULT_APPEND_COST_FACTOR adds much. it
means very little by itself. It also seems that most of the other cost
functions just use the magic number.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 13, 2017 at 12:06 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
While playing around with the patch I have noticed one regression with
the partial partition-wise aggregate.

I am consistently able to reproduce this on my local machine.

Scenario: Group by on non-key column and only one tuple per group.

Complete Test:
--------------------
create table t(a int,b int) partition by range(a);
create table t1 partition of t for values from (1) to (100000);
create table t2 partition of t for values from (100000) to (200000);

insert into t values (generate_series(1,199999),generate_series(1, 199999));
postgres=# explain analyze select sum(a) from t group by b;
QUERY
PLAN

--------------------------------------------------------------------------------------------------------------------------------------Finalize
GroupAggregate (cost=20379.55..28379.51 rows=199999
 
width=12) (actual time=102.311..322.969 rows=199999 loops=1)  Group Key: t1.b  ->  Merge Append
(cost=20379.55..25379.53rows=199999 width=12)
 
(actual time=102.303..232.310 rows=199999 loops=1)        Sort Key: t1.b        ->  Partial GroupAggregate
(cost=10189.72..11939.70
rows=99999 width=12) (actual time=52.164..108.967 rows=99999 loops=1)              Group Key: t1.b              ->
Sort (cost=10189.72..10439.72 rows=99999 width=8)
 
(actual time=52.158..66.236 rows=99999 loops=1)                    Sort Key: t1.b                    Sort Method:
externalmerge  Disk: 1768kB                    ->  Seq Scan on t1  (cost=0.00..1884.99
 
rows=99999 width=8) (actual time=0.860..20.388 rows=99999 loops=1)        ->  Partial GroupAggregate
(cost=10189.82..11939.82
rows=100000 width=12) (actual time=50.134..102.976 rows=100000
loops=1)              Group Key: t2.b              ->  Sort  (cost=10189.82..10439.82 rows=100000 width=8)
(actual time=50.128..63.362 rows=100000 loops=1)                    Sort Key: t2.b                    Sort Method:
externalmerge  Disk: 1768kB                    ->  Seq Scan on t2  (cost=0.00..1885.00
 
rows=100000 width=8) (actual time=0.498..20.977 rows=100000 loops=1)Planning time: 0.190 msExecution time: 339.929 ms
(18 rows)

postgres=# set enable_partition_wise_agg=off;
SET
postgres=# explain analyze select sum(a) from t group by b;
QUERYPLAN
 

----------------------------------------------------------------------------------------------------------------------------------GroupAggregate
(cost=26116.53..29616.51 rows=199999 width=12)
 
(actual time=139.413..250.751 rows=199999 loops=1)  Group Key: t1.b  ->  Sort  (cost=26116.53..26616.52 rows=199999
width=8)(actual
 
time=139.406..168.775 rows=199999 loops=1)        Sort Key: t1.b        Sort Method: external merge  Disk: 3544kB
->  Result  (cost=0.00..5769.98 rows=199999 width=8) (actual
 
time=0.674..76.392 rows=199999 loops=1)              ->  Append  (cost=0.00..3769.99 rows=199999 width=8)
(actual time=0.672..40.291 rows=199999 loops=1)                    ->  Seq Scan on t1  (cost=0.00..1884.99
rows=99999 width=8) (actual time=0.672..12.408 rows=99999 loops=1)                    ->  Seq Scan on t2
(cost=0.00..1885.00
rows=100000 width=8) (actual time=1.407..11.689 rows=100000 loops=1)Planning time: 0.146 msExecution time: 263.678 ms
(11 rows)


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 17, 2017 at 10:44 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>

> I didn't get what you mean by regression here. Can you please explain?
>
> I see that PWA plan is selected over regular plan when enabled on the basis
> of costing.
> Regular planning need a Result node due to which costing increases where as
> PWA don't need that and thus wins.

Sorry for not clearly explaining,  I meant that with normal plan
execution time is 263.678 ms whereas with PWA its 339.929 ms.

I only set enable_partition_wise_agg=on and it switched to PWA and
execution time increased by 30%.
I understand that the this is the worst case for PWA where
FinalizeAggregate is getting all the tuple.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Oct 27, 2017 at 1:01 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> 1. Added separate patch for costing Append node as discussed up-front in the
> patch-set.
> 2. Since we now cost Append node, we don't need
> partition_wise_agg_cost_factor
> GUC. So removed that. The remaining patch hence merged into main
> implementation
> patch.
> 3. Updated rows in test-cases so that we will get partition-wise plans.

With 0006 applied, cost_merge_append() is now a little bit confused:
   /*    * Also charge a small amount (arbitrarily set equal to operator cost) per    * extracted tuple.  We don't
chargecpu_tuple_cost because a MergeAppend    * node doesn't do qual-checking or projection, so it has less overhead
*than most plan nodes.    */   run_cost += cpu_operator_cost * tuples;
 
   /* Add MergeAppend node overhead like we do it for the Append node */   run_cost += cpu_tuple_cost *
DEFAULT_APPEND_COST_FACTOR* tuples;
 

The first comment says that we don't add cpu_tuple_cost, and the
second one then adds half of it anyway.

I think it's fine to have a #define for DEFAULT_APPEND_COST_FACTOR,
because as you say it's used twice, but I don't think that should be
exposed in cost.h; I'd make it private to costsize.c and rename it to
something like APPEND_CPU_COST_MULTIPLIER.  The word DEFAULT, in
particular, seems useless to me, since there's no provision for it to
be overridden by a different value.

What testing, if any, can we think about doing with this plan to make
sure it doesn't regress things?  For example, if we do a TPC-H run
with partitioned tables and partition-wise join enabled, will any
plans change with this patch?  Do they get faster or not?  Anyone have
other ideas for what to test?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, Nov 1, 2017 at 6:20 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Yep.
> But as David reported earlier, if we remove the first part i.e. adding
> cpu_operator_cost per tuple, Merge Append will be preferred over an Append
> node unlike before. And thus, I thought of better having both, but no so
> sure. Should we remove that part altogether, or add both in a single
> statement with updated comments?

I was only suggesting that you update the comments.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 10/27/2017 02:01 PM, Jeevan Chalke wrote:
> Hi,
>
> Attached new patch-set here. Changes include:
>
> 1. Added separate patch for costing Append node as discussed up-front in the
> patch-set.
> 2. Since we now cost Append node, we don't need partition_wise_agg_cost_factor
> GUC. So removed that. The remaining patch hence merged into main implementation
> patch.
> 3. Updated rows in test-cases so that we will get partition-wise plans.
>
> Thanks

I applied partition-wise-agg-v6.tar.gz patch to  the master and use shard.sh example from
https://www.postgresql.org/message-id/14577.1509723225%40localhost
Plan for count(*) is the following:

shard=# explain select count(*) from orders;                                      QUERY PLAN
--------------------------------------------------------------------------------------- Finalize Aggregate
(cost=100415.29..100415.30rows=1 width=8)   ->  Append  (cost=50207.63..100415.29 rows=2 width=8)         ->  Partial
Aggregate (cost=50207.63..50207.64 rows=1 width=8)               ->  Foreign Scan on orders_0 (cost=101.00..50195.13
rows=5000width=0)         ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)               ->  Foreign
Scanon orders_1 (cost=101.00..50195.13 rows=5000 width=0)
 


We really calculate partial aggregate for each partition, but to do we still have to fetch all data from remote host.
So for foreign partitions such plans is absolutely inefficient.
Amy be it should be combined with some other patch?
For example, with  agg_pushdown_v4.tgz patch https://www.postgresql.org/message-id/14577.1509723225%40localhost ?
But it is not applied after partition-wise-agg-v6.tar.gz patch.
Also postgres_fdw in 11dev is able to push down aggregates without agg_pushdown_v4.tgz patch.

In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch
there is the following check:
 /* Partial aggregates are not supported. */
+       if (extra->isPartial)
+           return;

If we just comment this line then produced plan will be the following:

shard=# explain select sum(product_id) from orders;                           QUERY PLAN
---------------------------------------------------------------- Finalize Aggregate  (cost=308.41..308.42 rows=1
width=8)  ->  Append  (cost=144.18..308.41 rows=2 width=8)         ->  Foreign Scan  (cost=144.18..154.20 rows=1
width=8)              Relations: Aggregate on (public.orders_0 orders)         ->  Foreign Scan  (cost=144.18..154.20
rows=1width=8)               Relations: Aggregate on (public.orders_1 orders)
 
(6 rows)

And it is actually desired plan!
Obviously such approach will not always work. FDW really doesn't support partial aggregates now.
But for most frequently used aggregates: sum, min, max, count aggtype==aggtranstype and there is no difference
between partial and normal aggregate calculation.
So instead of (extra->isPartial) condition we can add more complex check which will traverse pathtarget expressions
and
check if it can be evaluated in this way. Or... extend FDW API to support partial aggregation.

But even the last plan is not ideal: it will calculate predicates at each remote node sequentially.
There is parallel append patch:
https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_JUB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com
but ... FDW doesn't support parallel scan, so parallel append can not be applied in this case.
And we actually do not need parallel append with all its dynamic workers here.
We just need to start commands at all remote servers and only after it fetch results (which can be done sequentially).

I am investigating problem of efficient execution of OLAP queries on sharded tables (tables with remote partitions).
After reading all this threads and corresponding  patches, it seems to me
that we already have most of parts of the puzzle, what we need is to put them on right places and may be add missed
ones.
I wonder if somebody is busy with it and can I somehow help here?

Also I am not quite sure about the best approach with parallel execution of distributed query at all nodes.
Should we make postgres_fdw parallel safe and use parallel append? How difficult it will be?
Or in addition to parallel append we should also have "asynchronous append" which will be able to initiate execution at
allnodes?
 
It seems to be close to merge append, because it should simultaneously traverse all cursors.

Looks like second approach is easier for implementation. But in case of sharded table, distributed query may need to
traverseboth remote
 
and local shards and this approach doesn't allow to processed several local shards in parallel.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 11.11.2017 23:29, Konstantin Knizhnik wrote:
> On 10/27/2017 02:01 PM, Jeevan Chalke wrote:
>> Hi,
>>
>> Attached new patch-set here. Changes include:
>>
>> 1. Added separate patch for costing Append node as discussed up-front 
>> in the
>> patch-set.
>> 2. Since we now cost Append node, we don't need 
>> partition_wise_agg_cost_factor
>> GUC. So removed that. The remaining patch hence merged into main 
>> implementation
>> patch.
>> 3. Updated rows in test-cases so that we will get partition-wise plans.
>>
>> Thanks
>
> I applied partition-wise-agg-v6.tar.gz patch to  the master and use 
> shard.sh example from 
> https://www.postgresql.org/message-id/14577.1509723225%40localhost
> Plan for count(*) is the following:
>
> shard=# explain select count(*) from orders;
>                                       QUERY PLAN
> --------------------------------------------------------------------------------------- 
>
>  Finalize Aggregate  (cost=100415.29..100415.30 rows=1 width=8)
>    ->  Append  (cost=50207.63..100415.29 rows=2 width=8)
>          ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
>                ->  Foreign Scan on orders_0 (cost=101.00..50195.13 
> rows=5000 width=0)
>          ->  Partial Aggregate  (cost=50207.63..50207.64 rows=1 width=8)
>                ->  Foreign Scan on orders_1 (cost=101.00..50195.13 
> rows=5000 width=0)
>
>
> We really calculate partial aggregate for each partition, but to do we 
> still have to fetch all data from remote host.
> So for foreign partitions such plans is absolutely inefficient.
> Amy be it should be combined with some other patch?
> For example, with  agg_pushdown_v4.tgz patch 
> https://www.postgresql.org/message-id/14577.1509723225%40localhost ?
> But it is not applied after partition-wise-agg-v6.tar.gz patch.
> Also postgres_fdw in 11dev is able to push down aggregates without 
> agg_pushdown_v4.tgz patch.
>
> In 0009-Teach-postgres_fdw-to-push-aggregates-for-child-rela.patch
> there is the following check:
>
>  /* Partial aggregates are not supported. */
> +       if (extra->isPartial)
> +           return;
>
> If we just comment this line then produced plan will be the following:
>
> shard=# explain select sum(product_id) from orders;
>                            QUERY PLAN
> ----------------------------------------------------------------
>  Finalize Aggregate  (cost=308.41..308.42 rows=1 width=8)
>    ->  Append  (cost=144.18..308.41 rows=2 width=8)
>          ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
>                Relations: Aggregate on (public.orders_0 orders)
>          ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
>                Relations: Aggregate on (public.orders_1 orders)
> (6 rows)
>
> And it is actually desired plan!
> Obviously such approach will not always work. FDW really doesn't 
> support partial aggregates now.
> But for most frequently used aggregates: sum, min, max, count 
> aggtype==aggtranstype and there is no difference
> between partial and normal aggregate calculation.
> So instead of (extra->isPartial) condition we can add more complex 
> check which will traverse pathtarget expressions and
> check if it can be evaluated in this way. Or... extend FDW API to 
> support partial aggregation.
>
> But even the last plan is not ideal: it will calculate predicates at 
> each remote node sequentially.
> There is parallel append patch:
> https://www.postgresql.org/message-id/CAJ3gD9ctEcrVUmpY6fq_JUB6WDKGXAGd70EY68jVFA4kxMbKeQ%40mail.gmail.com 
>
> but ... FDW doesn't support parallel scan, so parallel append can not 
> be applied in this case.
> And we actually do not need parallel append with all its dynamic 
> workers here.
> We just need to start commands at all remote servers and only after it 
> fetch results (which can be done sequentially).
>
> I am investigating problem of efficient execution of OLAP queries on 
> sharded tables (tables with remote partitions).
> After reading all this threads and corresponding  patches, it seems to me
> that we already have most of parts of the puzzle, what we need is to 
> put them on right places and may be add missed ones.
> I wonder if somebody is busy with it and can I somehow help here?
>
> Also I am not quite sure about the best approach with parallel 
> execution of distributed query at all nodes.
> Should we make postgres_fdw parallel safe and use parallel append? How 
> difficult it will be?
> Or in addition to parallel append we should also have "asynchronous 
> append" which will be able to initiate execution at all nodes?
> It seems to be close to merge append, because it should simultaneously 
> traverse all cursors.
>
> Looks like second approach is easier for implementation. But in case 
> of sharded table, distributed query may need to traverse both remote
> and local shards and this approach doesn't allow to processed several 
> local shards in parallel.
>

I attach small patch for postgres_fdw.c which allows concurrent 
execution of aggregates by all remote servers (when them are accessed 
through postgres_fdw).
I have added "postgres_fdw.use_prefetch" GUC to enable/disable 
prefetching data in postgres_fdw.
This patch should be applied after of partition-wise-agg-v6.tar.gz patch.
With shard example and the following two GUCs set:

shard=# set postgres_fdw.use_prefetch=on;
shard=# set enable_partition_wise_agg=on;
shard=# select sum(product_id) from orders;
    sum
---------
  9965891
(1 row)

shard=# explain select sum(product_id) from orders;
                            QUERY PLAN
----------------------------------------------------------------
  Finalize Aggregate  (cost=308.41..308.42 rows=1 width=8)
    ->  Append  (cost=144.18..308.41 rows=2 width=8)
          ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
                Relations: Aggregate on (public.orders_0 orders)
          ->  Foreign Scan  (cost=144.18..154.20 rows=1 width=8)
                Relations: Aggregate on (public.orders_1 orders)
(6 rows)


sum aggregate is calculated in parallel by both servers.

I have not tested it much, it is just prove of concept.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

As explained by Ashutosh Bapat in reply
https://www.postgresql.org/message-id/CAFjFpRdpeMTd8kYbM_x0769V-aEKst5Nkg3+coG=8ki7s8Zqjw@mail.gmail.com
we cannot rely on just aggtype==aggtranstype.

However, I have tried pushing partial aggregation over remote server and also
submitted a PoC patch here:
https://www.postgresql.org/message-id/CAM2+6=UakP9+TSJuh2fbhHWNJc7OYFL1_gvu7mt2fXtVt6GY3g@mail.gmail.com

I have later removed these patches from Partition-wise-Aggregation patch set
as it is altogether a different issue than this mail thread. We might need to
discuss on it separately.

Interesting idea of "asynchronous append". However, IMHO it deserves its own
email-chain.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company 

On 16 November 2017 at 05:57, Konstantin Knizhnik
<k.knizhnik@postgrespro.ru> wrote:
> The main problem IMHO is that there are a lot of different threads and
> patches related with this topic:(
> And it is very difficult to combine all of them together to achieve the
> final goal: efficient execution of OLAP queries on sharded table.
> It will be nice if somebody who is making the most contribution in this
> direction can somehow maintain it...
> I just faced with particular problem with our pg_shardman extension and now
> (thanks to your patch) I have some working solution for it.
> But certainly I prefer to have this support in mainstream version of
> Postgres.

I don't think it's fair to be asking about additional features on this
thread. It seems to me you're asking about two completely separate
features, with the aim of trying to solve your own problems.

It also looks to me that Jeevan has been clear on what his goals are
for this patch. Perhaps what you're asking for is a logical direction
to travel once this patch is committed, so I think, probably, the best
way to conduct what you're after here is to either:

a) Wait until this is committed and spin up your own thread about
you're proposed changes to allow the PARTIAL aggregate to be pushed
into the foreign server, or;
b) Spin up your own thread now, with reference to this patch as a
prerequisite to your own patch.

I agree that what you're talking about is quite exciting stuff, but
please, let's not talk about it here.

-- David Rowley                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Nov 16, 2017 at 6:02 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> On 16 November 2017 at 05:57, Konstantin Knizhnik
> <k.knizhnik@postgrespro.ru> wrote:
>> The main problem IMHO is that there are a lot of different threads and
>> patches related with this topic:(
>> And it is very difficult to combine all of them together to achieve the
>> final goal: efficient execution of OLAP queries on sharded table.
>> It will be nice if somebody who is making the most contribution in this
>> direction can somehow maintain it...
>> I just faced with particular problem with our pg_shardman extension and now
>> (thanks to your patch) I have some working solution for it.
>> But certainly I prefer to have this support in mainstream version of
>> Postgres.
>
> I don't think it's fair to be asking about additional features on this
> thread. It seems to me you're asking about two completely separate
> features, with the aim of trying to solve your own problems.
>
> It also looks to me that Jeevan has been clear on what his goals are
> for this patch. Perhaps what you're asking for is a logical direction
> to travel once this patch is committed, so I think, probably, the best
> way to conduct what you're after here is to either:
>
> a) Wait until this is committed and spin up your own thread about
> you're proposed changes to allow the PARTIAL aggregate to be pushed
> into the foreign server, or;
> b) Spin up your own thread now, with reference to this patch as a
> prerequisite to your own patch.
>
> I agree that what you're talking about is quite exciting stuff, but
> please, let's not talk about it here.
>

+1 for all that.


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

On Wed, Nov 15, 2017 at 5:31 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
> OK. Done in the attached patch set.
>
> I have rebased all my patches on latest HEAD which is at
> 7518049980be1d90264addab003476ae105f70d4
>
> Thanks

These are review comments for the last set and I think most of them
apply to the new set as well.

Patches 0001 - 0005 refactoring existing code. I haven't
reviewed them in detail, checking whether we have missed anything in moving the
code, but they mostly look fine.

Comments on 0006/*
+ * cost_append
+ *      Determines and returns the cost of an Append node.
+ *
... clipped portion
+
+    /* Add Append node overhead. */
+    run_cost += cpu_tuple_cost * DEFAULT_APPEND_COST_FACTOR * tuples;
+

I am wondering whether it's really worth creating a new function for a single
line addition to create_append_path(). I think all we need to change in
create_append_path() is add (cpu_tuple_cost * DEFAULT_APPEND_COST_FACTOR *
tuples) to path->total_cost.


+    /* Add MergeAppend node overhead like we do it for the Append node */
+    run_cost += cpu_tuple_cost * DEFAULT_APPEND_COST_FACTOR * tuples;
+

With this change the following comment is no more true. Please remove it.    * extracted tuple.  We don't charge
cpu_tuple_costbecause a MergeAppend    * node doesn't do qual-checking or projection, so it has less overhead    * than
mostplan nodes.    */
 

+/*
+ * Arbitrarily use 50% of the cpu_tuple_cost to cost append node. Note that

May be reword it as " ... to cost per tuple processing by an append node ..."

+ * this value should be multiplied with cpu_tuple_cost wherever applicable.
+ */
+#define DEFAULT_APPEND_COST_FACTOR 0.5

I am wondering whether we should just define
#define APPEND_TUPLE_COST (cpu_tuple_cost * 0.5)
and use this macro everywhere. What else use DEFAULT_APPEND_COST_FACTOR would
have other than multiplying with cpu_tuple_cost?
-- test partition matching with N-way joinEXPLAIN (COSTS OFF)SELECT avg(t1.a), avg(t2.b), avg(t3.a + t3.b), t1.c, t2.c,
t3.cFROM
 
plt1 t1, plt2 t2, plt1_e t3 WHERE t1.c = t2.c AND ltrim(t3.c, 'A') =
t1.c GROUP BY t1.c, t2.c, t3.c ORDER BY t1.c, t2.c, t3.c;
-                                      QUERY PLAN
---------------------------------------------------------------------------------------
+                                   QUERY PLAN
+-------------------------------------------------------------------------------- Sort   Sort Key: t1.c, t3.c   ->
HashAggregate        Group Key: t1.c, t2.c, t3.c
 
-         ->  Result
+         ->  Hash Join
+               Hash Cond: (t1.c = t2.c)               ->  Append
-                     ->  Hash Join
-                           Hash Cond: (t1.c = t2.c)

That's sad. Interestingly this query has an aggregate, so the plan will use
partition-wise join again when partition-wise aggregation patch will be
applied. So may be fine.

- Append  (cost=0.00..0.04 rows=2 width=32)
+ Append  (cost=0.00..0.05 rows=2 width=32)

- Append  (cost=0.00..0.04 rows=2 width=4)
+ Append  (cost=0.00..0.05 rows=2 width=4)

We do have some testcases which print costs. Interesting :). I don't have
any objection to this change.

Comments on 0007

+       <para>
+        Enables or disables the query planner's use of partition-wise grouping
+        or aggregation, which allows  If partition-wise aggregation
does not result in the
+        cheapest path, it will still spend time in creating these paths and
+        consume memory which increase linearly with the number of partitions.
+        The default is <literal>off</>.
+       </para>
+      </listitem>
+     </varlistentry>
+
May be we should word this in the same manner as partition-wise join like
       Enables or disables the query planner's use of partition-wise grouping       or aggregation, which allows
aggregationor grouping on a partitioned       tables to be spread across the partitions. If <literal>GROUP
BY<literal>clause includes partition keys, the rows are aggregated at       each partition. Otherwise, partial
aggregatescomputed for each       partition are required to be combined. Because partition-wise
aggregation/gropuingcan use significantly more CPU time and memory       during planning, the default is
<literal>off</literal>.

+
+Partition-wise aggregates/grouping
+----------------------------------

... clipped patch

+In above plan, aggregation is performed after append node which means that the
+whole table is an input for the aggregation node. However, with partition-wise
+aggregation, same query will have plane like:

s/plane/plan/

+ Append

... clipped patch

+PartialAggregate stage greatly reduces the number of groups and lose if we have
+lots of small groups.

To keep the discussion brief, I suggest we rewrite this paragraph as

----
If GROUP BY clause has all partition keys, all the rows that belong to a given
group come from a single partition and thus aggregates can be finalized
separately for each partition. When the number of groups is far lesser than the
number of rows being grouped, as usually is the case, the number of rows
processed by an Append node reduces apart from reducing the size of the hash
table or size of the data to be sorted. This usually improves efficiency of the
query. If GROUP BY doesn't contain all the partition keys, partial
aggregates can be computed for
each partition followed by combining partial aggregates from one or more
partitions belonging to the same group to compute complete aggregate for each
group. This improves efficiency of the query if the number of groups is far
less than the number of rows produced by the scan underneath.
---

I am not sure whether we should be discussing why this technique performs
better or when it performs better. We don't have similar discussion for
partition-wise join. That paragraph just describes the technique and may be we
want to do the same here.

+ *
+ * extra is the additional information required when we are doing aggregation
+ * or grouping below the append node. In case of partial partition-wise
+ * aggregation on a child node, we need to compute finalized step after the
+ * append, which cannot be done in this function. And thus if we have non-NULL
+ * value for extra, we call create_partition_agg_paths() to create an append
+ * node and finalization, if any.

May be we want to just say "extra provides more information about the
partitioned aggregation/grouping e.g path target, whether to use partial
aggregate and so on." When present we call create_partition_agg_paths() to add
paths for partition-wise aggregatges.

-        add_path(rel, (Path *) create_append_path(rel, subpaths,
-                                                  rel->reltarget, NULL, 0,
-                                                  partitioned_rels));
+    {
+        if (extra)
+            create_partition_agg_paths(root, rel, subpaths, NIL,
+                                       NIL, NULL, 0,
+                                       partitioned_rels, extra);
+        else
+            add_path(rel, (Path *) create_append_path(rel, subpaths,
+                                                      rel->reltarget, NULL, 0,
+                                                      partitioned_rels));
+    }

I am wondering whether we could write a function to call appropriate one out of
create_append_path(), create_partition_agg_paths() or
create_merge_append_path() based on the presence of extra and/or pathkeys and
use it everywhere such a change is made. I don't know whether that will be
worth the code. But there are a handful places where such diffs are required.

-
-    plan = make_sort_from_pathkeys(subplan, best_path->path.pathkeys, NULL);
+    plan = make_sort_from_pathkeys(subplan, best_path->path.pathkeys,
+                                   IS_OTHER_REL(best_path->subpath->parent) ?
+                                   best_path->path.parent->relids : NULL);

While I can guess why this change is required, it may be better to separate it
into a patch of its own and adding some explanation in the commit message, for
other reviewers.

+    /* Copy input rels's relids to grouped rel */
+    grouped_rel->relids = input_rel->relids;

I am fine with this change, but Tom may not agree [1]. May be we should get his
opinion on this one.
    /*
+     * If input relation is partitioned, check if we can perform
+     * partition-wise grouping and/or aggregation.
+     */

Just like partition-wise join a concise "Apply partition-wise aggregation
technique, if possible." would suffice.
    dNumPartialGroups = get_number_of_groups(root,
cheapest_partial_path->rows,                                            gd,
 
-                                             parse->targetList);
+
make_tlist_from_pathtarget(target));
Can we guarantee that the output of make_tlist_from_pathtarget() will be same
as translation of parse->targetList for the given child? Even if not, may be
it's fine to pass slightly different tlist to get_number_of_groups() since it
doesn't depend upon the exact shape but right group column references.
Nonetheless something to test and verify.
 *
- * Determines whether parallel grouping and/or aggrgation is possible, or not.
+ * Determines whether parallel grouping and/or aggregation is possible, or not. * Returns true when possible, false
otherwise.

Does this hunk belong to one of the refactoring patches or as a separate patch
correcting a typo?

+/*
+ * try_partition_wise_grouping
+ *
+ * If the input relation is partitioned and the partition keys are part of the
+ * group by clauses, each partition produces a different set of groups.
+ * Aggregates within each such group can be computed partition-wise. This

While these sentences are correct, I think the reason why we could compute an
aggregate at the level of each partition is because rows from a given group
belong to a single partition. So, I guess, we have to reword this as

"If the partition keys of input relation are part of group by clause, all the
rows belonging to a given group come from a single partition, each partition
producing a different set of groups. This allows aggregation/grouping over a
partitioned relation to be broken down into aggregation/grouping on each
partition.

If group by clause does not contain all the partition keys, rows from a given
group may be spread across multiple partitions. In that case, we can combine
partial aggregates for a given group across partitions to produce the final
aggregate for a that group "

+ * might be optimal because of presence of suitable paths with pathkeys or
+ * because the hash tables for most of the partitions fit into the memory.
+ * However, if partition keys are not part of the group by clauses, then we
+ * still able to compute the partial aggregation for each partition and then
+ * finalize them over append. This can either win or lose. It may win if the
+ * PartialAggregate stage greatly reduces the number of groups and lose if we
+ * have lots of small groups.

I have not seen prologue of a function implementing a query optimization
technique explain why that technique improves performance. So I am not sure
whether the comment should include this explanation. One of the reasons being
that the reasons why a technique works might change over the period of time
with the introduction of other techniques, thus obsoleting the comment.  But
may be it's good to have it here.

+    /*
+     * Grouping sets plan does not work with an inheritance subtree (see notes
+     * in create_groupingsets_plan). Thus do not handle grouping sets here.
+     */
+    if (query->groupingSets || gd)
+        return;

Even if that restriction is lifted, we won't be able to compute
"whole" grouping sets
for each partition, since grouping sets implies multiple group by clauses, each
of which may not have all partition keys. Those sets which have all partition
keys will be computed completely for each partition, but others will require
partial aggregation. I guess, we will need to apply partition-wise aggregation
at each derived group by clause and not as a whole-sale strategy.

Anyway, it doesn't look like a good idea to pass an argument (gd) only to
return from that function in case of its presence. May be we should handle it
outside this function.

+
+    /* Nothing to do, if the input relation is not partitioned. */
+    if (!input_rel->part_scheme)
+        return;
+
+    Assert(input_rel->part_rels);

For a join between two partitioned tables with one of them being dummy
relation, would have part_scheme set but not part_rels (See
try_partition_wise_join()). This assertion would
fail in such a case. Have you tested the case? May be we should just test if
input_rel->part_rels exists similar to generate_partition_wise_join_paths().
Also, how is a dummy input relation is handled in this function? Do we need to
handle?

+    nparts = input_rel->nparts;
+    part_rels = (RelOptInfo **) palloc(nparts * sizeof(RelOptInfo *));
+    grouped_rel->part_rels = part_rels;

For a partial aggregation, we can't say that the child rels produced here are
partitions of the top grouped relation, so setting part_rels looks wrong. We
should set this only when a full aggregate is obtained from each partition.

+        scanjoin_target =
copy_pathtarget(input_rel->cheapest_startup_path->pathtarget);
+        scanjoin_target->exprs = (List *) adjust_appendrel_attrs(root,
+
(Node *) scanjoin_target->exprs,
+                                                                 nappinfos,
+                                                                 appinfos);

Why can't we use input_child_rel->pathtarget? It should be same as the
translation of its parent's path target. I probably understand that's because
the input rel's path targets have been changed after the underlying join was
planned, a step which is not applied to the individual children. May be add a
comment here?

+        child_target->exprs = (List *) adjust_appendrel_attrs(root,
+                                                              (Node
*) target->exprs,
+                                                              nappinfos,
+                                                              appinfos);
+        partial_target = make_partial_grouping_target(root, target);
+        partial_target->exprs = (List *) adjust_appendrel_attrs(root,
+                                                                (Node
*) partial_target->exprs,
+                                                                nappinfos,
+                                                                appinfos);

We need both of these steps for any aggregate since parallel paths will compute
parial paths anyway. If that's correct, may be we should add a comment?

+        extra.inputRows = 0;    /* Not needed at child paths creation */

Why? Comment should be on its own line.

+        if (!create_child_grouping_paths(root, input_child_rel, agg_costs, gd,
+                                         &extra))
+        {
+            /* Could not create path for childrel, return */
+            pfree(appinfos);
+            return;
+        }

Can we detect this condition and bail out even before planning any of the
children? It looks wasteful to try to plan children only to bail out in this
case.

+    /* Nothing to do if we have no live children */
+    if (live_children == NIL)
+        return;

A parent relation with all dummy children will also be dummy. May be we should
mark the parent dummy case using mark_dummy_rel() similar to
generate_partition_wise_join_paths().

+/*
+ * have_grouping_by_partkey
+ *

Somehow this name sounds like it would return true when GROUP BY contains only
partition key. May be rename as group_by_has_partkey? to indicate the

+ * Returns true, if partition keys of the given relation are part of the
+ * GROUP BY clauses, false otherwise.

Reword as " ... if all the partition keys of ... "

+static bool
+have_grouping_by_partkey(RelOptInfo *input_rel, PathTarget *target,
+                         List *groupClause)
+{
+    List       *tlist = make_tlist_from_pathtarget(target);
+    List       *groupexprs = get_sortgrouplist_exprs(groupClause, tlist);

Have we tested the case with multi-level partitioned table and children with
different order of partition key columns?

+        partexprs = input_rel->partexprs ? input_rel->partexprs[cnt] : NIL;
+
+        /* Rule out early, if there are no partition keys present */
+        if (partexprs == NIL)
+            return false;

If input_rel->partexprs is NIL, we should "bail" out even before the loop
starts.

+        foreach(lc, partexprs)
+        {
+            Expr       *partexpr = lfirst(lc);
+
+            if (list_member(groupexprs, partexpr))
+            {
+                found = true;
+                break;
+            }
+        }

This looks like a useful piece of general functionality
list_has_intersection(), which would returns boolean instead of the whole
intersection. I am not sure whether we should add that function to list.c and
use here.

+         * If none of the partition key matches with any of the GROUP BY

Reword as "... the partition key expressions match with ...."

This isn't a full review of 0007, but I think it covers most of the new
functionality.

[1] https://www.postgresql.org/message-id/CAFjFpRdUz6h6cmFZFYAngmQAX8Zvo+MZsPXidZ077h=gp9bvQw@mail.gmail.com



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

On Fri, Nov 17, 2017 at 7:24 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> + * this value should be multiplied with cpu_tuple_cost wherever applicable.
> + */
> +#define DEFAULT_APPEND_COST_FACTOR 0.5
>
> I am wondering whether we should just define
> #define APPEND_TUPLE_COST (cpu_tuple_cost * 0.5)
> and use this macro everywhere. What else use DEFAULT_APPEND_COST_FACTOR would
> have other than multiplying with cpu_tuple_cost?

-1.  If you wrap it in a macro like that, future readers of the code
will have to go look up what the macro does.  If you just multiply by
DEFAULT_APPEND_COST_FACTOR it will be clear that's what being used is
a multiple of cpu_tuple_cost.

> I am not sure whether we should be discussing why this technique performs
> better or when it performs better. We don't have similar discussion for
> partition-wise join. That paragraph just describes the technique and may be we
> want to do the same here.

+1.

> + * might be optimal because of presence of suitable paths with pathkeys or
> + * because the hash tables for most of the partitions fit into the memory.
> + * However, if partition keys are not part of the group by clauses, then we
> + * still able to compute the partial aggregation for each partition and then
> + * finalize them over append. This can either win or lose. It may win if the
> + * PartialAggregate stage greatly reduces the number of groups and lose if we
> + * have lots of small groups.
>
> I have not seen prologue of a function implementing a query optimization
> technique explain why that technique improves performance. So I am not sure
> whether the comment should include this explanation. One of the reasons being
> that the reasons why a technique works might change over the period of time
> with the introduction of other techniques, thus obsoleting the comment.  But
> may be it's good to have it here.

+1 for keeping it.

> +        extra.inputRows = 0;    /* Not needed at child paths creation */
>
> Why? Comment should be on its own line.

Comments on same line are fine if they are short enough.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Nov 23, 2017 at 6:38 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> Agree. However, there was ab existing comment in create_append_path() saying
> "We don't bother with inventing a cost_append(), but just do it here", which
> implies at sometime in future we may need it; so why not now where we are
> explicitly costing for an append node. Having a function is good so that, if
> required in future, we need update in only this function.
> Let me know if you think otherwise, I make those changes in next patchset.
>

I don't read that comment as something we will do it in future. I
don't think the amount of changes that this patch introduces just for
adding one more line of code aren't justified. There's anyway only one
place where we are costing append, so it's not that the new function
avoids code duplication. Although I am happy to defer this to the
committer, if you think that we need a separate function.

>
> As suggested by Robert, I have renamed it to APPEND_CPU_COST_MULTIPLIER in
> v7 patchset.
> Also, retained the #define for just multiplier as suggested by Robert.

Ok.


>
>>
>>
>> Anyway, it doesn't look like a good idea to pass an argument (gd) only to
>> return from that function in case of its presence. May be we should handle
>> it
>> outside this function.
>
>
> Well, I would like to have it inside the function itself. Let the function
> itself do all the necessary checking rather than doing some of them outside.

We will leave this to the committer. I don't like that style, but it's
also good to expect a function to do all related work.


>>
>> +        if (!create_child_grouping_paths(root, input_child_rel,
>> agg_costs, gd,
>> +                                         &extra))
>> +        {
>> +            /* Could not create path for childrel, return */
>> +            pfree(appinfos);
>> +            return;
>> +        }
>>
>> Can we detect this condition and bail out even before planning any of the
>> children? It looks wasteful to try to plan children only to bail out in
>> this
>> case.
>
>
> I don't think so. It is like non-reachable and added just for a safety in
> case we can't able to create a child path. The bail out conditions cannot be
> evaluated at the beginning. Do you this an Assert() will be good here? Am I
> missing something?

An Assert would help. If it's something that should not happen, we
should try catching that rather that silently ignoring it.

>
>>
>> +    /* Nothing to do if we have no live children */
>> +    if (live_children == NIL)
>> +        return;
>>
>> A parent relation with all dummy children will also be dummy. May be we
>> should
>> mark the parent dummy case using mark_dummy_rel() similar to
>> generate_partition_wise_join_paths().
>
>
> If parent is dummy, then we are not at all doing PWA. So no need to mark
> parent grouped_rel as dummy I guess.
> However, if some of the children are dummy, I am marking corresponding upper
> rel as dummy too.
> Actually, this condition will never going to be true as you said correctly
> that "A parent relation with all dummy children will also be dummy". Should
> we have an Assert() instead?

Yes.


>
>
> I have testcase for multi-level partitioned table.
> However, I did not understand by what you mean by "children with different
> order of partition key columns". I had a look over tests in
> partition_join.sql and it seems that I have cover all those scenarios.
> Please have a look over testcases added for PWA and let me know the
> scenarios missing, I will add them then.

By children with different order of partition key columns, I meant
something like this

parent(a int, b int, c int) partition by (a), child1(b int, c int, a
int) partition by b, child1_1 (c int, a int, b int);

where the attribute numbers of the partition keys in different
children are different.

>> This looks like a useful piece of general functionality
>> list_has_intersection(), which would returns boolean instead of the whole
>> intersection. I am not sure whether we should add that function to list.c
>> and
>> use here.
>
>
> Sounds good.
> But for now, I am keeping it as part of this feature itself.

ok

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

On Thu, Nov 23, 2017 at 6:38 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Let me know if I missed any comment to be fixed.

Hi,

I have applied v8 patches on commit id 8735978e7aebfbc499843630131c18d1f7346c79,
and getting below observation, please take a look.

Observation:
"when joining a foreign partition table with local partition table
getting wrong output
with partition_wise_join enabled, same is working fine on PG-head
without aggregates patch."

Test-case:
CREATE EXTENSION postgres_fdw;
CREATE SERVER pwj_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS
(dbname 'postgres',port '5432',use_remote_estimate 'true');
CREATE USER MAPPING FOR PUBLIC SERVER pwj_server;

CREATE TABLE fplt1 (a int,  c text) PARTITION BY LIST(c);
CREATE TABLE fplt1_p1 (a int,  c text);
CREATE TABLE fplt1_p2 (a int,  c text);
CREATE FOREIGN TABLE ftplt1_p1 PARTITION OF fplt1 FOR VALUES IN
('0000', '0001', '0002', '0003') SERVER pwj_server OPTIONS (TABLE_NAME
'fplt1_p1');
CREATE FOREIGN TABLE ftplt1_p2 PARTITION OF fplt1 FOR VALUES IN
('0004', '0005', '0006', '0007') SERVER pwj_server OPTIONS (TABLE_NAME
'fplt1_p2');
INSERT INTO fplt1_p1 SELECT i, to_char(i%8, 'FM0000') FROM
generate_series(0, 199, 2) i;
INSERT INTO fplt1_p2 SELECT i, to_char(i%8, 'FM0000') FROM
generate_series(200, 398, 2) i;

CREATE TABLE lplt2 (a int,  c text) PARTITION BY LIST(c);
CREATE TABLE lplt2_p1 PARTITION OF lplt2 FOR VALUES IN ('0000',
'0001', '0002', '0003');
CREATE TABLE lplt2_p2 PARTITION OF lplt2 FOR VALUES IN ('0004',
'0005', '0006', '0007');
INSERT INTO lplt2 SELECT i, to_char(i%8, 'FM0000') FROM
generate_series(0, 398, 3) i;

SELECT t1.c, t2.c,count(*) FROM fplt1 t1 JOIN lplt2 t2 ON (t1.c = t2.c
and t1.a = t2.a)  WHERE t1.a % 25 = 0 GROUP BY 1,2 ORDER BY t1.c,
t2.c; c   |  c   | count
------+------+-------0000 | 0000 |     10004 | 0004 |     10006 | 0006 |     1
(3 rows)

SET enable_partition_wise_join = on;
SELECT t1.c, t2.c,count(*) FROM fplt1 t1 JOIN lplt2 t2 ON (t1.c = t2.c
and t1.a = t2.a)  WHERE t1.a % 25 = 0 GROUP BY 1,2 ORDER BY t1.c,
t2.c; c   |  c   | count
------+------+-------0000 | 0000 |     10004 | 0004 |     1
(2 rows)


Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

On Tue, Nov 28, 2017 at 5:50 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> [snip]

This is still a hot topic so I am moving it to next CF.
-- 
Michael

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 relids in grouped_rel->relids for FDW. There are
two ways to do this: 1. set grouped_rel->relids for parent upper rel as well,
but then we should pass relids to fetch_upper_rel() instead of setting those
later. 2. For a parent upper rel, in create_foreignscan_plan(), set relids to
all_baserels, if upper_rel->relids is NULL and don't set relids for a parent
upper rel. I am fine with either of those.

+            /* partial phase */
+            get_agg_clause_costs(root, (Node *) partial_target->exprs,
+                                 AGGSPLIT_INITIAL_SERIAL,
+                                 &agg_partial_costs);

IIUC, the costs for evaluating aggregates would not change per child. They
won't be different for parent and any of the children. So, we should be able to
calculate those once, save in "extra" and use repeatedly.

+        if (can_sort)
+        {
+            Path       *path = cheapest_path;
+
+            if (!(pathkeys_contained_in(root->group_pathkeys,
+                                        path->pathkeys)))
[ .. clipped patch .. ]
+                                           NIL,
+                                           dNumGroups));
+        }

We create two kinds of paths partial paths for parallel query and partial
aggregation paths when group keys do not have partition keys. The comments and
code uses partial to mean both the things, which is rather confusing. May be we
should use term "partial aggregation" explicitly wherever it means that in
comments and in variable names.

I still feel that create_grouping_paths() and create_child_grouping_paths()
have a lot of common code. While I can see that there are some pockets in
create_grouping_paths() which are not required in create_child_grouping_paths()
and vice-versa, may be we should create only one function and move those
pockets under "if (child)" or "if (parent)" kind of conditions. It will be a
maintenance burden to keep these two functions in sync in future. If we do not
keep them in sync, that will introduce bugs.

+
+/*
+ * create_partition_agg_paths
+ *
+ * Creates append path for all the partitions and adds into the grouped rel.

I think you want to write "Creates an append path containing paths from all the
child grouped rels and adds into the given parent grouped rel".

+ * For partial mode we need to add a finalize agg node over append path before
+ * adding a path to grouped rel.
+ */
+void
+create_partition_agg_paths(PlannerInfo *root,
+                           RelOptInfo *grouped_rel,
+                           List *subpaths,
+                           List *partial_subpaths,

Why do we have these two as separate arguments? I don't see any call to
create_partition_agg_paths() through add_append_path() passing both of them
non-NULL simultaneously. May be you want use a single list subpaths and another
boolean indicating whether it's list of partial paths or regular paths.

+
+    /* For non-partial path, just create a append path and we are done. */
This is the kind of confusion, I am talking about above. Here you have
mentioned "non-partial path" which may mean a regular path but what you
actually mean by that term is a path representing partial aggregates.

+    /*
+     * Partial partition-wise grouping paths.  Need to create final
+     * aggregation path over append relation.
+     *
+     * If there are partial subpaths, then we need to add gather path before we
+     * append these subpaths.

More confusion here.

+     */
+    if (partial_subpaths)
+    {
+        ListCell   *lc;
+
+        Assert(subpaths == NIL);
+
+        foreach(lc, partial_subpaths)
+        {
+            Path       *path = lfirst(lc);
+            double        total_groups = path->rows * path->parallel_workers;
+
+            /* Create gather paths for partial subpaths */
+            Path *gpath = (Path *) create_gather_path(root, grouped_rel, path,
+                                                      path->pathtarget, NULL,
+                                                      &total_groups);
+
+            subpaths = lappend(subpaths, gpath);

Using the argument variable is confusing and that's why you need two different
List variables. Instead probably you could have another variable local to this
function to hold the gather subpaths.

AFAIU, the Gather paths that this code creates has its parent set to
parent grouped
rel. That's not correct. These partial paths come from children of grouped rel
and each gather is producing rows corresponding to one children of grouped rel.
So gather path's parent should be set to corresponding child and not parent
grouped rel.

This code creates plans where there are multiple Gather nodes under an Append
node. AFAIU, the workers assigned to one gather node can be reused until that
Gather node finishes. Having multiple Gather nodes under an Append mean that
every worker will be idle from the time that worker finishes the work till the
last worker finishes the work. That doesn't seem to be optimal use of workers.
The plan that we create with Gather on top of Append seems to be better. So, we
should avoid creating one Gather node per child plans. Have we tried to compare
performance of these two plans?

+        if (!IsA(apath, MergeAppendPath) && root->group_pathkeys)
+        {
+            spath = (Path *) create_sort_path(root,
+                                              grouped_rel,
+                                              apath,
+                                              root->group_pathkeys,
+                                              -1.0);
+        }

The code here assumes that a MergeAppend path will always have pathkeys
matching group_pathkeys. I believe that's true but probably we should have an
Assert to make it clear and add comments. If that's not true, we will need to
sort the output of MergeAppend OR discard MergeAppend paths which do not have
pathkeys matching group_pathkeys.


diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index b422050..1941468 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2345,6 +2345,7 @@ UnlistenStmt
 UnresolvedTup
 UnresolvedTupData
 UpdateStmt
+UpperPathExtraData
 UpperRelationKind
 UpperUniquePath
 UserAuth

Do we commit this file as part of the feature?
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Fri, Dec 1, 2017 at 7:41 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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 workers assigned to one gather node can be reused until that
> Gather node finishes. Having multiple Gather nodes under an Append mean that
> every worker will be idle from the time that worker finishes the work till the
> last worker finishes the work.

No, workers will exit as soon as they finish.  They don't hang around idle.

> index b422050..1941468 100644
> --- a/src/tools/pgindent/typedefs.list
> +++ b/src/tools/pgindent/typedefs.list
> @@ -2345,6 +2345,7 @@ UnlistenStmt
>  UnresolvedTup
>  UnresolvedTupData
>  UpdateStmt
> +UpperPathExtraData
>  UpperRelationKind
>  UpperUniquePath
>  UserAuth
>
> Do we commit this file as part of the feature?

Andres and I regularly commit such changes; Tom rejects them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Dec 2, 2017 at 4:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Dec 1, 2017 at 7:41 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> 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.

Ah, I didn't think about it. Thanks for bringing it up.

>
>> AFAIU, the workers assigned to one gather node can be reused until that
>> Gather node finishes. Having multiple Gather nodes under an Append mean that
>> every worker will be idle from the time that worker finishes the work till the
>> last worker finishes the work.
>
> No, workers will exit as soon as they finish.  They don't hang around idle.

Sorry, I think I used wrong word "idle". I meant that if a worker
finishes and exists, the query can't use it that worker slot until the
next Gather node starts. But as you pointed out, starting and stopping
a worker is costlier than the cost of not using the slot. So we should
avoid such plans.

>
>> index b422050..1941468 100644
>> --- a/src/tools/pgindent/typedefs.list
>> +++ b/src/tools/pgindent/typedefs.list
>> @@ -2345,6 +2345,7 @@ UnlistenStmt
>>  UnresolvedTup
>>  UnresolvedTupData
>>  UpdateStmt
>> +UpperPathExtraData
>>  UpperRelationKind
>>  UpperUniquePath
>>  UserAuth
>>
>> Do we commit this file as part of the feature?
>
> Andres and I regularly commit such changes; Tom rejects them.
>

We will leave it to the committer to decide what to do with this hunk.

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

Hello,

I'm testing Partition wise and found a strange result in V8 patch
see  init_star_schema_agg.sql
<http://www.postgresql-archive.org/file/t348768/init_star_schema_agg.sql>  

Explain gives  
  ->  Seq Scan on facts_p2  (...) (actual time=0.012..0.012 rows=1 loops=1)
for partitions that are joined with empty partitions

I was expecting a message saying that partition facts_p2 was not accessed

Am I wrong ?
Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Thank you for the answer
This is a miss understanding of hash join behaviour on my side.

That means that there is at less on line read in facts_p2 part even if the
second table partition of the hash join operation is empty.

I will remember it now ;o)

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

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 aggregate values,
support unpartitioned aggregation first and then work on partitioned
aggregates. That is definitely a separate piece of work.

+-- ===================================================================
+-- test partition-wise-aggregates
+-- ===================================================================
+CREATE TABLE pagg_tab (a int, b int, c text) PARTITION BY RANGE(a);
+CREATE TABLE pagg_tab_p1 (a int, b int, c text);
+CREATE TABLE pagg_tab_p2 (a int, b int, c text);
+CREATE TABLE pagg_tab_p3 (a int, b int, c text);

Like partition-wise join testcases please use LIKE so that it's easy to change
the table schema if required.

+INSERT INTO pagg_tab_p1 SELECT i % 30, i % 50, to_char(i/30,
'FM0000') FROM generate_series(1, 3000) i WHERE (i % 30) < 10;
+INSERT INTO pagg_tab_p2 SELECT i % 30, i % 50, to_char(i/30,
'FM0000') FROM generate_series(1, 3000) i WHERE (i % 30) < 20 and (i %
30) >= 10;
+INSERT INTO pagg_tab_p3 SELECT i % 30, i % 50, to_char(i/30,
'FM0000') FROM generate_series(1, 3000) i WHERE (i % 30) < 30 and (i %
30) >= 20;

We have to do this because INSERT tuple routing to a foreign partition is not
supported right now. Somebody has to remember to change this to a single
statement once that's done.

+ANALYZE fpagg_tab_p1;
+ANALYZE fpagg_tab_p2;
+ANALYZE fpagg_tab_p3;

I thought this is not needed. When you ANALYZE the partitioned table, it would
analyze the partitions as well. But I see that partition-wise join is also
ANALYZING the foreign partitions separately. When I ran ANALYZE on a
partitioned table with foreign partitions, statistics for only the local tables
(partitioned and partitions) was updated. Of course this is separate work, but
probably needs to be fixed.

+-- When GROUP BY clause matches with PARTITION KEY.
+-- Plan when partition-wise-agg is disabled

s/when/with/

+-- Plan when partition-wise-agg is enabled

s/when/with/

+   ->  Append

Just like ForeignScan node's Relations tell what kind of ForeignScan this is,
may be we should annotate Append to tell whether the children are joins,
aggregates or relation scans. That might be helpful. Of course as another
patch.

+SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING
avg(b) < 25 ORDER BY 1;
+ a  | sum  | min | count
+----+------+-----+-------
+  0 | 2000 |   0 |   100
+  1 | 2100 |   1 |   100
[ ... clipped ...]
+ 23 | 2300 |   3 |   100
+ 24 | 2400 |   4 |   100
+(15 rows)

May be we want to reduce the number of rows to a few by using a stricter HAVING
clause?

+
+-- When GROUP BY clause not matches with PARTITION KEY.

... clause does not match ...

+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING
sum(a) < 800 ORDER BY 1;
+
QUERY PLAN

+-----------------------------------------------------------------------------------------------------------------------------------------------------
+ Sort
+   Output: fpagg_tab_p1.b, (avg(fpagg_tab_p1.a)),
(max(fpagg_tab_p1.a)), (count(*))
+               ->  Partial HashAggregate
[ ... clipped ... ]
+                     Output: fpagg_tab_p3.b, PARTIAL
avg(fpagg_tab_p3.a), PARTIAL max(fpagg_tab_p3.a), PARTIAL count(*),
PARTIAL sum(fpagg_tab_p3.a)
+                     Group Key: fpagg_tab_p3.b
+                     ->  Foreign Scan on public.fpagg_tab_p3
+                           Output: fpagg_tab_p3.b, fpagg_tab_p3.a
+                           Remote SQL: SELECT a, b FROM public.pagg_tab_p3
+(26 rows)

I think we interested in overall shape of the plan and not the details of
Remote SQL etc. So, may be turn off VERBOSE. This comment applies to an earlier
plan with enable_partition_wise_agg = false;

+
+SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING
sum(a) < 800 ORDER BY 1;
+ b  |         avg         | max | count
+----+---------------------+-----+-------
+  0 | 10.0000000000000000 |  20 |    60
+  1 | 11.0000000000000000 |  21 |    60
[... clipped ...]
+ 42 | 12.0000000000000000 |  22 |    60
+ 43 | 13.0000000000000000 |  23 |    60
+(20 rows)

Since the aggregates were not pushed down, I doubt we should be testing the
output. But this test is good to check partial aggregates over foreign
partition scans, which we don't have in postgres_fdw.sql I think. So, may be
add it as a separate patch?

Can you please add a test where we reference a whole-row; that usually has
troubles.

-    if (root->hasHavingQual && query->havingQual)
+    if (root->hasHavingQual && fpinfo->havingQual)

This is not exactly a problem with your patch, but why do we need to check both
the boolean and the actual clauses? If the boolean is true, query->havingQual
should be non-NULL and NULL otherwise.

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

I think we don't need havingQual as a separate member. foreign_grouping_ok()
separates the clauses in havingQual into shippable and non-shippable clauses
and saves in local_conditions and remote_conditions. Probably we want to use
those instead of adding a new member.

index 04e43cc..c8999f6 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -62,7 +62,8 @@ typedef void (*GetForeignJoinPaths_function)
(PlannerInfo *root,
 typedef void (*GetForeignUpperPaths_function) (PlannerInfo *root,
                                                UpperRelationKind stage,
                                                RelOptInfo *input_rel,
-                                               RelOptInfo *output_rel);
+                                               RelOptInfo *output_rel,
+                                               UpperPathExtraData *extra);

Probably this comment belongs to 0007, but it's in this patch that it becomes
clear how invasive UpperPathExtraData changes are. While UpperPathExtraData has
upper paths in the name, all of its members are related to grouping. That's
fine since we only support partition-wise aggregate and not the other upper
operations. But if we were to do that in future, which of these members would
be applicable to other upper relations? inputRows, pathTarget,
partialPathTarget may be applicable to other upper rels as well. can_sort,
can_hash may be applicable to DISTINCT, SORT relations. isPartial and
havingQual will be applicable only to Grouping/Aggregation. So, may be it's ok,
and like RelOptInfo we may separate them by comments.

Another problem with that structure is its name doesn't mention that the
structure is used only for child upper relations, whereas the code assumes that
if extra is not present it's a parent upper relation. May be we want to rename
it to that effect or always use it whether for a parent or a child relation.

We may want to rename pathTarget and partialPathTarget as relTarget and
partialRelTarget since those targets are not specific to any path, but will be
applicable to all the paths created for that rel.

On Mon, Dec 4, 2017 at 7:44 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Sat, Dec 2, 2017 at 4:08 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Dec 1, 2017 at 7:41 AM, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> 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.
>
> Ah, I didn't think about it. Thanks for bringing it up.
>
>>
>>> AFAIU, the workers assigned to one gather node can be reused until that
>>> Gather node finishes. Having multiple Gather nodes under an Append mean that
>>> every worker will be idle from the time that worker finishes the work till the
>>> last worker finishes the work.
>>
>> No, workers will exit as soon as they finish.  They don't hang around idle.
>
> Sorry, I think I used wrong word "idle". I meant that if a worker
> finishes and exists, the query can't use it that worker slot until the
> next Gather node starts. But as you pointed out, starting and stopping
> a worker is costlier than the cost of not using the slot. So we should
> avoid such plans.
>
>>
>>> index b422050..1941468 100644
>>> --- a/src/tools/pgindent/typedefs.list
>>> +++ b/src/tools/pgindent/typedefs.list
>>> @@ -2345,6 +2345,7 @@ UnlistenStmt
>>>  UnresolvedTup
>>>  UnresolvedTupData
>>>  UpdateStmt
>>> +UpperPathExtraData
>>>  UpperRelationKind
>>>  UpperUniquePath
>>>  UserAuth
>>>
>>> Do we commit this file as part of the feature?
>>
>> Andres and I regularly commit such changes; Tom rejects them.
>>
>
> We will leave it to the committer to decide what to do with this hunk.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company



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

On Wed, Dec 13, 2017 at 6:37 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
>
>
> On Tue, Dec 12, 2017 at 3:43 PM, Ashutosh Bapat
> <ashutosh.bapat@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 need to resolve the conflicts too.
>
> Once done, I will submit the new patch-set fixing these and earlier review
> comments.
>

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_parallel_workers_per_gather TO 0;

If you enable parallel aggregation for smaller data partition-wise aggregation
paths won't be chosen. I think this is the reason why you are disabling
parallel query. But we should probably explain that in a comment. Better if we
could come up testcases without disabling parallel query. Since parallel append
is now committed, may be it can help.

+
+-- Check with multiple columns in GROUP BY, order in target-list is reversed
+EXPLAIN (COSTS OFF)
+SELECT c, a, count(*) FROM pagg_tab GROUP BY a, c;
+                   QUERY PLAN
+-------------------------------------------------
+ Append
+   ->  HashAggregate
+         Group Key: pagg_tab_p1.a, pagg_tab_p1.c
+         ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(10 rows)

Why do we need this testcase?

+
+-- 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.c
+   ->  Result
+         One-Time Filter: false
+(4 rows)

Not part of your patch, I am wondering if we can further optimize this plan by
converting HashAggregate to Result (One-time Filter: false) and the aggregate
target. Just an idea.

+
+SELECT c, sum(a) FROM pagg_tab WHERE 1 = 2 GROUP BY c;
+ c | sum
+---+-----
+(0 rows)

I think we also need a case when the child input relations are marked dummy and
then the parent is marked dummy. Just use a condition with partkey = <none of
list bounds>.

+
+-- Check with SORTED paths. Disable hashagg to get group aggregate

Suggest: "Test GroupAggregate paths by disabling hash aggregates."

+-- When GROUP BY clause matches with PARTITION KEY.

I don't think we need "with", and just extend the same sentence with "complete
aggregation is performed for each partition"

+-- Should choose full partition-wise aggregation path

suggest: "Should choose full partition-wise GroupAggregate plan", but I guess
with the above suggestion, this sentence is not needed.

+
+-- When GROUP BY clause not matches with PARTITION KEY.
+-- Should choose partial partition-wise aggregation path

Similar suggestions as above.

+-- No aggregates, but still able to perform partition-wise aggregates

That's a funny construction. May be "Test partition-wise grouping without any
aggregates".

We should try some output for this query.

+
+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
+         Sort Key: pagg_tab_p1.a
+         ->  Group
+               Group Key: pagg_tab_p1.a
+               ->  Sort
+                     Sort Key: pagg_tab_p1.a
+                     ->  Seq Scan on pagg_tab_p1
[ ... clipped ... ]
+(19 rows)

It's strange that we do not annotate partial grouping as Partial. Does not look
like a bug in your patch. Do we get similar output with parallel grouping?

+
+-- ORDERED SET within aggregate
+EXPLAIN (COSTS OFF)
+SELECT a, sum(b order by a) FROM pagg_tab GROUP BY a ORDER BY 1, 2;
+                               QUERY PLAN
+------------------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_p1.a, (sum(pagg_tab_p1.b ORDER BY pagg_tab_p1.a))
+   ->  GroupAggregate
+         Group Key: pagg_tab_p1.a
+         ->  Sort
+               Sort Key: pagg_tab_p1.a
+               ->  Append
+                     ->  Seq Scan on pagg_tab_p1
+                     ->  Seq Scan on pagg_tab_p2
+                     ->  Seq Scan on pagg_tab_p3
+(10 rows)

pagg_tab is partitioned by column c. So, not having it in GROUP BY
itself might produce this plan if Partial parallel aggregation is expensive.
When testing negative tests like this GROUP BY should always have the partition
key.

In case of full aggregation, since all the rows that belong to the same group
come from the same partition, having an ORDER BY doesn't make any difference.
We should support such a case.

+INSERT INTO pagg_tab1 SELECT i%30, i%20 FROM generate_series(0, 299, 2) i;
+INSERT INTO pagg_tab2 SELECT i%20, i%30 FROM generate_series(0, 299, 3) i;

spaces around % operator?

+-- When GROUP BY clause matches with PARTITION KEY.
+-- Should choose full partition-wise aggregation path.

Probably we should just club single table and join cases under one set of
comments rather than repeating those? Create the tables once at the beginning
of the test file and group together the queries under one comment head.

+-- Disable mergejoin to get hash aggregate.
+SET enable_mergejoin TO false;

Why? We have tested that once.

+
+-- When GROUP BY clause not matches with PARTITION KEY.
+-- Should choose partial partition-wise aggregation path.
+-- Also check with SORTED paths. Disable hashagg to get group aggregate.
+SET enable_hashagg TO false;

Same as above. Two of those clubbed together they will produce one hash and one
group plan. That will cover it.

+-- Check with LEFT/RIGHT/FULL OUTER JOINs which produces NULL values for
+-- aggregation
+-- LEFT JOIN, should produce partial partition-wise aggregation plan as
+-- GROUP BY is on nullable column
+EXPLAIN (COSTS OFF)
+SELECT b.y, sum(a.y) FROM pagg_tab1 a LEFT JOIN pagg_tab2 b ON a.x =
b.y GROUP BY 1 ORDER BY 1 NULLS LAST;

May be you should explicitly use GROUP BY b.y in all of these queries.

+-- FULL JOIN, should produce partial partition-wise aggregation plan as
+-- GROUP BY is on nullable column

In case of a FULL JOIN partition keys from the joining relations land on
nullable side; there is no key on non-nulllable side, so an aggregation on top
of FULL JOIN will always be partial partition-wise aggregation.

+
+-- Empty relations on LEFT side, no partition-wise agg plan.

Suggest: Empty join relation because of empty outer side.  I don't think we are
writing a negative test to check whether partition-wise agg plan is not chosen.
We are testing the case when the join relation is empty.

+
+EXPLAIN (COSTS OFF)
+SELECT a, c, sum(b), avg(c), count(*) FROM pagg_tab GROUP BY c, a,
(a+b)/2 HAVING sum(b) = 50 AND avg(c) > 25 ORDER BY 1, 2, 3;

Keep this or the previous one, both is overkill. I will vote for this one, but
it's upto you.

May be add a testcase with the partition keys themselves switched; output just
the plan.

+-- Test with multi-level partitioning scheme
+-- Partition-wise aggregation is tried only on first level.
[ ... clipped ... ]
+-- Full aggregation as GROUP BY clause matches with PARTITION KEY

This seems to contradict with the previous comment. May be club them together
and say "Partition-wise aggregation with full aggregation only at the first
leve" and move that whole comment down.

+
+-- 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;
+                           QUERY PLAN
+----------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_p1.b, (sum(pagg_tab_p1.a)), (count(*))
+   ->  Finalize GroupAggregate
+         Group Key: pagg_tab_p1.b
+         ->  Sort
+               Sort Key: pagg_tab_p1.b
+               ->  Append
+                     ->  Partial HashAggregate
+                           Group Key: pagg_tab_p1.b
+                           ->  Seq Scan on pagg_tab_p1
+                     ->  Partial HashAggregate
+                           Group Key: pagg_tab_p2_s1.b
+                           ->  Append
+                                 ->  Seq Scan on pagg_tab_p2_s1
+                                 ->  Seq Scan on pagg_tab_p2_s2
+                     ->  Partial HashAggregate
+                           Group Key: pagg_tab_p3_s1.b
+                           ->  Append
+                                 ->  Seq Scan on pagg_tab_p3_s1
+                                 ->  Seq Scan on pagg_tab_p3_s2
+(20 rows)

Why aren't we seeing partial aggregation paths for level two and below
partitions?

+
+-- Test on middle level partitioned table which is further partitioned on b.
+-- Full aggregation as GROUP BY clause matches with PARTITION KEY
+EXPLAIN (COSTS OFF)
+SELECT b, sum(a), count(*) FROM pagg_tab_p3 GROUP BY b ORDER BY 1, 2, 3;
+                            QUERY PLAN
+-------------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_p3_s1.b, (sum(pagg_tab_p3_s1.a)), (count(*))
+   ->  Append
+         ->  HashAggregate
+               Group Key: pagg_tab_p3_s1.b
+               ->  Seq Scan on pagg_tab_p3_s1
+         ->  HashAggregate
+               Group Key: pagg_tab_p3_s2.b
+               ->  Seq Scan on pagg_tab_p3_s2
+(9 rows)
+
+SELECT b, sum(a), count(*) FROM pagg_tab_p3 GROUP BY b ORDER BY 1, 2, 3;
+ b | sum  | count
+---+------+-------
+ 0 | 2000 |   100
+ 1 | 2100 |   100
+ 2 | 2200 |   100
+ 3 | 2300 |   100
+ 4 | 2400 |   100
+ 5 | 2500 |   100
+ 6 | 2600 |   100
+ 7 | 2700 |   100
+ 8 | 2800 |   100
+ 9 | 2900 |   100
+(10 rows)

We should just remove this case, it's same as testing top-level partitioned
tables.

+
+-- Full aggregation as GROUP BY clause matches with PARTITION KEY
+EXPLAIN (COSTS OFF)
+SELECT a, sum(b), array_agg(distinct c), count(*) FROM pagg_tab GROUP
BY a, b HAVING avg(b) < 3 ORDER BY 1, 2, 3;
+                                      QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_p1.a, (sum(pagg_tab_p1.b)), (array_agg(DISTINCT
pagg_tab_p1.c))
+   ->  Append
+         ->  GroupAggregate
+               Group Key: pagg_tab_p1.a, pagg_tab_p1.b
+               Filter: (avg(pagg_tab_p1.b) < '3'::numeric)
+               ->  Sort
+                     Sort Key: pagg_tab_p1.a, pagg_tab_p1.b
+                     ->  Seq Scan on pagg_tab_p1
+         ->  GroupAggregate
+               Group Key: pagg_tab_p2_s1.a, pagg_tab_p2_s1.b
+               Filter: (avg(pagg_tab_p2_s1.b) < '3'::numeric)
+               ->  Sort
+                     Sort Key: pagg_tab_p2_s1.a, pagg_tab_p2_s1.b
+                     ->  Append
+                           ->  Seq Scan on pagg_tab_p2_s1
+                           ->  Seq Scan on pagg_tab_p2_s2
+         ->  GroupAggregate
+               Group Key: pagg_tab_p3_s1.a, pagg_tab_p3_s1.b
+               Filter: (avg(pagg_tab_p3_s1.b) < '3'::numeric)
+               ->  Sort
+                     Sort Key: pagg_tab_p3_s1.a, pagg_tab_p3_s1.b
+                     ->  Append
+                           ->  Seq Scan on pagg_tab_p3_s1
+                           ->  Seq Scan on pagg_tab_p3_s2
+(25 rows)

Instead of an Append node appearing under GroupAggregate, I think we should
flatten all the partition scans for the subpartitions whose partition keys are
part of group keys and add GroupAggregate on top of each of such partition
scans.

+-- Parallelism within partition-wise aggregates
+RESET max_parallel_workers_per_gather;
+SET min_parallel_table_scan_size TO '8kB';
+SET parallel_setup_cost TO 0;
+INSERT INTO pagg_tab_para SELECT i%30, i%20 FROM generate_series(0, 29999) i;

spaces around % operator?

+SHOW max_parallel_workers_per_gather;
+ max_parallel_workers_per_gather
+---------------------------------
+ 2

Why do we need this?

+
+-- When GROUP BY clause matches with PARTITION KEY.
+EXPLAIN (COSTS OFF)
+SELECT x, sum(y), avg(y), count(*) FROM pagg_tab_para GROUP BY x
HAVING avg(y) < 7 ORDER BY 1, 2, 3;
+                                      QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_para_p1.x, (sum(pagg_tab_para_p1.y)),
(avg(pagg_tab_para_p1.y))
+   ->  Append
[ ... clipped ...]
+         ->  Finalize GroupAggregate
+               Group Key: pagg_tab_para_p3.x
+               Filter: (avg(pagg_tab_para_p3.y) < '7'::numeric)
+               ->  Sort
+                     Sort Key: pagg_tab_para_p3.x
+                     ->  Gather
+                           Workers Planned: 2
+                           ->  Partial HashAggregate
+                                 Group Key: pagg_tab_para_p3.x
+                                 ->  Parallel Seq Scan on pagg_tab_para_p3
[ ... clipped ... ]
+-- When GROUP BY clause not matches with PARTITION KEY.
+EXPLAIN (COSTS OFF)
+SELECT y, sum(x), avg(x), count(*) FROM pagg_tab_para GROUP BY y
HAVING avg(x) < 12 ORDER BY 1, 2, 3;
+                                      QUERY PLAN
+--------------------------------------------------------------------------------------
+ Sort
+   Sort Key: pagg_tab_para_p1.y, (sum(pagg_tab_para_p1.x)),
(avg(pagg_tab_para_p1.x))
+   ->  Finalize HashAggregate
+         Group Key: pagg_tab_para_p1.y
[ ... clipped ... ]
+               ->  Gather
+                     Workers Planned: 2
+                     ->  Partial HashAggregate
+                           Group Key: pagg_tab_para_p3.y
+                           ->  Parallel Seq Scan on pagg_tab_para_p3

Per a prior discussion on this thread, we shouldn't produce such plans;
Parallel Append instead?

+SET enable_partition_wise_agg to true;

May be just enable it at the beginning instead of enabling and disabling twice?

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

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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
> +         Sort Key: pagg_tab_p1.a
> +         ->  Group
> +               Group Key: pagg_tab_p1.a
> +               ->  Sort
> +                     Sort Key: pagg_tab_p1.a
> +                     ->  Seq Scan on pagg_tab_p1
> [ ... clipped ... ]
> +(19 rows)
>
> It's strange that we do not annotate partial grouping as Partial. Does not look
> like a bug in your patch. Do we get similar output with parallel grouping?

I am wrong here. It's not partial grouping. It's two level grouping. I
think annotating Group as Partial would be misleading. Sorry.


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

On Thu, Dec 14, 2017 at 4:01 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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.c
> +   ->  Result
> +         One-Time Filter: false
> +(4 rows)
>
> Not part of your patch, I am wondering if we can further optimize this plan by
> converting HashAggregate to Result (One-time Filter: false) and the aggregate
> target. Just an idea.
>
This comment is also wrong. The finalization step of aggregates needs
to be executed irrespective of whether or not the underlying scan
produces any rows. It may, for example, add a constant value to the
transition result. We may apply this optimization only when none of
aggregations have finalization functions, but it may not be worth the
code.

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

On Thu, Jan 11, 2018 at 6:00 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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
relation with 0007.  I find the way that the refactoring has been done
slightly odd.  With 0001 and 0002 applied, we end up with three
functions for creating aggregate paths: create_partial_agg_path, which
handles the partial-path case for both sort and hash;
create_sort_agg_path, which handles the sort case for non-partial
paths only; and create_hash_agg_path, which handles the hash case for
non-partial paths only.  This leads to the following code in 0007:

+               /* Full grouping paths */
+
+               if (try_parallel_aggregation)
+               {
+                       Assert(extra->agg_partial_costs &&
extra->agg_final_costs);
+                       create_partial_agg_path(root, input_rel,
grouped_rel, target,
+
 partial_target, extra->agg_partial_costs,
+
 extra->agg_final_costs, gd, can_sort,
+
 can_hash, (List *) extra->havingQual);
+               }
+
+               if (can_sort)
+                       create_sort_agg_path(root, input_rel,
grouped_rel, target,
+
partial_target, agg_costs,
+
extra->agg_final_costs, gd, can_hash,
+
dNumGroups, (List *) extra->havingQual);
+
+               if (can_hash)
+                       create_hash_agg_path(root, input_rel,
grouped_rel, target,
+
partial_target, agg_costs,
+
extra->agg_final_costs, gd, dNumGroups,
+                                                                (List
*) extra->havingQual);

That looks strange -- you would expect to see either "sort" and "hash"
cases here, or maybe "partial" and "non-partial", or maybe all four
combinations, but seeing three things here looks surprising.  I think
the solution is just to create a single function that does both the
work of create_sort_agg_path and the work of create_hash_agg_path
instead of having two separate functions.

A related thing that is also surprising is that 0007 manages to reuse
create_partial_agg_path for both the isPartialAgg and non-isPartialAgg
cases -- in fact, the calls appear to be identical, and could be
hoisted out of the "if" statement -- but create_sort_agg_path and
create_hash_agg_path do not get reused.  I think you should see
whether you can define the new combo function that can be used for
both cases.  The logic looks very similar, and I'm wondering why it
isn't more similar than it is; for instance, create_sort_agg_path
loops over the input rel's pathlist, but the code for
isPartialAgg/can_sort seems to consider only the cheapest path.  If
this is correct, it needs a comment explaining it, but I don't see why
it should be correct.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 16, 2018 at 3:56 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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(), similar to what commit
c44c47a773bd9073012935a29b0264d95920412c did with
add_paths_to_append_rel().

> Oops. My mistake. Missed. We should loop over the input rel's pathlist.
>
> Yep. With above change, the logic is very similar except
> (1) isPartialAgg/can_sort case creates the partial paths and
> (2) finalization step is not needed at this stage.

I'm not sure what you mean by #1.

> I think it can be done by passing a flag to create_sort_agg_path() (or new
> combo function) and making appropriate adjustments. Do you think addition of
> this new flag should go in re-factoring patch or main PWA patch?
> I think re-factoring patch.

I think the refactoring patch should move the existing code into a new
function without any changes, and then the main patch should add an
additional argument to that function that allows for either behavior.

By the way, I'm also a bit concerned about this:

+               /*
+                * For full aggregation, we are done with the partial
paths.  Just
+                * clear it out so that we don't try to create a
parallel plan over it.
+                */
+               grouped_rel->partial_pathlist = NIL;

I think that's being done for the same reason as mentioned at the
bottom of the current code for create_grouping_paths().  They are only
partially aggregated and wouldn't produce correct final results if
some other planning step -- create_ordered_paths, or the code that
sets up final_rel -- used them as if they had been fully agggregated.
I'm worried that there might be an analogous danger for partition-wise
aggregation -- that is, that the paths being inserted into the partial
pathlists of the aggregate child rels might get reused by some later
planning step which doesn't realize that the output they produce
doesn't quite match up with the rel to which they are attached.  You
may have already taken care of that problem somehow, but we should
make sure that it's fully correct and clearly commented.  I don't
immediately see why the isPartialAgg case should be any different from
the !isPartialAgg case.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jan 18, 2018 at 8:55 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 29, 2018 at 3:42 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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
OtherUpperPathExtraData *extra instead of adding them to the path list
of some RelOptInfo?  If we had an appropriate RelOptInfo to which we
could add the paths, then we could make this simpler.

If I understand correctly, the reason you're doing it this way is
because we have no place to put partially-aggregated, non-partial
paths.  If we only needed to worry about the parallel query case, we
could just build an append of partially-aggregated paths for each
child and stick it into the grouped rel's partial pathlist, just as we
already do for regular parallel aggregation.  There's no reason why
add_paths_to_grouping_rel() needs to care about the difference a
Partial Aggregate on top of whatever and an Append each branch of
which is a Partial Aggregate on top of whatever.  However, this won't
work for non-partial paths, because add_paths_to_grouping_rel() needs
to put paths into the grouped rel's pathlist -- and we can't mix
together partially-aggregated paths and fully-aggregated paths in the
same path list.

But, really, the way we're using grouped_rel->partial_pathlist right
now is an awful hack.  What I'm thinking we could do is introduce a
new UpperRelationKind called UPPERREL_PARTIAL_GROUP_AGG, coming just
before UPPERREL_GROUP_AGG.  Without partition-wise aggregate,
partially_grouped_rel's pathlist would always be NIL, and its partial
pathlist would be constructed using the logic in
add_partial_paths_to_grouping_rel, which would need renaming.  Then,
add_paths_to_grouping_rel would use paths from input_rel when doing
non-parallel aggregation and paths from partially_grouped_rel when
doing parallel aggregation.  This would eliminate the ugly
grouped_rel->partial_pathlist = NIL assignment at the bottom of
create_grouping_paths(), because the grouped_rel's partial_pathlist
would never have been (bogusly) populated in the first place, and
hence would not need to be reset.  All of these changes could be made
via a preparatory patch.

Then the main patch needs to worry about four cases:

1. Parallel partition-wise aggregate, grouping key doesn't contain
partition key.  This should just be a matter of adding additional
Append paths to partially_grouped_rel->partial_pathlist.  The existing
code already knows how to stick a Gather and FinalizeAggregate step on
top of that, and I don't see why that logic would need any
modification or addition.  An Append of child partial-grouping paths
should be producing the same output as a partial grouping of an
Append, except that the former case might produce more separate groups
that need to be merged; but that should be OK: we can just throw all
the paths into the same path list and let the cheapest one win.

2. Parallel partition-wise aggregate, grouping key contains partition
key.  For the most part, this is no different from case #1.  We won't
have groups spanning different partitions in this case, but we might
have groups spanning different workers, so we still need a
FinalizeAggregate step.  As an exception, Gather -> Parallel Append ->
[non-partial Aggregate path] would give us a way of doing aggregation
in parallel without a separate Finalize step.  I'm not sure if we want
to consider that to be in scope for this patch.  If we do, then we'd
add the Parallel Append path to grouped_rel->partial_pathlist.  Then,
we could stick Gather (Merge) on top if it to produce a path for
grouped_rel->pathlist using generate_gather_paths(); alternatively, it
can be used by upper planning steps -- something we currently can't
ever make work with parallel aggregation.

3. Non-parallel partition-wise aggregate, grouping key contains
partition key.  Build Append paths from the children of grouped_rel
and add them to grouped_rel->pathlist.

3. Non-parallel partition-wise aggregate, grouping key doesn't contain
partition key.  Build Append paths from the children of
partially_grouped_rel and add them to partially_grouped_rel->pathlist.
Also add code to generate paths for grouped_rel->pathlist by sticking
a FinalizeAggregate step on top of each path from
partially_grouped_rel->pathlist.

Overall, what I'm trying to argue for here is making this feature look
less like its own separate thing and more like part of the general
mechanism we've already got: partial paths would turn into regular
paths via generate_gather_paths(), and partially aggregated paths
would turn into fully aggregated paths by adding FinalizeAggregate.
The existing special case that allows us to build a non-partial, fully
aggregated path from a partial, partially-aggregated path would be
preserved.

I think this would probably eliminate some other problems I see in the
existing design as well.  For example, create_partition_agg_paths
doesn't consider using Gather Merge, but that might be a win.  With
the design above, I think you never need to call create_gather_path()
anywhere.  In case #1, the existing code takes care of it.  In the
special case mentioned under #2, if we chose to support that,
generate_gather_paths() would take care of it.  Both of those places
already know about Gather Merge.

On another note, I found preferFullAgg to be wicked confusing.  To
"prefer" something is to like it better, but be willing to accept
other options if the preference can't be accommodated.  Here, it seems
like preferFullAgg = false prevents consideration of full aggregation.
So it's really more like allowFullAgg, or, maybe better,
try_full_aggregation.  Also, try_partition_wise_grouping has a
variable isPartialAgg which is always ends up getting set to
!preferFullAgg.  Having two Boolean variables which are always set to
the opposite of each other isn't good.  To add to the confusion, the
code following the place where isPartialAgg is set sometimes refers to
isPartialAgg and sometimes refers to preferFullAgg.

I think the comments in this patch still need a good bit of work.
They tend to explain what the code does rather than the reason it does
it, and they tend to speak vaguely rather than precisely about things
happening in other places.  For example, consider the need to set
partial_pathlist = NIL in create_grouping_paths(). Here's the existing
comment:

    /*
     * We've been using the partial pathlist for the grouped relation to hold
     * partially aggregated paths, but that's actually a little bit bogus
     * because it's unsafe for later planning stages -- like ordered_rel ---
     * to get the idea that they can use these partial paths as if they didn't
     * need a FinalizeAggregate step.  Zap the partial pathlist at this stage
     * so we don't get confused.
     */

Here's your comment about the same hazzard:

+        * For full aggregation, at this point we are already done with the
+        * finalization step and thus partial paths are no more needed. Keeping
+        * those will lead to some unwanted result later in the planning stage.
+        * Thus like create_grouping_paths(), clear them out.

Notice that your comment says that it will created an "unwanted
result" and that this result will happen "later", whereas the existing
comment is a lot more specific.  It says exactly what the problem is
(FinalizeAggregate needed) and where the confusion will happen
(ordered_rel).  Some other examples of comments with similar problems:

+        * If there are partial subpaths for parallelism, then we need to add
+        * gather path on top of the append. However, we only do this when full
+        * aggregation is required.  For partial aggregation this can be done at
+        * later stage.

Doesn't really explain why we're doing any of those things, just says
that we are.  Also, what later stage?

+        * For non-partial aggregation path, just need to add given
append path to
+        * a grouped_rel.  Also, if caller requested a partial aggregation only,
+        * skip finalize step.

Again, why?

+        * Add all collected append paths into the grouped_rel.  For partial
+        * aggregation mode we need to add a finalize agg node over an append
+        * path.

Why?

+       /* Similarly, for partial paths.  Here we need to add gather
node too. */

Why?

+                * However, in partial aggregation mode this is done
at later stage,
+                * so skip it.

When?

Here's an example of a much better comment you wrote:

+        * In find_ec_member_for_tle(), child EC members are ignored
if they don't
+        * belong to the given relids. Thus, if this sort path is
based on a child
+        * relation, we must pass the relids of it. Otherwise, we will
end-up into
+        * an error requiring pathkey item.

I haven't studied this patch in enough depth yet to figure out whether
I think that makes sense, but clearly when I go to do that this
comment is going to be a big help in figuring it out.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Feb 1, 2018 at 8:59 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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_pathlist, which should no longer be necessary; also, it needs
substantial comment updates).

>> 1. Parallel partition-wise aggregate, grouping key doesn't contain
>> partition key.  This should just be a matter of adding additional
>> Append paths to partially_grouped_rel->partial_pathlist.  The existing
>> code already knows how to stick a Gather and FinalizeAggregate step on
>> top of that, and I don't see why that logic would need any
>> modification or addition.  An Append of child partial-grouping paths
>> should be producing the same output as a partial grouping of an
>> Append, except that the former case might produce more separate groups
>> that need to be merged; but that should be OK: we can just throw all
>> the paths into the same path list and let the cheapest one win.
>
> For any partial aggregation we need to add finalization step after we are
> done with the APPEND i.e. post add_paths_to_append_rel(). Given that we need
> to replicate the logic of sticking Gather and FinalizeAggregate step at
> later stage. This is what exactly done in create_partition_agg_paths().
> Am I missing something here?

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.  Both of those places except to take a
partial path that has been partially aggregated and produce a
non-partial path that is fully aggregated.  We do not need or want two
copies of that code.

Here's another way to look at it.  We have four kinds of things.

1. Partially aggregated partial paths
2. Partially aggregated non-partial paths
3. Fully aggregated partial paths
4. Fully aggregated non-partial paths

The current code only ever generates paths of type #1 and #4; this
patch will add paths of type #2 as well, and maybe also type #3.  But
the way you've got it, the existing paths of type #1 go into the
grouping_rel's partial_pathlist, and the new paths of type #1 go into
the OtherUpperPathExtraData's partial_paths list.  Maybe there's a
good reason why we should keep them separate, but I'm inclined to
think they should all be going into the same list.

>> Overall, what I'm trying to argue for here is making this feature look
>> less like its own separate thing and more like part of the general
>> mechanism we've already got: partial paths would turn into regular
>> paths via generate_gather_paths(), and partially aggregated paths
>> would turn into fully aggregated paths by adding FinalizeAggregate.
>> The existing special case that allows us to build a non-partial, fully
>> aggregated path from a partial, partially-aggregated path would be
>> preserved.
>>
>> I think this would probably eliminate some other problems I see in the
>> existing design as well.  For example, create_partition_agg_paths
>> doesn't consider using Gather Merge, but that might be a win.
>
> Append path is always non-sorted and has no pathkeys. Thus Gather Merge over
> an Append path seems infeasible, isn't it?

We currently never generate an Append path with pathkeys, but we do
generate MergeAppend paths with pathkeys, as in the following example:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
rhaas=# create index on foo (a);
CREATE INDEX
rhaas=# create table foo1 partition of foo for values from (0) to (1000000);
CREATE TABLE
rhaas=# create table foo2 partition of foo for values from (1000000)
to (2000000);
CREATE TABLE
rhaas=# select * from foo foo order by a;
 a | b
---+---
(0 rows)
rhaas=# explain select * from foo foo order by a;
                                       QUERY PLAN
----------------------------------------------------------------------------------------
 Merge Append  (cost=0.32..145.47 rows=2540 width=36)
   Sort Key: foo.a
   ->  Index Scan using foo1_a_idx on foo1 foo  (cost=0.15..63.20
rows=1270 width=36)
   ->  Index Scan using foo2_a_idx on foo2 foo_1  (cost=0.15..63.20
rows=1270 width=36)
(4 rows)

Actually, in this example, the MergeAppend could be safely converted
into an Append, because the partitions are in bound order, and
somebody already proposed a patch for that.

The point is that we want to be able to get plans like this:

Finalize GroupAggregate
-> Gather Merge
  -> MergeAppend
    -> Partial GroupAggregate
      -> Parallel Index Scan on t1
    -> Partial GroupAggregate
      -> Parallel Index Scan on t2
    -> Partial GroupAggregate
      -> Parallel Index Scan on t3

If we only consider Gather, not Gather Merge, when turning a partially
aggregated partial path into a non-partial path, then we end up having
to insert a Sort node if we want to perform a Finalize GroupAggregate
step.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 2, 2018 at 8:25 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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.  Both of those places except to take a
>> partial path that has been partially aggregated and produce a
>> non-partial path that is fully aggregated.  We do not need or want two
>> copies of that code.
>
> OK. Got it.
>
> Will try to find a common place for them and will also check how it goes
> with your suggested design change.
>
>> Here's another way to look at it.  We have four kinds of things.
>>
>> 1. Partially aggregated partial paths
>> 2. Partially aggregated non-partial paths
>> 3. Fully aggregated partial paths
>> 4. Fully aggregated non-partial paths

So in the new scheme I'm proposing, you've got a partially_grouped_rel
and a grouped_rel.  So all paths of type #1 go into
partially_grouped_rel->partial_pathlist, paths of type #2 go into
partially_grouped_rel->pathlist, type #3 (if we have any) goes into
grouped_rel->partial_pathlist, and type #4 goes into
grouped_rel->pathlist.

> add_paths_to_append_rel() -> generate_mergeappend_paths() does not consider
> partial_pathlist. Thus we will never see MergeAppend over parallel scan
> given by partial_pathlist. And thus plan like:
> -> Gather Merge
>   -> MergeAppend
> is not possible with current HEAD.
>
> Are you suggesting we should implement that here? I think that itself is a
> separate task.

Oh, I didn't realize that wasn't working already.  I agree that it's a
separate task from this patch, but it's really too bad that it doesn't
already work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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 Thu, Feb 8, 2018 at 8:05 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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, rather
than in addition to, cpu_operator_cost; and [3] I modified the
regression test so that the overall plan shape didn't change.

[2] was proposed upthread, but not adopted.  I had the same thought
while reading the patch (having forgotten the previous discussion) and
that seemed like a good enough reason to do it according to the
previous proposal.  If there is a good reason to think MergeAppend
needs that extra cost increment to be fairly-costed, I don't see it on
this thread.

[3] was also remarked upon upthread -- Ashutosh mentioned that the
change in plan shape was "sad" but there was no further discussion of
the matter.  I also found it sad; hence the change.  This is, by the
way, an interesting illustration of how partition-wise join could
conceivably lose.  Up until now I've thought that it seemed to be a
slam dunk to always win or at least break even, but if you've got a
relatively unselective join, such that the output is much larger than
either input, then doing the join partition-wise means putting all of
the output rows through an Append node, whereas doing it the normal
way means putting only the input rows through Append nodes.  If the
smaller number of rows being joined at one time doesn't help -- e.g.
all of the inner rows across all partitions fit in a tiny little hash
table -- then we're just feeding more rows through the Append for no
gain.  Not a common case, perhaps, but not impossible.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Feb 8, 2018 at 8:05 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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 version is that this now uses generate_gather_paths() to add
Gather/Gather Merge nodes (except in the case where we sort by group
pathkeys and then Gather Merge) rather than keeping all of the bespoke
code.  That turned up to be a bit less elegant than I would have liked
-- I had to an override_rows argument to generate_gather_paths to make
it work.  But overall I think this is still a big improvement, since
it lets us share code instead of duplicating it.  Also, it potentially
lets us add partially-aggregated but non-parallel paths into
partially_grouped_rel->pathlist and that should Just Work; they will
get the Finalize Aggregate step but not the Gather.  With your
arrangement that wouldn't work.

Please review/test.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 26, 2018 at 6:38 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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
fields, not everything, and copying those fields is cheap, because
they are just simple assignment statements.  I think memcpy()'ing the
whole structure would be using a sledgehammer to solve a problem for
which a scalpel is more suited.

> Paths in pathlist are added by add_path(). Though we have paths is pathlist
> is sorted with the cheapest total path, we generally use
> RelOptInfo->cheapest_total_path instead of using first entry, unlike partial
> paths. But here you use the first entry like partial paths case. Will it
> better to use cheapest total path from partially_grouped_rel? This will
> require calling set_cheapest on partially_grouped_rel before we call this
> function.

Hmm, I guess that seems like a reasonable approach, although I am not
sure it matters much either way.

> Attached top-up patch doing this along with few indentation fixes.

I don't see much point to the change in generate_gather_paths -- that
line is only 77 characters long.

Committed after incorporating your other fixes and updating the
optimizer README.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Feb 27, 2018 at 4:29 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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
> partial path from the input_rel to create a Gather Merge; instead we need a
> cheapest partial path from the partially_grouped_rel.
>
> Attached fix_aggref_in_non-agg_error.patch fixing this.

Oops.  Thanks, committed.

> test_for_aggref_in_non-agg_error.patch has a testcase reported by Rajkumar
> which I have added in a aggregates.sql.

Didn't commit this; I think that's overkill.

> While doing so, I have observed few cleanup changes, added those in
> misc_cleanup.patch.

Committed those.

> While re-basing my partitionwise aggregate changes, I observed that when we
> want to create partial aggregation paths for a child partition, we don't
> need to add Gather or Gather Merge on top of it as we first want to append
> them all and then want to stick a gather on it. So it will be better to have
> that code part in a separate function so that we can call it from required
> places.
>
> I have attached patch (create_non_partial_paths.patch) for it including all
> above fix.

I don't like that very much.  For one thing, the name
create_non_partial_paths() is not very descriptive at all.  For
another thing, it somewhat renders add_paths_to_partial_grouping_rel()
a misnomer, as that function then adds only partial paths.  I think
what you should just do is have the main patch add a test for
rel->reloptkind == RELOPT_UPPER_REL; if true, add the Gather paths; if
not, skip it.  Then it will be skipped for RELOPT_OTHER_UPPER_REL
which is what we want.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 1, 2018 at 5:34 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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 node needs
+        * to be preserved as adding new partial paths in same rel may
delete this
+        * referenced path. To do this we need to clear the
partial_pathlist from
+        * the partially_grouped_rel as we may add partial paths again
while doing
+        * partitionwise aggregation. Keeping older partial path intact seems
+        * reasonable too as it might possible that the final path
chosen which is
+        * using it wins, but the underneath partial path is not the
cheapest one.

This isn't a good design.  You shouldn't create a Gather or Gather
Merge node until all partial paths have been added.  I mean, the point
is to put a Gather node on top of the cheapest path, not the path that
is currently the cheapest but might not actually be the cheapest once
we've added them all.

+add_gather_or_gather_merge(PlannerInfo *root,

Please stop picking generic function names for functions that have
very specific purposes.  I don't really think that you need this to be
a separate function at all, but it it is certainly NOT a
general-purpose function for adding a Gather or Gather Merge node.

+        /*
+         * Collect statistics about aggregates for estimating costs of
+         * performing aggregation in parallel or in partial, if not already
+         * done. We use same cost for all the children as they will be same
+         * anyways.
+         */

If it only needs to be done once, do we really have to have it inside
the loop?  I see that you're using the varno-translated
partial_target->exprs and target->exprs, but if the specific varnos
don't matter, why not just use the untranslated version of the targets
before entering the loop?  And if the specific varnos do matter, then
presumably you need to do it every time.

This is not a full review, but I'm out of time for right now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 1, 2018 at 4:52 PM, Robert Haas <robertmhaas@gmail.com> 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 reuse the code instead of
duplicating it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Mar 5, 2018 at 3:56 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> 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 doing this, we will be having all partial paths in
> partially_grouped_rel and then existing code will do required finalization
> along with any Gather or Gather Merge, if required.
>
> Please have a look over attached patch-set and let me know if it needs
> further changes.

This does look better.

+      <term><varname>enable_partitionwise_agg</varname> (<type>boolean</type>)

Please don't abbreviate "aggregate" to "agg".

-       /* Build final grouping paths */
-       add_paths_to_grouping_rel(root, input_rel, grouped_rel, target,
-
partially_grouped_rel, agg_costs,
-
&agg_final_costs, gd, can_sort, can_hash,
-                                                         dNumGroups,
(List *) parse->havingQual);
+       if (isPartialAgg)
+       {
+               Assert(agg_partial_costs && agg_final_costs);
+               add_paths_to_partial_grouping_rel(root, input_rel,
+
           partially_grouped_rel,
+
           agg_partial_costs,
+
           gd, can_sort, can_hash,
+
           false, true);
+       }
+       else
+       {
+               double          dNumGroups;
+
+               /* Estimate number of groups. */
+               dNumGroups = get_number_of_groups(root,
+
           cheapest_path->rows,
+
           gd,
+
           child_data ? make_tlist_from_pathtarget(target) :
parse->targetList);
+
+               /* Build final grouping paths */
+               add_paths_to_grouping_rel(root, input_rel, grouped_rel, target,
+
partially_grouped_rel, agg_costs,
+
agg_final_costs, gd, can_sort, can_hash,
+
dNumGroups, (List *) havingQual);
+       }

This looks strange.  Why do we go down two completely different code
paths here?  It seems to me that the set of paths we add to the
partial_pathlist shouldn't depend at all on isPartialAgg.  I might be
confused, but it seems to me that any aggregate path we construct that
is going to run in parallel must necessarily be partial, because even
if each group will occur only in one partition, it might still occur
in multiple workers for that partition, so finalization would be
needed.  On the other hand, for non-partial paths, we can add then to
partially_grouped_rel when isPartialAgg = true and to grouped_rel when
isPartialAgg = false, with the only difference being AGGSPLIT_SIMPLE
vs. AGGSPLIT_INITIAL_SERIAL.  But that doesn't appear to be what this
is doing.

+       /*
+        * If there are any fully aggregated partial paths present,
may be because
+        * of parallel Append over partitionwise aggregates, we must stick a
+        * Gather or Gather Merge path atop the cheapest partial path.
+        */
+       if (grouped_rel->partial_pathlist)

This comment is copied from someplace where the code does what the
comment says, but here it doesn't do any such thing.

More tomorrow...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company