Re: Aggregate Push Down - Performing aggregation on foreign server

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