Hi, While testing "Aggregate pushdown", i found the below error: -- GROUP BY alias showing different behavior after adding patch.
-- Create table "t1", insert few records. create table t1(c1 int); insert into t1 values(10), (20); -- Create foreign table: create foreign table f_t1 (c1 int) server db1_server options (table_name 't1'); -- with local table: postgres=# select 2 a, avg(c1) from t1 group by a; a | avg ---+--------------------- 2 | 15.0000000000000000 (1 row) -- with foreign table: postgres=# select 2 a, avg(c1) from f_t1 group by a; ERROR: aggregate functions are not allowed in GROUP BY CONTEXT: Remote SQL command: EXPLAIN SELECT 2, avg(c1) FROM public.t1 GROUP BY 2 On Thu, Sep 8, 2016 at 10:41 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > > > >> While checking for shippability, we build the target list which is passed >> to >> the foreign server as fdw_scan_tlist. The target list contains >> a. All the GROUP BY expressions >> b. Shippable entries from the target list of upper relation >> c. Var and Aggref nodes from non-shippable entries from the target list of >> upper relation >> > > The code in the patch doesn't seem to add Var nodes explicitly. It assumes > that > the Var nodes will be part of GROUP BY clause. The code is correct, I > think. > > >> d. Var and Aggref nodes from non-shippable HAVING conditions. >> > > This needs to be changed as per the comments below. > >> >> Costing: >> >> If use_foreign_estimate is true for input relation, like JOIN case, we use >> EXPLAIN output to get the cost of query with aggregation/grouping on the >> foreign server. If not we calculate the costs locally. Similar to core, >> we use >> get_agg_clause_costs() to get costs for aggregation and then using logic >> similar to cost_agg() we calculate startup and total cost. Since we have >> no >> idea which aggregation strategy will be used at foreign side, we add all >> startup cost (startup cost of input relation, aggregates etc.) into >> startup >> cost for the grouping path and similarly for total cost. >> > > This looks OK to me. > > >> >> Deparsing the query: >> >> Target list created while checking for shippability is deparsed using >> deparseExplicitTargetList(). sortgroupref are adjusted according to this >> target list. Most of the logic to deparse an Aggref is inspired from >> get_agg_expr(). For an upper relation, FROM and WHERE clauses come from >> the >> underlying scan relation and thus for simplicity, FROM clause deparsing >> logic >> is moved from deparseSelectSql() to a new function deparseFromClause(). >> The >> same function adds WHERE clause to the remote SQL. >> > > Ok. > > >> >> >> Area of future work: >> >> 1. Adding path with path-keys to push ORDER BY clause along with >> aggregation/ >> grouping. Should be supported as a separate patch. >> >> 2. Grouping Sets/Rollup/Cube is not supported in current version. I have >> left >> this aside to keep patch smaller. If required I can add that support in >> the >> next version of the patch. >> > > I am fine with these limitations for first cut of this feature. > > I think we should try to measure performance gain because of aggregate > pushdown. The EXPLAIN > doesn't show actual improvement in the execution times. > > Here are the comments on the patch. > > Patch compiles without errors/warnings - Yes > make check passes - Yes. > make check in contrib/postgres_fdw passes - Yes > > Attached patch (based on your patch) has some typos corrected, some > comments > rephrased. It also has some code changes, as explained in various comments > below. Please see if those look good. > > 1. Usually for any deparseSomething function, Something is the type of node > produced by the parser when the string output by that function is parsed. > deparseColumnRef, for example, produces a string, which when parsed > produces a > columnRef node. There is are nodes of type FromClause and AggOrderBy. I > guess, > you meant FromExpr instead of FromClause. deparseAggOrderBy() may be > renamed as > appendOrderByFromList() or something similar. It may be utilized for window > functions if required. > > 2. Can we infer the value of foragg flag from the RelOptInfo passed to > is_foreign_expr()? For any upper relation, it is ok to have aggregate in > there. For any other relation aggregate is not expected. > > 3. In function foreign_grouping_ok(), should we use classifyConditions()? > The > function is written and used for base relations. There's nothing in that > function, which prohibits it being used for other relations. I feel that > foreign_join_ok() should have used the same function to classify the other > clauses. > > 4. May be the individual criteria in the comment block > + /* > + * Aggregate is safe to pushdown if > + * 1. It is a built-in aggregate > + * 2. All its arguments are safe to push-down > + * 3. The functions involved are immutable. > + * 4. Other expressions involved like aggorder, > aggdistinct are > + * safe to be pushed down. > + */ > should be associated with the code blocks which implement those. As the > criteria change it's difficult to maintain the numbered list in sync with > the > code. > > 5. The comment > + /* Aggregates other than simple one are non-pushable. */ > should read /* Only non-split aggregates are pushable. */ as > AGGSPLIT_SIMPLE > means a complete, non-split aggregation step. > > 6. An aggregate function has transition, combination and finalization > function > associated with it. Should we check whether all of the functions are > shippable? > But probably it suffices to check whether aggregate function as whole is > shippable or not using is_shippable() since it's the whole aggregate we are > interested in and not the intermediate results. Probably, we should add a > comment explaining why it's sufficient to check the aggregate function as a > whole. I wondered whether we should check shippability of aggregate return > type, but we don't do that for functions as well. So it's fine. > > 7. It looks like aggdirectargs, aggdistinct, aggorder expressions are all > present in args list. If all the expressions in args are shippable, it > means > those lists are also shippable and hence corresponding aggregate > subclauses are > shippable. Are we unnecessarily checking shippability of those other lists? > > 8. The prologue of build_tlist_to_deparse() mentions that the output > targetlist > contains columns, which is not true with your patch. The targetlist > returned by > this function can have expressions for upper relations. Please update the > prologue to reflect this change. > > 9. In build_tlist_to_deparse(), we are including aggregates from > unshippable > conditions without checking whether they are shippable or not. This can > cause > an unshippable expression to be pushed to the foreign server as seen below > > explain verbose select avg(c1) from ft1 having avg(c1 * random()) > 100; > QUERY PLAN > > ------------------------------------------------------------ > -------------------------- > Foreign Scan (cost=112.50..133.53 rows=1 width=32) > Output: (avg(c1)) > Filter: ((avg(((ft1.c1)::double precision * random()))) > '100'::double > precision) > Relations: Aggregate on (public.ft1) > Remote SQL: SELECT avg(r1."C 1"), avg((r1."C 1" * random())) FROM "S > 1"."T 1" r1 > (5 rows) > > We should check shippability of aggregates in unshippable conditions in > foreign_grouping_ok(). If any of those are not shippable, aggregation can > not > be pushed down. Otherwise, include those in the grouped_tlist. > > 10. Comment /* Construct FROM clause */ should read "Construct FROM and > WHERE > clauses." to be in sync with the next function call. deparseFromClause() > should > be renamed as deparseFromExpr() inline with the naming convention of > deparse<something> functions. From the function signature of > deparseFromClause(), it doesn't become clear as to what contributes to the > FROM > clause. Should we add a comment in the prologue of that function > explaining the > same. Also it strikes odd that for an upper relation, we pass remote_conds > to > this function, but it doesn't deparse those but deparses the remote > conditions > from the scan relation and later deparses the same remote_conds as HAVING > clause. Although this is correct, the code looks odd. May be the codeblock > in > deparseFuncClause() > 1008 /* > 1009 * For aggregates the FROM clause will be build from underneath > scan rel. > 1010 * WHERE clause conditions too taken from there. > 1011 */ > 1012 if (foreignrel->reloptkind == RELOPT_UPPER_REL) > 1013 { > 1014 PgFdwRelationInfo *ofpinfo; > 1015 > 1016 ofpinfo = (PgFdwRelationInfo *) fpinfo->outerrel->fdw_private; > 1017 scan_rel = fpinfo->outerrel; > 1018 context->foreignrel = scan_rel; > 1019 remote_conds = ofpinfo->remote_conds; > 1020 } > should be moved into deparseSelectStmtForRel() to make the things clear. > > 11. Whether to qualify a column name by an alias should be based on > whether the > underlying scan relation is a join or base relation scan. That can be > achieved > by setting scanrel in the deparse_context. Attached patch has this > implemented. > Instead of scanrel, we may also have use_alias value as part of the > context, > which is used everywhere. > > 12. The code to print the function name in deparseAggref() seems similar to > that in deparseFuncExpr(). Should we extract it out into a separate > function > and call that function in deparseAggref() and deparseFuncExpr() both? > > 13. While deparsing the direct aggregates, the code is not adding "," > between > two arguments, resulting in error like below. > select rank(c1, c2) within group (order by c1, c2) from ft1 group by c1, > c2; > ERROR: syntax error at or near "c2" > CONTEXT: Remote SQL command: SELECT rank("C 1"c2) WITHIN GROUP (ORDER BY > "C > 1", c2), "C 1", c2 FROM "S 1"."T 1" GROUP BY "C 1", c2 > May be we should add support to deparse List expressions by separating list > items by "," similar to get_rule_expr() and use that here. > > 14. In deparseAggOrderBy() we do not output anything for default cases > like ASC > or NULLS FIRST (for DESC). But like appendOrderByClause() it's better to be > explicit and output even the default specifications. That way, we do not > leave > anything to the interpretation of the foreign server. > > 15. deparseAggOrderBy supports deparsing USING subclause for ORDER BY for > an > aggregate, but there is no corresponding shippability check for ordering > operator. Also, we should try to produce a testcase for the same. > > 16. The code to deparse a sort/group reference in deparseAggOrderBy() and > appendGroupByClause() looks similar. Should we extract it out into a > function > by itself and call that function in those two places similar to > get_rule_sortgroupclause()? > > 17. In postgresGetForeignPlan() the code to extract conditions and > targetlist > is duplicated for join and upper relation. Attached patch removes this > duplication. Since DML, FOR SHARE/UPDATE is not allowed with aggregation > and > grouping, we should get an outer plan in that code. > > 18. estimate_path_cost_size() has grown quite long (~400 lines). Half of > that > function deals with estimating costs locally. Should we split this function > into smaller functions, one for estimating size and costs of each kind of > relations locally? > > 19. Condition below > + if (sgref && query->groupClause && query->groupingSets == NIL && > + get_sortgroupref_clause_noerr(sgref, query->groupClause) != > NULL) > can be rewritten as > if (sgref && get_sortgroupref_clause_noerr(sgref, query->groupClause)) > since we won't come here with non-NULL query->groupingSets and > get_sortgroupref_clause_noerr() will return NULL, if there aren't any > groupClause. > > 20. Please use word "foreign" instead of "remote" in comments. > > 21. This code looks dubious > + /* If not GROUP BY ref, reset it as we are not pushing those > */ > + if (sgref) > + grouping_target->sortgrouprefs[i] = 0; > grouping_target comes from RelOptInfo, which is being modified here. We > shouldn't be modifying anything in the RelOptInfo while checking whether > the > aggregate/grouping is shippable. You may want to copy the pathtaget and > modify > the copy. > > 22. Following comment needs more elaboration. > + /* > + * Add aggregates, if any, into tlist. Plain Var nodes > pulled > + * are already part of GROUP BY and thus no need to add > them > + * explicitly. > + */ > Plain Var nodes will either be same as some GROUP BY expression or should > be > part of some GROUP BY expression. In the later case, the query can not > refer > those without the surrounding expression. which will be part of the > targetlist > list. Hence we don't need to add plain Var nodes in the targetlist. In fact > adding those in SELECT clause will cause error on the foreign server if > they > are not part of GROUP BY clause. > > 23. Probably you don't need to save this in fpinfo. You may want to > calculate > it whenever it's needed. > + fpinfo->grouped_exprs = get_sortgrouplist_exprs(query->groupClause, > tlist); > > 24. Can this ever happen for postgres_fdw? The core sets fdwroutine and > calls > this function when the underlying scan relation has fdwroutine set, which > implies that it called corresponding GetForeignPath hooks, which should > have > set fdw_private. Nonetheless, I think, still the condition is useful to > avoid > crashing the server in case the fdw_private is not set. But we need better > comments. > + /* If input rel is not aware of fdw, simply return */ > + if (!input_rel->fdw_private) > + return; > > 25. Although it's desirable to get a switch case in > postgresGetForeignUpperPaths() eventually, for this patch I guess we should > have an if condition there. > > 26. Attached patch has restructured code in appendGroupByClause() to reduce > indentation and make the code readable. Let me know if this looks good to > you. > > 27. Prologue of create_foreign_grouping_paths() does not have formatting > similar to the surrounding functions. Please fix it. Since the function > "create"s and "add"s paths, it might be better to rename it as > add_foreign_grouping_paths(). > > 28. Isn't the following true for any upper relation and shouldn't it be > part of > postgresGetForeignUpperPaths(), rather than create_foreign_grouping_paths( > )? > + /* > + * If input rel is not safe to pushdown, we cannot do grouping and/or > + * aggregation on it. > + */ > + if (!ifpinfo || !ifpinfo->pushdown_safe) > + return; > > 29. We require RelOptInfo::relids since create_foreignscan_plan() copies > them > into ForeignPlan::fs_relids and executor uses them. So, I guess, we have > to set > those in the core somewhere. May be while calling fetch_upper_rel() in > grouping_planner(), we should call it with relids of the underlying scan > relation. I don't see any function calling fetch_upper_rel() with non-NULL > relids. In fact, if we are to create an upper relation with relids in > future, > this code is going to wipe that out, which will be undesirable. Also, > generally, when copying relids, it's better to use bms_copy() to make a > copy > of Bitmapset, instead of just assigning the pointer. > > 30. By the time postgresGetForeignUpperPaths() gets called, the core has > already added its own paths, so it doesn't make much sense to set rows and > width grouped_rel in create_foreign_grouping_paths(). > > 31. fpinfo->server and user fields are being set twice, once in > create_foreign_grouping_paths() then in foreign_grouping_ok(). Do we need > that? > > 32. > + /* > + * Do we want to create paths with pathkeys corresponding for > + * root->query_pathkeys. > + */ > Yes, it would be desirable to do that. If we are not going to do that in > this > patch, may be remove that line or add a TODO kind of comment. > > 33. The patch marks build_path_tlist() as non-static but does not use it > anywhere outside creatplan.c. Is this change intentional? > > -- > 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 > > -- *Thanks & Regards,* *Prabhat Kumar Sahu* Mob: 7758988455 Skype ID: prabhat.sahu1984 www.enterprisedb.co <http://www.enterprisedb.com/>m <http://www.enterprisedb.com/>