Обсуждение: Aggregate Push Down - Performing aggregation on foreign server

On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> It is pity - lot of performance issues are related to this missing feature.

I don't think you are being very clear about what feature you are
talking about.  The feature that Jeevan has implemented is pushing
aggregates to the remote side when postgres_fdw is in use.  The
feature you are talking about is evidently something else, but you
haven't really said what it is.  Or not in a way that I can
understand.

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

On 2016/08/31 16:42, Pavel Stehule wrote:
> 2016-08-31 9:00 GMT+02:00 Robert Haas <robertmhaas@gmail.com>:
> 
>> On Wed, Aug 31, 2016 at 11:56 AM, Pavel Stehule <pavel.stehule@gmail.com>
>> wrote:
>>> It is pity - lot of performance issues are related to this missing
>> feature.
>>
>> I don't think you are being very clear about what feature you are
>> talking about.  The feature that Jeevan has implemented is pushing
>> aggregates to the remote side when postgres_fdw is in use.  The
>> feature you are talking about is evidently something else, but you
>> haven't really said what it is.  Or not in a way that I can
>> understand.
>>
> 
> yes, It is not clear if FDW aggregate push down can be implemented together
> with internal aggregate push down. Aggregate push down ~ try to aggregate
> first when it is possible

What do you mean by "internal aggregate push down"?  Partition-wise
aggregation?  Aggregate/group by before join (something like [1])?  IIUC,
what Jeevan proposes in this thread is to implement the aggregate push
down API that is specific to FDWs in postgres_fdw.  Any other push down
work would need to use different APIs and would need to separately
proposed/discussed.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAKJS1f9kw95K2pnCKAoPmNw==7fgjSjC-82cy1RB+-x-Jz0QHA@mail.gmail.com

On Fri, Sep 16, 2016 at 9:45 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result.

+1.

> Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.

I don't think that'd be appropriate.  The user is calling the
aggregate, not its constituent functions.

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

Thanks Jeevan for working on the comments.

>
> 3. classifyConditions() assumes list expressions of type RestrictInfo. But
> HAVING clause expressions (and JOIN conditions) are plain expressions. Do
> you mean we should modify the classifyConditions()? If yes, then I think it
> should be done as a separate (cleanup) patch.

Ok. Yes, we should also handle bare conditions in
classifyConditions(). I am fine doing it as a separate patch.


>
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result. Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.

Ok. I agree with this analysis.


> 13. Fixed. However instead of adding new function made those changes inline.
> Adding support for deparsing List expressions separating list by comma can
> be
> considered as cleanup patch as it will touch other code area not specific to
> aggregate push down.

Ok.

>
> 18. I think re-factoring estimate_path_cost_size() should be done separately
> as a cleanup patch too.

Ok.

>
> 29. I have tried passing input rel's relids to fetch_upper_rel() call in
> create_grouping_paths(). It solved this patch's issue, but ended up with
> few regression failures which is mostly plan changes. I am not sure whether
> we get good plan now or not as I have not analyzed them.
> So for this patch, I am setting relids in add_foreign_grouping_path()
> itself.
> However as suggested, used bms_copy(). I still kept the FIXME over there as
> I think it should have done by the core itself.

I don't think add_foreign_grouping_path() is the right place to change
a structure managed by the core and esp when we are half-way adding
paths. An FDW should not meddle with an upper RelOptInfo. Since
create_foreignscan_plan() picks those up from RelOptInfo and both of
those are part of core, we need a place in core to set the
RelOptInfo::relids for an upper relation OR we have stop using
fs_relids for upper relation foreign scans.

Here are some more comments on the patch, mostly focused on the tests.
1. If we dig down each usage of deparse_expr_cxt::scanrel, it boils down
checking whether a given Var comes from given scan relation (deparseVar()) or
checking whether a given Var needs relation qualification while deparsing
(again deparseVar()). I think both of those purposes can be served by looking
at scanrel::relids, multiple relids implying relation qualification. So,
instead of having a separate member scan_rel, we should instead fetch the
relids from deparse_expr_cxt::foreignrel. I am assuming that the proposed
change to set relids in upper relations is acceptable. If it's not, we should
pass scanrel->relids through the context instead of scanrel itself.

2. SortGroupClause is a parser node, so we can name appendSortGroupClause() as
deparseSortGroupClause(), to be consistent with the naming convention. If we do
that change, may be it's better to pass SortGroupClause as is and handle other
members of that structure as well.

3. Following block may be reworded
+                /*
+                 * aggorder too present into args so no need to check its
+                 * shippability explicitly.  However, if any expression has
+                 * USING clause with sort operator, we need to make sure the
+                 * shippability of that operator.
+                 */

as "For aggorder elements, check whether the sort operator, if specified, is
shippable or not." and mention aggorder expression along with aggdirectargs in
the comment before this one.

4. Following line is correct as long as there is only one upper relation.
+    context.scanrel = (rel->reloptkind == RELOPT_UPPER_REL) ?
fpinfo->outerrel : rel;
But as we push down more and more of upper operation, there will be a chain of
upper relations on top of scan relation. So, it's better to assert that the
scanrel is a join or a base relation to be future proof.

5. In deparseConst(), showtype is compared with hardcoded values. The
callers of this function pass hardcoded values. Instead, we should
define an enum and use it. I understand that this logic has been borrowed from
get_const_expr(), which also uses hardcoded values. Any reason why not to adopt
a better style here? In fact, it only uses two states for showtype, 0 and ">
0". Why don't we use a boolean then OR why isn't the third state in
get_const_expr() applicable here?

6. "will" or "should" is missing from the following sentence.
"Plain var nodes either be same as some GROUP BY expression or part of some
GROUP BY expression.

7. The changes in block           else           {               /*                * If we have sortgroupref set, then
itmeans that we have an                * ORDER BY entry pointing to this expression.  Since we are                * not
pushingORDER BY with GROUP BY, clear it.                */               if (sgref)
grouping_target->sortgrouprefs[i]= 0;
 
               /* Not matched exactly, pull the var with aggregates then */               aggvars =
pull_var_clause((Node*) expr,                                         PVC_INCLUDE_AGGREGATES);
 
               if (!is_foreign_expr(root, grouped_rel, (Expr *) aggvars))                   return false;
               /*                * Add aggregates, if any, into the targetlist.  Plain var                * nodes
eitherbe same as some GROUP BY expression or part of                * some GROUP BY expression.  In later case, the
querycannot                * refer plain var nodes without the surrounding expression.                * In both the
cases,they are already part of the targetlist                * and thus no need to add them again.  In fact adding
pulled               * plain var nodes in SELECT clause will cause an error on the                * foreign server if
theyare not same as some GROUP BY                * expression.                */               foreach(l, aggvars)
        {                   Expr   *expr = (Expr *) lfirst(l);
 
                   if (IsA(expr, Aggref))                       tlist = add_to_flat_tlist(tlist, list_make1(expr));
         }           }
 

and those in the block   if (fpinfo->local_conds)   {       ListCell   *lc;       List       *aggvars =
pull_var_clause((Node*) fpinfo->local_conds,                                             PVC_INCLUDE_AGGREGATES);
 
       foreach(lc, aggvars)       {           Expr       *expr = (Expr *) lfirst(lc);
           /*            * If aggregates within local conditions are not safe to push down,            * then we cannot
pushdown the query.  Vars are already part of            * GROUP BY clause which are checked above, so no need to
access           * them again here.            */           if (IsA(expr, Aggref) && !is_foreign_expr(root,
grouped_rel,expr))               return false;       }
 
       /* Add Vars and aggregates into the target list. */       tlist = add_to_flat_tlist(tlist, aggvars);   }
should be in sync. The later block adds both aggregates and Vars but the first
one doesn't. Why is this difference?

8.  In the comment, "If input rel is not safe to pushdown, then simply return
as we cannot perform any post-join operations remotely on it.", "remotely on
it" may be rephrased as "on the foreign server." to be consistent with the
terminology at in the file.

9. EXPLAIN of foreign scan is adding paranthesis around output expressions,
which is not needed. EXPLAIN output of other plan nodes like Aggregate doesn't
do that. If it's easy to avoid paranthesis, we should do it in this patch. With
this patch, we have started pushing down expressions in the target list, so
makes sense to fix it in this patch.

10. While deparsing the GROUP BY clauses, the code fetches the expressions and
deparses the expressions. We might save some CPU cycles if we deparse the
clauses by their positional indexes in SELECT clause instead of the actual
expressions. This might make the query slightly unreadable. What do you think?

Comments about tests:

1. I don't think the comment below is required. Someone wants to know the
status of that flag might read the corresponding CREATE TABLE command.
+-- Both ft1 and ft2 are used to exercise cost estimates when
+-- use_remote_estimate is false and true respectively.

2. I am assuming that you will remove the SQL statements involving local
tables. They are being used only to test the outputs.

3. The test select count(c6) from ft1; may be clubbed with a previous
testcase? Why is it separate? Some tests like this do not have comments
explaining the purpose behind them. Can you please add comments there.

4. May be bulk of these testcases need to be moved next to Join testcases.
That way their outputs do not change in case the DMLs change in future. In case
you do that, adjust capitalization accordingly.

5. Can we merge following testcase
+select count(*) from ft1 t1 inner join ft2 t2 on (t1.c2 = t2.c2)
where t2.c2 = 6;
with a testcase just before that? I don't see much point in having them
separate.

6. From the comments below, it seems that you want to test the case when the
underlying scan (as a whole) has some local conditions. But the way case has
been written, the underlying join is itself not pushable. Is this intended?
+-- Not pushed down due to local conditions present in underneath input relk
+explain (verbose, costs off)
+select count(*) from ft1 t1 inner join ft1 t2 on (t1.c2 = t2.c2 * random());

7. Instead of separating tests with and without GROUP BY clause into separate
sections, you may want to add them one after the other. Better even if you can
combine them to reduce the number of SQL statements in the file. Do you see any
advantage in having them separate?

8. These two testcases are commented
+--select c1 * random(), sum(c1) * c1 from ft1 group by c1;
+--select "C 1" * random(), sum("C 1") * "C 1" from "S 1"."T 1" group by "C 1";
Either uncomment them and add expected output or remove them altogether. I do
not see any purpose in having them commented.

9. The comment below is too specific.
+-- Aggregate is not pushed down as random() is part of group by expression
It should probably be "Aggregates with unshippable GROUP BY clause are not
shippable" or something general like that.

10. Need some comment about the intention of this and following 3 tests.
+explain (verbose, costs off)
+select count(c2), 5 from ft1 group by 2;

11. About section C, I have a comment similar to comment 7 above. I see that
you have built testcases by gradually adding each clause to a simple SQL
statement. It will become more evident if the improvizations next to the simple
test. Again better if we can merge them into one. Maintaining expected output
of many testcases becomes a pain, if the plans change in future.

12. Following comments
+-- Having clause with random() will be evaluated locally
+-- Having clause with random() will be evaluated locally, and other
having qual is pushed
may be better worded as "Unshippable having clause will be evaluated locally."
to clarify the intention behind the testcase.

13. Wouldn't having random() in the testcase, make them produce different
output in different runs? Also, please see if we can reduce the output rows to
a handful few. Following query produces 21 rows. Is it really important to have
all those rows in the output?
+select c2, sum(c1) from ft2 group by c2 having sum(c1) * random() <
500000 order by c2;

14. For many testcases in section D, one needs to read the testcases carefully
in order to understand the difference between successive testcases. A comment
line clarifying the intention will help.

15. If you have GROUP BY expression in the SELECT clause and apply DISTINCT on
it, DISTINCT is not going to have any effect, will it?
+select distinct sum(c1), c2 from ft2 where c2 < 6 group by c2 order by c2;

16. In the following query, the subquery returns only a single row, so DISTINCT
doesn't have any effects. Please change the subquery to return multiple rows
with some duplicates so that DISTINCT will be propertly tested.
+select distinct (select count(*) filter (where t2.c2 = 6 and t2.c1 <
10) from ft1 t1 where t1.c1 = 6) from ft1 t2 order by 1;
What's the difference between this query and the next one. The comment mentions
INNER and OUTER queries, but I don't see that difference. Am I missing
something?

17. Please mention Why.
+-- Aggregate not pushed down

18. Please club percentile and rank testcases into one to reduce number of SQL
statements.

19. This comment is misleading. VARIADIC is not the reason why the aggregate
was not being pushed down.
+-- Now aggregate with VARIADIC will be pushed

20. Why do we need the last statement ALTERing server extensions? Is it not
already done when the function was added to the extension?
+alter extension postgres_fdw drop function least_accum(anyelement,
variadic anyarray);
+alter extension postgres_fdw drop aggregate least_agg(variadic items anyarray);
+alter server loopback options (set extensions 'postgres_fdw');

21. Following comment is misleading. It will not be pushed down because a user
defined operator is considered unshippable (unless it's part of an extension
known to be shippable.)
+-- This will not be pushed as sort operator is not known to extension.

22. Is following scenario already covered in testcases in section for HAVING?
+-- Clauses with random() will be evaluated locally, and other clauses
are pushed

23. This comment talks about deeper code level internal details. Should have a
better user-visible explanations.
+-- ORDER BY expression is not present as is in target list of remote
query. This needed clearing out sortgrouprefs flag.

24. In the following query, which tests PlaceHolderVars, shouldn't there be an
aggregate on the placeholder column i.e. q.a? Else, I don't see much use of
this case in the context of aggregates. Also, for some reason, the planner
thinks that only avg(ft1.c1) is useful and other two can be nullified. Is that
intentional?
+select count(ft4.c1), sum(q.b) from ft4 left join (select 13,
avg(ft1.c1), sum(ft2.c1) from ft1 right join ft2 on (ft1.c1 = ft2.c1)
where ft1.c1 = 12) q(a, b, c) on (ft4.c1 = q.b) where ft4.c1 between
10 and 15 group by q.b order by 1;

25. Following test looks more appropriate as a join pushdown test, rather than
aggregate pushdown. Nothing in this test gets pushed down.
+select q.a, count(q.a), avg(ft2.c1) from (select 13 from ft1 where c1
= 13) q(a) right join ft2 on (q.a = ft2.c1) where ft2.c1 between 10
and 15 group by q.a order by 1 nulls last;

26. Instead of the testcase below a test which has window aggregates over a
pushed down aggregate makes sense in the context of aggregate pushdown.
+-- WindowAgg
+explain (verbose, costs off)
+select c2, count(c2) over (partition by c2%2) from ft2 where c2 < 10
order by 1 limit 5 offset 95;

postgres_fdw.out has grown by 2000 lines because of testcases in this
patch. We need to compact the testcases so that easier to maintain in
the future.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

This patch will need some changes to conversion_error_callback(). That
function reports an error in case there was an error converting the
result obtained from the foreign server into an internal datum e.g.
when the string returned by the foreign server is not acceptable by
local input function for the expected datatype. In such cases, the
input function will throw error and conversion_error_callback() will
provide appropriate context for that error. postgres_fdw.sql has tests
to test proper context

-- ===================================================================
-- conversion error
-- ===================================================================
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE int;
SELECT * FROM ft1 WHERE c1 = 1;  -- ERROR
SELECT  ft1.c1,  ft2.c2, ft1.c8 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1
AND ft1.c1 = 1; -- ERROR
SELECT  ft1.c1,  ft2.c2, ft1 FROM ft1, ft2 WHERE ft1.c1 = ft2.c1 AND
ft1.c1 = 1; -- ERROR
ALTER FOREIGN TABLE ft1 ALTER COLUMN c8 TYPE user_enum;

Right now we report the column name in the error context. This needs
to change for aggregate pushdown, which is pushing down expressions.
Right now, in case the aggregate on foreign server produces a string
unacceptable locally, it would crash at
4982         Assert(IsA(var, Var));
4983
4984         rte = rt_fetch(var->varno, estate->es_range_table);

since it's not a Var node as expected.

We need to fix the error context to provide meaningful information or
at least not crash. This has been discussed briefly in [1].

[1]
https://www.postgresql.org/message-id/flat/CAFjFpRdcC7Ykb1SkARBYikx9ubKniBiAgHqMD9e47TxzY2EYFw%40mail.gmail.com#CAFjFpRdcC7Ykb1SkARBYikx9ubKniBiAgHqMD9e47TxzY2EYFw@mail.gmail.com

On Fri, Sep 16, 2016 at 7:15 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Hi,
>
> On Mon, Sep 12, 2016 at 5:17 PM, Jeevan Chalke
> <jeevan.chalke@enterprisedb.com> wrote:
>>
>> Thanks Ashutosh for the detailed review comments.
>>
>> I am working on it and will post updated patch once I fix all your
>> concerns.
>>
>>
>
> Attached new patch fixing the review comments.
>
> Here are few comments on the review points:
>
> 1. Renamed deparseFromClause() to deparseFromExpr() and
> deparseAggOrderBy() to appendAggOrderBy()
>
> 2. Done
>
> 3. classifyConditions() assumes list expressions of type RestrictInfo. But
> HAVING clause expressions (and JOIN conditions) are plain expressions. Do
> you mean we should modify the classifyConditions()? If yes, then I think it
> should be done as a separate (cleanup) patch.
>
> 4, 5. Both done.
>
> 6. Per my understanding, I think checking for just aggregate function is
> enough as we are interested in whole aggregate result. Meanwhile I will
> check whether we need to find and check shippability of transition,
> combination and finalization functions or not.
>
> 7, 8, 9, 10, 11, 12. All done.
>
> 13. Fixed. However instead of adding new function made those changes inline.
> Adding support for deparsing List expressions separating list by comma can
> be
> considered as cleanup patch as it will touch other code area not specific to
> aggregate push down.
>
> 14, 15, 16, 17. All done.
>
> 18. I think re-factoring estimate_path_cost_size() should be done separately
> as a cleanup patch too.
>
> 19, 20, 21, 22, 23, 24, 25, 26, 27, 28. All done.
>
> 29. I have tried passing input rel's relids to fetch_upper_rel() call in
> create_grouping_paths(). It solved this patch's issue, but ended up with
> few regression failures which is mostly plan changes. I am not sure whether
> we get good plan now or not as I have not analyzed them.
> So for this patch, I am setting relids in add_foreign_grouping_path()
> itself.
> However as suggested, used bms_copy(). I still kept the FIXME over there as
> I think it should have done by the core itself.
>
> 30, 31, 32, 33. All done.
>
> Let me know your views.
>
> Thanks
> --
> Jeevan B Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>



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

On Fri, Sep 30, 2016 at 8:58 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> On Mon, Sep 26, 2016 at 6:15 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> This patch will need some changes to conversion_error_callback(). That
>> function reports an error in case there was an error converting the
>> result obtained from the foreign server into an internal datum e.g.
>> when the string returned by the foreign server is not acceptable by
>> local input function for the expected datatype. In such cases, the
>> input function will throw error and conversion_error_callback() will
>> provide appropriate context for that error. postgres_fdw.sql has tests
>> to test proper context
>> We need to fix the error context to provide meaningful information or
>> at least not crash. This has been discussed briefly in [1].
>
> Oops. I had that in mind when working on this. Somehow skipped checking
> for conversion error context. I have fixed that in v3 patch.
> Removed assert and for non Var expressions, printing generic context.
> This context is almost in-line with the discussion you referred here.

Moved to next CF.
-- 
Michael

Thanks Jeevan for taking care of the comments.

On Fri, Sep 30, 2016 at 5:23 PM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Hi Ashutosh,
>
> On Mon, Sep 26, 2016 at 2:28 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> Thanks Jeevan for working on the comments.
>>
>> Ok. Yes, we should also handle bare conditions in
>> classifyConditions(). I am fine doing it as a separate patch.
>
>
> Doing that with separate patch would be good.

Ok.

>
>>
>>
>> I don't think add_foreign_grouping_path() is the right place to change
>> a structure managed by the core and esp when we are half-way adding
>> paths. An FDW should not meddle with an upper RelOptInfo. Since
>> create_foreignscan_plan() picks those up from RelOptInfo and both of
>> those are part of core, we need a place in core to set the
>> RelOptInfo::relids for an upper relation OR we have stop using
>> fs_relids for upper relation foreign scans.
>
>
> Yes, agree that relids must be set by the core rather than a fdw.
> However I don't want to touch the core for this patch and also not
> exactly sure where we can do that. I think, for this patch, we can
> copy relids into grouped_rel in create_grouping_paths() at place
> where we assign fdwroutine, userid etc. from the input relation.

I don't think this is acceptable since it changes the search key for
an upper without going through fetch_upper_rel(). Rest of the code,
which tries to find this upper rel with relids = NULL, will not be
able to find it anymore. I have started a discussion on this topic
[1]. One possible solution discussed there is to use all_baserels for
an upper rel. But that solution looks temporary. The discussion has
not yet concluded.

In case we decide not to have any relids in the upper relation, we
will have to obtain those from the scanrel in the context, which is
not being used right now.

Also there are places in code where we check reloptkind and assume it
to be either JOINREL or BASEREL e.g. deparseLockingClause. Those need
to be examined again since now we handle an upper relation. As
discussed before, we might user bms_num_members() on relids to decide
whether the underlying scan is join or base relation.

>
>>
>>
>> 2. SortGroupClause is a parser node, so we can name
>> appendSortGroupClause() as
>> deparseSortGroupClause(), to be consistent with the naming convention. If
>> we do
>> that change, may be it's better to pass SortGroupClause as is and handle
>> other
>> members of that structure as well.
>
>
> Renamed appendSortGroupClause() to deparseSortGroupClause().
> I have kept this function in sync with get_rule_sortgroupclause() which
> takes the Index ref from SortGroupClause(). This function require just
> an index and thus passing SortGroupClause as whole is unnecessary. However
> we cannot pass entire order by list or group by list, because in case of
> order by list we need some extra processing on list elements. So passing
> just Index is sufficient and in sync with get_rule_sortgroupclause() too.

Ok.
In this function+       /* Must force parens for other expressions */
Please explain why we need parenthesis.

>
>>
>> 5. In deparseConst(), showtype is compared with hardcoded values. The
>> callers of this function pass hardcoded values. Instead, we should
>> define an enum and use it. I understand that this logic has been borrowed
>> from
>> get_const_expr(), which also uses hardcoded values. Any reason why not to
>> adopt
>> a better style here? In fact, it only uses two states for showtype, 0 and
>> ">
>> 0". Why don't we use a boolean then OR why isn't the third state in
>> get_const_expr() applicable here?
>
>
> We certainly can add an enum here, but for consistency with other related
> code I think using hard-coded value is good for now. Also I see this
> comment in prologue of deparseConst()
>  * This function has to be kept in sync with ruleutils.c's get_const_expr.
>
> So better to have handling like it.
> Also, currently we use only two values for showtype. But for consistency
> let use int instead of bool. In future if we add support for coercion
> expr, then we need this third value. At that time we will not need changes
> here.
> However if required, we can submit a separate patch for adding enum
> instead of int for showtype in ruleutils.c.
>

Since this patch adds showtype argument which is present in
get_const_expr(), it's clear that we haven't really kept those two
functions in sync, even though the comment says so. It's kind of ugly
to see those hardcoded values. But you have a point. Let's see what
committer says.

>>
>>
>> 7. The changes in block ...
>> should be in sync. The later block adds both aggregates and Vars but the
>> first
>> one doesn't. Why is this difference?
>
>
> add_to_flat_tlist() is taking care of duplicate entries and thus in second
> block, I was calling it after the loop to avoid calling it for every list
> element. Well, moved that inside loop like first block.
>

+   /*
+    * We may need to modify the sortgrouprefs from path target, thus copy it
+    * so that we will not have any undesired effect.  We need to modify the
+    * sortgrouprefs when it points to one of the ORDER BY expression but not
+    * to any GROUP BY expression and that expression is not pushed as is.  If
+    * we do not clear such entries, then we will end up into an error.
+    */
What kind of error? Explain it better.

>>
>>
>> 9. EXPLAIN of foreign scan is adding paranthesis around output
>> expressions,
>> which is not needed. EXPLAIN output of other plan nodes like Aggregate
>> doesn't
>> do that. If it's easy to avoid paranthesis, we should do it in this patch.
>> With
>> this patch, we have started pushing down expressions in the target list,
>> so
>> makes sense to fix it in this patch.
>
>
> ExecInitForeignScan() while determining scan tuple type from passed
> fdw_scan_tlist, has target list containing Vars with varno set to
> INDEX_VAR. Due to which while deparsing we go through
> resolve_special_varno() and get_special_variable() function which
> forces parenthesis around the expression.
> I can't think of any easy and quick solution here. So keeping this
> as is. Input will be welcome or this can be done separately later.
>

I guess, you can decide whether or not to add paranthesis by looking
at the corresponding namespace in the deparse_context. If the
planstate there is ForeignScanState, you may skip the paranthesis if
istoplevel is true. Can you please check if this works?

>>
>>
>> 10. While deparsing the GROUP BY clauses, the code fetches the expressions
>> and
>> deparses the expressions. We might save some CPU cycles if we deparse the
>> clauses by their positional indexes in SELECT clause instead of the actual
>> expressions. This might make the query slightly unreadable. What do you
>> think?
>
>
> To me it is better to have expression in the GROUP BY clause as it is
> more readable and easy to understand. Also it is in sync with deparsing
> logic in ruleutils.c.

Not exactly. get_rule_sortgroupclause() prints only the target list
positions instead of actual expression in GROUP BY or ORDER BY clause
if requested so. We could opt to do the same in all cases. But I think
it's better to add the whole expression in the remote query for
readability.

>
>>
>>
>> Comments about tests:
>>
>> 4. May be bulk of these testcases need to be moved next to Join testcases.
>> That way their outputs do not change in case the DMLs change in future. In
>> case
>> you do that, adjust capitalization accordingly.
>
>
> Moved testcases after Join testcases.
> However I have not made any capitalization changes. I see many tests
> like those elsewhere in the file too. Let me know if that's the policy
> to have appropriate capitalization in test-case. I don't want violate
> the policy if any and will prefer to do the changes as per the policy.

This file has used different styles for different sets. But we usually
adopt the style from surrounding testcases. The testcases surrounding
aggregate testcase block are capitalized. It will look odd to have
just the aggregate tests using different style.

>
>>
>>
>> 13. Wouldn't having random() in the testcase, make them produce different
>> output in different runs? Also, please see if we can reduce the output
>> rows to
>> a handful few. Following query produces 21 rows. Is it really important to
>> have
>> all those rows in the output?
>> +select c2, sum(c1) from ft2 group by c2 having sum(c1) * random() <
>> 500000 order by c2;
>
>
> The condition is written in such a way that we will get all rows,
> nullifying the random() effect, so I don't think there will be any
> issue with the multiple runs.
>

While I agree with that, we can't deny a future change where the test
returns random results. Usually the other tests have used t1.c8 to
induce an unshippable condition. Or use a condition like (random() <=
1) which is always true for any value returned by random(). The
expression will be deemed as volatile. See if you can do something
like that.

>>
>>
>> 20. Why do we need the last statement ALTERing server extensions? Is it
>> not
>> already done when the function was added to the extension?
>> +alter extension postgres_fdw drop function least_accum(anyelement,
>> variadic anyarray);
>> +alter extension postgres_fdw drop aggregate least_agg(variadic items
>> anyarray);
>> +alter server loopback options (set extensions 'postgres_fdw');
>
>
> Whenever we alter extension, we must refresh the server and thus required
> last statement ALTERing server. Without that elements added into extension
> does not reflect in the server.
>

Ok.

>>
>> 23. This comment talks about deeper code level internal details. Should
>> have a
>> better user-visible explanations.
>> +-- ORDER BY expression is not present as is in target list of remote
>> query. This needed clearing out sortgrouprefs flag.
>
>
> Not sure how can I put those comments without referring some code level
> internal details. I have tried altering those comments but it will be
> good if you too try rephrasing it.

I think that testcase can be simplified if we use a non-shippable
expression where an aggregate if shippable e.g. random() * sum(c2) and
order the result by this expression. Because of the unshippable
expression, ORDER BY won't be pushed down and we test the case when
ORDER BY expression is part of the targetlist but can not be pushed
down. If we do that, this need not be a separate testcase and can be
merged with one of the tests earlier.

I guess, it will suffice if the comment just says that it's testing a
case where ORDER BY expression is part of the targetlist but not
pushed down. You don't need to explain what kind of problem that
showed and how it was fixed. If the code changes later, the comment in
the test will be out of sync.

>
>>
>> postgres_fdw.out has grown by 2000 lines because of testcases in this
>> patch. We need to compact the testcases so that easier to maintain in
>> the future.
>
>
> I have removed many duplicate tests and also combined many of them.
> Also removed tests involving local tables. Testcase changes now
> become 1/3 of earlier one.

I guess, there are still avenues for compressing the testcases e.g.
following three testcases and some test cases with HAVING clauses, can
be merged together, since they are testing orthogonal functionality.

+-- Simple aggregates
+explain (verbose, costs off)
+select count(c6), sum(c1), avg(c1), min(c2), max(c1), stddev(c2) from
ft1 group by c2 order by 1, 2;
+select count(c6), sum(c1), avg(c1), min(c2), max(c1), stddev(c2) from
ft1 group by c2 order by 1, 2;
+
+-- Only sum() is pushed. Expression will be executed locally.
+explain (verbose, costs off)
+select sum(c1) * random() from ft1;
+
+-- Simple aggregate with WHERE clause
+explain (verbose, costs off)
+select sum(c1) from ft2 where c2 < 5;
+select sum(c1) from ft2 where c2 < 5;

Here are some more (mostly minor comments)
Why do we need this deparse.c? Even without this include, the code
compiles fine for me.
+#include "utils/ruleutils.h"
+#include "nodes/print.h"

May be you want to explain when foreignrel and scanrel will be different and
their purpose in the comment just above this structure definition.

Please use "foreign server" instead of "remote".
+                /*
+                 * If aggregate's input collation is not derived from a foreign
+                 * Var, it can't be sent to remote.
+                 */

I guess, aggregate's input collation should consider aggfilter collation, since
that doesn't affect aggregates collation. But we should make sure that
aggfilter is safe to pushdown from collation's perspective.
+                /* Check aggregate filter */
+                if (!foreign_expr_walker((Node *) agg->aggfilter,
+                                         glob_cxt, &inner_cxt))
+                    return false;

We need an "a" before upper, I guess.
+     * For upper relation, we have already built the target list while checking


If we add a new kind of RELOPT, this assertion will unintentionally accept it.
May be it's good idea to explicitly list all the RELOPTs even if we support all
of them except one. It was probably I who suggested this change, but thinking
about it again, it doesn't seems to be a good idea. What do you think?
-    /* We handle relations for foreign tables and joins between those */
-    Assert(rel->reloptkind == RELOPT_JOINREL ||
-           rel->reloptkind == RELOPT_BASEREL ||
-           rel->reloptkind == RELOPT_OTHER_MEMBER_REL);
+    /* We handle all relations other than dead one. */
+    Assert(rel->reloptkind != RELOPT_DEADREL);

missing "expect" after ... so we don't
+    /*
+     * Queries with grouping sets are not pushed down, so we don't grouping
+     * sets here.
+     */

Do we need following #include in postgres_fdw.c? I am able to compile the code
fine with those.
+#include "nodes/print.h"


[1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg295435.html
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Wed, Oct 12, 2016 at 9:18 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> I did performance testing for aggregate push down and see good performance
> with the patch.

Are you planning another update to this patch based on Ashutosh's comments?

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

The patch compiles and make check-world doesn't show any failures.

>>
>> >
>> > ExecInitForeignScan() while determining scan tuple type from passed
>> > fdw_scan_tlist, has target list containing Vars with varno set to
>> > INDEX_VAR. Due to which while deparsing we go through
>> > resolve_special_varno() and get_special_variable() function which
>> > forces parenthesis around the expression.
>> > I can't think of any easy and quick solution here. So keeping this
>> > as is. Input will be welcome or this can be done separately later.
>> >
>>
>> I guess, you can decide whether or not to add paranthesis by looking
>> at the corresponding namespace in the deparse_context. If the
>> planstate there is ForeignScanState, you may skip the paranthesis if
>> istoplevel is true. Can you please check if this works?
>
>
> I have tried it. Attached separate patch for it.
> However I have noticed that istoplevel is always false (at-least for the
> testcase we have, I get it false always). And I also think that taking
> this decision only on PlanState is enough. Does that in attached patch.
> To fix this, I have passed deparse_namespace to the private argument and
> accessed it in get_special_variable(). Changes looks very simple. Please
> have a look and let me know your views.
> This issue is not introduced by the changes done for the aggregate push
> down, it only got exposed as we now ship expressions in the target list.
> Thus I think it will be good if we make these changes separately as new
> patch, if required.
>


The patch looks good and pretty simple.

+    * expression.  However if underneath PlanState is ForeignScanState, then
+    * don't force parentheses.
We need to explain why it's safe not to add paranthesis. The reason
this function adds paranthesis so as to preserve any operator
precedences imposed by the expression tree of which this IndexVar is
part. For ForeignScanState, the Var may represent the root of the
expression tree, and thus not need paranthesis. But we need to verify
this and explain it in the comments.

As you have explained this is an issue exposed by this patch;
something not must have in this patch. If committers feel that
aggregate pushdown needs this fix as well, please provide a patch
addressing the above comment.

>> >
>> >
>> > Moved testcases after Join testcases.
>> > However I have not made any capitalization changes. I see many tests
>> > like those elsewhere in the file too. Let me know if that's the policy
>> > to have appropriate capitalization in test-case. I don't want violate
>> > the policy if any and will prefer to do the changes as per the policy.
>>
>> This file has used different styles for different sets. But we usually
>> adopt the style from surrounding testcases. The testcases surrounding
>> aggregate testcase block are capitalized. It will look odd to have
>> just the aggregate tests using different style.
>
>
> I see mixed use of capitalization in the file and use of it is adhoc too.
> So not convinced with your argument yet. I don't think there is any policy
> for that; otherwise use of capitalization in this file would have been
> consistent already. Can we defer this to the committer's opinion.
>

Ok. PFA patch with cosmetic changes (mostly rewording comments). Let
me know if those look good. I am marking this patch is ready for
committer.

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

On Thu, Oct 20, 2016 at 5:38 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Changes look good to me.
> Thanks for the detailed review.

I didn't find anything structurally wrong with this patch, so I've
committed it with many cosmetic changes.  Judging by what happened
with join pushdown, there are probably some residual bugs, but
hopefully not too many.  Anyhow, I don't think waiting longer to
commit this is necessarily going to help, so let's see what happens...

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

Robert Haas <robertmhaas@gmail.com> writes:
> I didn't find anything structurally wrong with this patch, so I've
> committed it with many cosmetic changes.  Judging by what happened
> with join pushdown, there are probably some residual bugs, but
> hopefully not too many.

dromedary seems to have found one, or at least an unstable test result.
        regards, tom lane

On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> I didn't find anything structurally wrong with this patch, so I've
>> committed it with many cosmetic changes.  Judging by what happened
>> with join pushdown, there are probably some residual bugs, but
>> hopefully not too many.
>
> dromedary seems to have found one, or at least an unstable test result.

OK, looking at that now.  Thanks.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> dromedary seems to have found one, or at least an unstable test result.

> OK, looking at that now.  Thanks.

Looking at further failures, it looks like 32-bit machines in general
get that result.  Might be just a cost estimation difference.

Also, some of the windows machines are folding "sqrt(2)" to a different
constant than is hard-wired into the expected-result file.  That's
slightly annoying because it calls into question whether we can ship
floating-point computations to the far end at all :-(.
        regards, tom lane

On Fri, Oct 21, 2016 at 11:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> dromedary seems to have found one, or at least an unstable test result.
>
>> OK, looking at that now.  Thanks.
>
> Looking at further failures, it looks like 32-bit machines in general
> get that result.  Might be just a cost estimation difference.
>
> Also, some of the windows machines are folding "sqrt(2)" to a different
> constant than is hard-wired into the expected-result file.  That's
> slightly annoying because it calls into question whether we can ship
> floating-point computations to the far end at all :-(.

IMHO, not shipping floating-point computations for that reason would
be more annoying than helpful.  To really guarantee that the remote
and identical results are identical, you'd need far more
infrastructure than we have - you'd have to make sure the operating
system collations matched, for example.  And we're already assuming
(without any proof) that the default collations match and, for that
matter, that the datatypes match.  If you're pushing down to
PostgreSQL on the other end, you at least have a hope that the other
side might be sufficiently identical to the local side that the
results will be the same, but if somebody implements these pushdowns
for Oracle or SQL server, you almost might as well give up and go
home.  Even for PostgreSQL, getting the same results in every possible
corner case requires a certain degree of optimism.

For my money, the way to handle that is to add more control over
pushdown rather than automatically disabling it in certain cases.  For
example, we could have GUCs that disable all pushdown or certain types
of pushdown - e.g. you could specify that you don't trust the remote
side to sort data properly, or that you don't like it's floating-point
implementation.  I'm not sure what controls are most useful here, but
I'd be willing to bet you a nice glass of white wine that many people
will be happier with a system that they can control than they will be
with one that just disables the optimization in every case where it
might hypothetically not work out.  It's not clear that we can even
reasonably foresee all such cases.

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

On Fri, Oct 21, 2016 at 8:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> dromedary seems to have found one, or at least an unstable test result.
>
>> OK, looking at that now.  Thanks.
>
> Looking at further failures, it looks like 32-bit machines in general
> get that result.  Might be just a cost estimation difference.

How do I or Jeevan get to look at those results?

>
> Also, some of the windows machines are folding "sqrt(2)" to a different
> constant than is hard-wired into the expected-result file.  That's
> slightly annoying because it calls into question whether we can ship
> floating-point computations to the far end at all :-(.
>

There are two things where the floating point push-down can go wrong:
a. the floating point calculation on the foreign server itself is
different from the local server, b. while converting the binary
floating point representation into text, the actual number gets
truncated because of limit on the number of digits in the text
representation. The later can be avoided by using binary data transfer
mode for floating point number. Is that something worth considering?

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

On Fri, Oct 21, 2016 at 9:09 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Oct 21, 2016 at 11:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Fri, Oct 21, 2016 at 10:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> dromedary seems to have found one, or at least an unstable test result.
>>
>>> OK, looking at that now.  Thanks.
>>
>> Looking at further failures, it looks like 32-bit machines in general
>> get that result.  Might be just a cost estimation difference.
>>
>> Also, some of the windows machines are folding "sqrt(2)" to a different
>> constant than is hard-wired into the expected-result file.  That's
>> slightly annoying because it calls into question whether we can ship
>> floating-point computations to the far end at all :-(.
>
> IMHO, not shipping floating-point computations for that reason would
> be more annoying than helpful.

+1

> To really guarantee that the remote
> and identical results are identical, you'd need far more
> infrastructure than we have - you'd have to make sure the operating
> system collations matched, for example.  And we're already assuming
> (without any proof) that the default collations match and, for that
> matter, that the datatypes match.  If you're pushing down to
> PostgreSQL on the other end, you at least have a hope that the other
> side might be sufficiently identical to the local side that the
> results will be the same, but if somebody implements these pushdowns
> for Oracle or SQL server, you almost might as well give up and go
> home.  Even for PostgreSQL, getting the same results in every possible
> corner case requires a certain degree of optimism.
>
> For my money, the way to handle that is to add more control over
> pushdown rather than automatically disabling it in certain cases.  For
> example, we could have GUCs that disable all pushdown or certain types
> of pushdown - e.g. you could specify that you don't trust the remote
> side to sort data properly, or that you don't like it's floating-point
> implementation.  I'm not sure what controls are most useful here, but
> I'd be willing to bet you a nice glass of white wine that many people
> will be happier with a system that they can control than they will be
> with one that just disables the optimization in every case where it
> might hypothetically not work out.  It's not clear that we can even
> reasonably foresee all such cases.

+1. But having too many of them will make the customer go crazy.
Everytime s/he stumbles upon such a problem (or bug from her/his point
of view), we will add a GUC/option OR tell, "oh! you have to turn
on/off that GUC/option". That will just make the users loose faith in
FDW (or at least postgres_fdw), if we end up with too many of them.

Instead, we may want to add the intelligence to detect when floating
point calculations on the foreign server are different from local one
OR that the default locale or collation on the foreign server are
different from the local one and accordingly set the foreign server
options automatically. This may be part of the postgres_fdw or a
separate utility. I guess, this will be similar to what configure does
to detect compatible libraries. The information gathered will
certainly become obsolete if the user re-configures the server or
changes settings, but at least we can advice them to run the utility
to detect any inconsistencies caused by changes and report or correct
them. I think this is a bigger project, separate from any FDW
pushdown.

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

brolga is still not terribly happy with this patch: it's choosing not to
push down the aggregates in one of the queries.  While I failed to
duplicate that result locally, investigation suggests that brolga's result
is perfectly sane; in fact it's not very clear why we're not getting that
from multiple critters, because the plan brolga is choosing is not
inferior to the expected one.

The core of the problem is this subquery:

contrib_regression=# explain verbose select min(13), avg(ft1.c1), sum(ft2.c1) from ft1 right join ft2 on (ft1.c1 =
ft2.c1)where ft1.c1 = 12;                                                                      QUERY PLAN
                                                         

---------------------------------------------------------------------------------------------------------------------------------------------------------Foreign
Scan (cost=108.61..108.64 rows=1 width=44)  Output: (min(13)), (avg(ft1.c1)), (sum(ft2.c1))  Relations: Aggregate on
((public.ft1)INNER JOIN (public.ft2))  Remote SQL: SELECT min(13), avg(r1."C 1"), sum(r2."C 1") FROM ("S 1"."T 1" r1
INNERJOIN "S 1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C 1" = 12)))) 
(4 rows)

If you look at the estimate to just fetch the data, it's:

contrib_regression=# explain verbose select ft1.c1, ft2.c1 from ft1 right join ft2 on (ft1.c1 = ft2.c1) where ft1.c1 =
12;                                                             QUERY PLAN
                

--------------------------------------------------------------------------------------------------------------------------------------Foreign
Scan (cost=100.55..108.62 rows=1 width=8)  Output: ft1.c1, ft2.c1  Relations: (public.ft1) INNER JOIN (public.ft2)
RemoteSQL: SELECT r1."C 1", r2."C 1" FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r2."C 1" = 12)) AND ((r1."C
1"= 12)))) 
(4 rows)

Note we're expecting only one row out of the join.  Now the cost of doing
three aggregates on a single row of input is not a lot.  Comparing these
local queries:

regression=# explain select min(13),avg(q1),sum(q2) from int8_tbl where q2=456;                         QUERY PLAN
                     
---------------------------------------------------------------Aggregate  (cost=1.07..1.08 rows=1 width=68)  ->  Seq
Scanon int8_tbl  (cost=0.00..1.06 rows=1 width=16)        Filter: (q2 = 456) 
(3 rows)

regression=# explain select (q1),(q2) from int8_tbl where q2=456;                      QUERY PLAN
 
---------------------------------------------------------Seq Scan on int8_tbl  (cost=0.00..1.06 rows=1 width=16)
Filter:(q2 = 456) 
(2 rows)

we seem to have startup = input cost + .01 and then another .01
for total.  So the estimate to do the above remote scan and then
aggregate locally should have been 108.63 startup and 108.64 total,
give or take.  The estimate for aggregating remotely is a hair better,
but it's not nearly better enough to guarantee that the planner won't
see it as fuzzily the same cost.

In short: the problem with this test case is that it's considering
aggregation over only a single row, which is a situation in which
pushing down the aggregate actually doesn't save us anything, because
we're retrieving one row from the remote either way.  So it's not at all
surprising that we don't get a stable plan choice.  The test query needs
to be adjusted so that the aggregation is done over multiple rows,
allowing fdw_tuple_cost to kick in and provide some daylight between
the cost estimates.
        regards, tom lane