diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 8d59247d30..efaf387890 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -40,6 +40,7 @@ #include "catalog/pg_collation.h" #include "catalog/pg_namespace.h" #include "catalog/pg_operator.h" +#include "catalog/pg_opfamily.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" #include "commands/defrem.h" @@ -180,6 +181,8 @@ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root, Index ignore_rel, List **ignore_conds, List **params_list); static void deparseAggref(Aggref *node, deparse_expr_cxt *context); static void appendGroupByClause(List *tlist, deparse_expr_cxt *context); +static void appendOrderBySuffix(Oid sortop, Oid sortcoltype, bool nulls_first, + deparse_expr_cxt *context); static void appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context); static void appendFunctionName(Oid funcid, deparse_expr_cxt *context); @@ -910,6 +913,33 @@ is_foreign_param(PlannerInfo *root, return false; } +/* + * Returns true if it's safe to push down the sort expression described by + * 'pathkey' to the foreign server. + */ +bool +is_foreign_pathkey(PlannerInfo *root, + RelOptInfo *baserel, + PathKey *pathkey) +{ + EquivalenceClass *pathkey_ec = pathkey->pk_eclass; + PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private; + + /* + * is_foreign_expr would detect volatile expressions as well, but checking + * ec_has_volatile here saves some cycles. + */ + if (pathkey_ec->ec_has_volatile) + return false; + + /* can't push down the sort if the pathkey's opfamily is not shippable */ + if (!is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, fpinfo)) + return false; + + /* can push if a suitable EC member exists */ + return (find_em_for_rel(root, pathkey_ec, baserel) != NULL); +} + /* * Convert type OID + typmod info into a type name we can ship to the remote * server. Someplace else had better have verified that this type name is @@ -3165,46 +3195,61 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context) { SortGroupClause *srt = (SortGroupClause *) lfirst(lc); Node *sortexpr; - Oid sortcoltype; - TypeCacheEntry *typentry; if (!first) appendStringInfoString(buf, ", "); first = false; + /* Deparse the sort expression proper. */ sortexpr = deparseSortGroupClause(srt->tleSortGroupRef, targetList, false, context); - sortcoltype = exprType(sortexpr); - /* See whether operator is default < or > for datatype */ - typentry = lookup_type_cache(sortcoltype, - TYPECACHE_LT_OPR | TYPECACHE_GT_OPR); - if (srt->sortop == typentry->lt_opr) - appendStringInfoString(buf, " ASC"); - else if (srt->sortop == typentry->gt_opr) - appendStringInfoString(buf, " DESC"); - else - { - HeapTuple opertup; - Form_pg_operator operform; - - appendStringInfoString(buf, " USING "); - - /* Append operator name. */ - opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(srt->sortop)); - if (!HeapTupleIsValid(opertup)) - elog(ERROR, "cache lookup failed for operator %u", srt->sortop); - operform = (Form_pg_operator) GETSTRUCT(opertup); - deparseOperatorName(buf, operform); - ReleaseSysCache(opertup); - } - - if (srt->nulls_first) - appendStringInfoString(buf, " NULLS FIRST"); - else - appendStringInfoString(buf, " NULLS LAST"); + /* Add decoration as needed. */ + appendOrderBySuffix(srt->sortop, exprType(sortexpr), srt->nulls_first, + context); } } +/* + * Append the ASC, DESC, USING and NULLS FIRST / NULLS LAST parts + * of an ORDER BY clause. + */ +static void +appendOrderBySuffix(Oid sortop, Oid sortcoltype, bool nulls_first, + deparse_expr_cxt *context) +{ + StringInfo buf = context->buf; + TypeCacheEntry *typentry; + + /* See whether operator is default < or > for sort expr's datatype. */ + typentry = lookup_type_cache(sortcoltype, + TYPECACHE_LT_OPR | TYPECACHE_GT_OPR); + + if (sortop == typentry->lt_opr) + appendStringInfoString(buf, " ASC"); + else if (sortop == typentry->gt_opr) + appendStringInfoString(buf, " DESC"); + else + { + HeapTuple opertup; + Form_pg_operator operform; + + appendStringInfoString(buf, " USING "); + + /* Append operator name. */ + opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(sortop)); + if (!HeapTupleIsValid(opertup)) + elog(ERROR, "cache lookup failed for operator %u", sortop); + operform = (Form_pg_operator) GETSTRUCT(opertup); + deparseOperatorName(buf, operform); + ReleaseSysCache(opertup); + } + + if (nulls_first) + appendStringInfoString(buf, " NULLS FIRST"); + else + appendStringInfoString(buf, " NULLS LAST"); +} + /* * Print the representation of a parameter to be sent to the remote side. * @@ -3285,9 +3330,13 @@ appendGroupByClause(List *tlist, deparse_expr_cxt *context) } /* - * Deparse ORDER BY clause according to the given pathkeys for given base - * relation. From given pathkeys expressions belonging entirely to the given - * base relation are obtained and deparsed. + * Deparse ORDER BY clause defined by the given pathkeys. + * + * The clause should use Vars from context->scanrel if !has_final_sort, + * or from context->foreignrel's targetlist if has_final_sort. + * + * We find a suitable pathkey expression (some earlier step + * should have verified that there is one) and deparse it. */ static void appendOrderByClause(List *pathkeys, bool has_final_sort, @@ -3295,8 +3344,7 @@ appendOrderByClause(List *pathkeys, bool has_final_sort, { ListCell *lcell; int nestlevel; - char *delim = " "; - RelOptInfo *baserel = context->scanrel; + const char *delim = " "; StringInfo buf = context->buf; /* Make sure any constants in the exprs are printed portably */ @@ -3306,7 +3354,9 @@ appendOrderByClause(List *pathkeys, bool has_final_sort, foreach(lcell, pathkeys) { PathKey *pathkey = lfirst(lcell); + EquivalenceMember *em; Expr *em_expr; + Oid oprid; if (has_final_sort) { @@ -3314,26 +3364,48 @@ appendOrderByClause(List *pathkeys, bool has_final_sort, * By construction, context->foreignrel is the input relation to * the final sort. */ - em_expr = find_em_expr_for_input_target(context->root, - pathkey->pk_eclass, - context->foreignrel->reltarget); + em = find_em_for_rel_target(context->root, + pathkey->pk_eclass, + context->foreignrel); } else - em_expr = find_em_expr_for_rel(pathkey->pk_eclass, baserel); + em = find_em_for_rel(context->root, + pathkey->pk_eclass, + context->scanrel); - Assert(em_expr != NULL); + /* + * We don't expect any error here; it would mean that shippability + * wasn't verified earlier. For the same reason, we don't recheck + * shippability of the sort operator. + */ + if (em == NULL) + elog(ERROR, "could not find pathkey item to sort"); + + em_expr = em->em_expr; + + /* + * Lookup the operator corresponding to the strategy in the opclass. + * The datatype used by the opfamily is not necessarily the same as + * the expression type (for array types for example). + */ + oprid = get_opfamily_member(pathkey->pk_opfamily, + em->em_datatype, + em->em_datatype, + pathkey->pk_strategy); + if (!OidIsValid(oprid)) + elog(ERROR, "missing operator %d(%u,%u) in opfamily %u", + pathkey->pk_strategy, em->em_datatype, em->em_datatype, + pathkey->pk_opfamily); appendStringInfoString(buf, delim); deparseExpr(em_expr, context); - if (pathkey->pk_strategy == BTLessStrategyNumber) - appendStringInfoString(buf, " ASC"); - else - appendStringInfoString(buf, " DESC"); - if (pathkey->pk_nulls_first) - appendStringInfoString(buf, " NULLS FIRST"); - else - appendStringInfoString(buf, " NULLS LAST"); + /* + * Here we need to use the expression's actual type to discover + * whether the desired operator will be the default or not. + */ + appendOrderBySuffix(oprid, exprType((Node *) em_expr), + pathkey->pk_nulls_first, context); delim = ", "; } diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index fc8f8bae6f..02313dd28e 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -3169,6 +3169,19 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" < 100)) AND ((c2 = 6)) (6 rows) +-- This should not be pushed either. +explain (verbose, costs off) +select * from ft2 order by c1 using operator(public.<^); + QUERY PLAN +------------------------------------------------------------------------------- + Sort + Output: c1, c2, c3, c4, c5, c6, c7, c8 + Sort Key: ft2.c1 USING <^ + -> Foreign Scan on public.ft2 + Output: c1, c2, c3, c4, c5, c6, c7, c8 + Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" +(6 rows) + -- Update local stats on ft2 ANALYZE ft2; -- Add into extension @@ -3196,6 +3209,16 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 {6,16,26,36,46,56,66,76,86,96} (1 row) +-- This should be pushed too. +explain (verbose, costs off) +select * from ft2 order by c1 using operator(public.<^); + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------- + Foreign Scan on public.ft2 + Output: c1, c2, c3, c4, c5, c6, c7, c8 + Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" ORDER BY "C 1" USING OPERATOR(public.<^) NULLS LAST +(3 rows) + -- Remove from extension alter extension postgres_fdw drop operator class my_op_class using btree; alter extension postgres_fdw drop function my_op_cmp(a int, b int); diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index bf69f9d1da..2193383eec 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -18,6 +18,7 @@ #include "access/sysattr.h" #include "access/table.h" #include "catalog/pg_class.h" +#include "catalog/pg_opfamily.h" #include "commands/defrem.h" #include "commands/explain.h" #include "commands/vacuum.h" @@ -918,8 +919,6 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel) foreach(lc, root->query_pathkeys) { PathKey *pathkey = (PathKey *) lfirst(lc); - EquivalenceClass *pathkey_ec = pathkey->pk_eclass; - Expr *em_expr; /* * The planner and executor don't have any clever strategy for @@ -927,13 +926,8 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel) * getting it to be sorted by all of those pathkeys. We'll just * end up resorting the entire data set. So, unless we can push * down all of the query pathkeys, forget it. - * - * is_foreign_expr would detect volatile expressions as well, but - * checking ec_has_volatile here saves some cycles. */ - if (pathkey_ec->ec_has_volatile || - !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) || - !is_foreign_expr(root, rel, em_expr)) + if (!is_foreign_pathkey(root, rel, pathkey)) { query_pathkeys_ok = false; break; @@ -980,16 +974,19 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel) foreach(lc, useful_eclass_list) { EquivalenceClass *cur_ec = lfirst(lc); - Expr *em_expr; PathKey *pathkey; /* If redundant with what we did above, skip it. */ if (cur_ec == query_ec) continue; + /* Can't push down the sort if the EC's opfamily is not shippable. */ + if (!is_shippable(linitial_oid(cur_ec->ec_opfamilies), + OperatorFamilyRelationId, fpinfo)) + continue; + /* If no pushable expression for this rel, skip it. */ - em_expr = find_em_expr_for_rel(cur_ec, rel); - if (em_expr == NULL || !is_foreign_expr(root, rel, em_expr)) + if (find_em_for_rel(root, cur_ec, rel) == NULL) continue; /* Looks like we can generate a pathkey, so let's do it. */ @@ -6547,7 +6544,6 @@ add_foreign_ordered_paths(PlannerInfo *root, RelOptInfo *input_rel, { PathKey *pathkey = (PathKey *) lfirst(lc); EquivalenceClass *pathkey_ec = pathkey->pk_eclass; - Expr *sort_expr; /* * is_foreign_expr would detect volatile expressions as well, but @@ -6556,13 +6552,20 @@ add_foreign_ordered_paths(PlannerInfo *root, RelOptInfo *input_rel, if (pathkey_ec->ec_has_volatile) return; - /* Get the sort expression for the pathkey_ec */ - sort_expr = find_em_expr_for_input_target(root, - pathkey_ec, - input_rel->reltarget); + /* + * Can't push down the sort if pathkey's opfamily is not shippable. + */ + if (!is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, + fpinfo)) + return; - /* If it's unsafe to remote, we cannot push down the final sort */ - if (!is_foreign_expr(root, input_rel, sort_expr)) + /* + * The EC must contain a shippable EM that is computed in input_rel's + * reltarget, else we can't push down the sort. + */ + if (find_em_for_rel_target(root, + pathkey_ec, + input_rel) == NULL) return; } @@ -7388,14 +7391,55 @@ conversion_error_callback(void *arg) } /* - * Find an equivalence class member expression to be computed as a sort column - * in the given target. + * Given an EquivalenceClass and a foreign relation, find an EC member + * that can be used to sort the relation remotely according to a pathkey + * using this EC. + * + * If there is more than one suitable candidate, return an arbitrary + * one of them. If there is none, return NULL. + * + * This checks that the EC member expression uses only Vars from the given + * rel and is shippable. Caller must separately verify that the pathkey's + * ordering operator is shippable. */ -Expr * -find_em_expr_for_input_target(PlannerInfo *root, - EquivalenceClass *ec, - PathTarget *target) +EquivalenceMember * +find_em_for_rel(PlannerInfo *root, EquivalenceClass *ec, RelOptInfo *rel) { + ListCell *lc; + + foreach(lc, ec->ec_members) + { + EquivalenceMember *em = (EquivalenceMember *) lfirst(lc); + + /* + * Note we require !bms_is_empty, else we'd accept constant + * expressions which are not suitable for the purpose. + */ + if (bms_is_subset(em->em_relids, rel->relids) && + !bms_is_empty(em->em_relids) && + is_foreign_expr(root, rel, em->em_expr)) + return em; + } + + return NULL; +} + +/* + * Find an EquivalenceClass member that is to be computed as a sort column + * in the given rel's reltarget, and is shippable. + * + * If there is more than one suitable candidate, return an arbitrary + * one of them. If there is none, return NULL. + * + * This checks that the EC member expression uses only Vars from the given + * rel and is shippable. Caller must separately verify that the pathkey's + * ordering operator is shippable. + */ +EquivalenceMember * +find_em_for_rel_target(PlannerInfo *root, EquivalenceClass *ec, + RelOptInfo *rel) +{ + PathTarget *target = rel->reltarget; ListCell *lc1; int i; @@ -7438,15 +7482,18 @@ find_em_expr_for_input_target(PlannerInfo *root, while (em_expr && IsA(em_expr, RelabelType)) em_expr = ((RelabelType *) em_expr)->arg; - if (equal(em_expr, expr)) - return em->em_expr; + if (!equal(em_expr, expr)) + continue; + + /* Check that expression (including relabels!) is shippable */ + if (is_foreign_expr(root, rel, em->em_expr)) + return em; } i++; } - elog(ERROR, "could not find pathkey item to sort"); - return NULL; /* keep compiler quiet */ + return NULL; } /* diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index ca83306af9..bd4e592b54 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -171,6 +171,9 @@ extern bool is_foreign_expr(PlannerInfo *root, extern bool is_foreign_param(PlannerInfo *root, RelOptInfo *baserel, Expr *expr); +extern bool is_foreign_pathkey(PlannerInfo *root, + RelOptInfo *baserel, + PathKey *pathkey); extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte, Index rtindex, Relation rel, List *targetAttrs, bool doNothing, @@ -213,10 +216,12 @@ extern void deparseTruncateSql(StringInfo buf, DropBehavior behavior, bool restart_seqs); extern void deparseStringLiteral(StringInfo buf, const char *val); -extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel); -extern Expr *find_em_expr_for_input_target(PlannerInfo *root, - EquivalenceClass *ec, - PathTarget *target); +extern EquivalenceMember *find_em_for_rel(PlannerInfo *root, + EquivalenceClass *ec, + RelOptInfo *rel); +extern EquivalenceMember *find_em_for_rel_target(PlannerInfo *root, + EquivalenceClass *ec, + RelOptInfo *rel); extern List *build_tlist_to_deparse(RelOptInfo *foreignrel); extern void deparseSelectStmtForRel(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel, List *tlist, diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index da004397ed..4680e5cba3 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -873,6 +873,10 @@ create operator class my_op_class for type int using btree family my_op_family a explain (verbose, costs off) select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 and c1 < 100 group by c2; +-- This should not be pushed either. +explain (verbose, costs off) +select * from ft2 order by c1 using operator(public.<^); + -- Update local stats on ft2 ANALYZE ft2; @@ -890,6 +894,10 @@ explain (verbose, costs off) select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 and c1 < 100 group by c2; select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6 and c1 < 100 group by c2; +-- This should be pushed too. +explain (verbose, costs off) +select * from ft2 order by c1 using operator(public.<^); + -- Remove from extension alter extension postgres_fdw drop operator class my_op_class using btree; alter extension postgres_fdw drop function my_op_cmp(a int, b int);