diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index bf12eac028..8f4d8a5022 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" @@ -183,6 +184,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); @@ -1038,6 +1041,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 @@ -3445,44 +3475,59 @@ 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); - } + /* Add decoration as needed. */ + appendOrderBySuffix(srt->sortop, exprType(sortexpr), srt->nulls_first, + context); + } +} - if (srt->nulls_first) - appendStringInfoString(buf, " NULLS FIRST"); - else - appendStringInfoString(buf, " NULLS LAST"); +/* + * 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"); } /* @@ -3565,9 +3610,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, @@ -3575,8 +3624,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 */ @@ -3586,7 +3634,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) { @@ -3594,26 +3644,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); + + /* + * 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; - Assert(em_expr != NULL); + /* + * 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 f210f91188..2725b2b211 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -3265,6 +3265,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 @@ -3292,6 +3305,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 56654844e8..c51dd68722 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. */ @@ -6543,7 +6540,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 @@ -6552,13 +6548,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; } @@ -7384,14 +7387,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. + */ +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. */ -Expr * -find_em_expr_for_input_target(PlannerInfo *root, - EquivalenceClass *ec, - PathTarget *target) +EquivalenceMember * +find_em_for_rel_target(PlannerInfo *root, EquivalenceClass *ec, + RelOptInfo *rel) { + PathTarget *target = rel->reltarget; ListCell *lc1; int i; @@ -7434,15 +7478,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 8ae79e97e4..21f2b20ce8 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -173,6 +173,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, @@ -215,10 +218,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 95b6b7192e..6b5de89e14 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -907,6 +907,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; @@ -924,6 +928,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); diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 8c6770de97..ba1931c312 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -931,35 +931,6 @@ is_exprlist_member(Expr *node, List *exprs) return false; } -/* - * Find an equivalence class member expression, all of whose Vars, come from - * the indicated relation. - */ -Expr * -find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel) -{ - ListCell *lc_em; - - foreach(lc_em, ec->ec_members) - { - EquivalenceMember *em = lfirst(lc_em); - - if (bms_is_subset(em->em_relids, rel->relids) && - !bms_is_empty(em->em_relids)) - { - /* - * If there is more than one equivalence member whose Vars are - * taken entirely from this relation, we'll be content to choose - * any one of those. - */ - return em->em_expr; - } - } - - /* We didn't find any suitable equivalence class expression */ - return NULL; -} - /* * relation_can_be_sorted_early * Can this relation be sorted on this EC before the final output step? diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 0c3a0b90c8..e313eb2138 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -143,7 +143,6 @@ extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root, List *exprs, Relids relids, bool require_parallel_safe); -extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel); extern bool relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel, EquivalenceClass *ec, bool require_parallel_safe);