Le mercredi 21 juillet 2021 15:45:15 CEST, vous avez écrit :
> On Thu, 22 Jul 2021 at 00:28, Ronan Dunklau <ronan.dunk...@aiven.io> wrote:
> > The attached patch does the following:
> > - verify the opfamily is shippable to keep pathkeys
> > - generate a correct order by clause using the actual operator.
>
> Thanks for writing the patch.
>
> This is just a very superficial review. I've not spent a great deal
> of time looking at postgres_fdw code, so would rather some eyes that
> were more familiar with the code looked too.
Thank you for the review.
>
> 1. This comment needs to be updated. It still mentions
> is_foreign_expr, which you're no longer calling.
>
> * 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))
>
Done. By the way, the comment just above mentions we don't have a way to use a
prefix pathkey, but I suppose we should revisit that now that we have
IncrementalSort. I'll mark it in my todo list for another patch.
> 2. This is not a very easy return condition to read:
>
> + return (!pathkey_ec->ec_has_volatile &&
> + (em = find_em_for_rel(pathkey_ec, baserel)) &&
> + is_foreign_expr(root, baserel, em->em_expr) &&
> + is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, fpinfo));
>
> I think it would be nicer to break that down into something easier on
> the eyes that could be commented a little more.
Done, let me know what you think about it.
>
> 3. This comment is no longer true:
>
> * 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)
> +EquivalenceMember*
> +find_em_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
>
> Also, missing space after EquivalenceMember.
>
> The comment can just be moved down to:
>
> +Expr *
> +find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
> +{
> + EquivalenceMember *em = find_em_for_rel(ec, rel);
> + return em ? em->em_expr : NULL;
> +}
>
> and you can rewrite the one for find_em_for_rel.
I have done it the other way around. I'm not sure we really need to keep the
find_em_expr_for_rel function on HEAD. If we decide to backpatch, it would need
to be kept though.
--
Ronan Dunklaucommit f7b3dc81878bf2ac503899f010eb18f390a64e37
Author: Ronan Dunklau <ronan.dunk...@aiven.io>
Date: Wed Jul 21 12:44:41 2021 +0200
Fix postgres_fdw PathKey's handling.
The operator family being used for the sort was completely
ignored, and as such its existence on the foreign server was not
checked.
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 31919fda8c..5efefff65e 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -37,9 +37,11 @@
#include "access/sysattr.h"
#include "access/table.h"
#include "catalog/pg_aggregate.h"
+#include "catalog/pg_amop.h"
#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 +182,7 @@ 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 appendOrderbyUsingClause(Oid sortop, Oid sortcoltype, 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,37 @@ is_foreign_param(PlannerInfo *root,
return false;
}
+/* Returns true if the given pathkey can be evaluated on the remote side
+ */
+bool
+is_foreign_pathkey(PlannerInfo *root,
+ RelOptInfo *baserel,
+ PathKey *pathkey)
+{
+ EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
+ EquivalenceMember *em;
+ 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;
+
+ em = find_em_for_rel(pathkey_ec, baserel);
+ if (em == NULL)
+ return false;
+
+ /*
+ * Finally, verify the found member's expression is foreign and its operator
+ * family is shippable.
+ */
+ return (is_foreign_expr(root, baserel, em->em_expr) &&
+ is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, fpinfo));
+}
+
+
/*
* 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
@@ -3125,6 +3159,37 @@ deparseAggref(Aggref *node, deparse_expr_cxt *context)
appendStringInfoChar(buf, ')');
}
+/*
+ * Append the USING <OPERATOR> part of an ORDER BY clause
+ */
+static void
+appendOrderbyUsingClause(Oid sortop, Oid sortcoltype, deparse_expr_cxt *context)
+{
+ StringInfo buf = context->buf;
+ TypeCacheEntry *typentry;
+ 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);
+ }
+}
+
/*
* Append ORDER BY within aggregate function.
*/
@@ -3140,7 +3205,6 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context)
SortGroupClause *srt = (SortGroupClause *) lfirst(lc);
Node *sortexpr;
Oid sortcoltype;
- TypeCacheEntry *typentry;
if (!first)
appendStringInfoString(buf, ", ");
@@ -3150,27 +3214,7 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context)
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);
- }
+ appendOrderbyUsingClause(srt->sortop, sortcoltype, context);
if (srt->nulls_first)
appendStringInfoString(buf, " NULLS FIRST");
@@ -3280,7 +3324,11 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
foreach(lcell, pathkeys)
{
PathKey *pathkey = lfirst(lcell);
+ EquivalenceMember *em;
Expr *em_expr;
+ HeapTuple tuple;
+ Oid oprid;
+ bool isNull;
if (has_final_sort)
{
@@ -3288,21 +3336,44 @@ 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_input_target(context->root,
+ pathkey->pk_eclass,
+ context->foreignrel->reltarget);
}
else
- em_expr = find_em_expr_for_rel(pathkey->pk_eclass, baserel);
+ em = find_em_for_rel(pathkey->pk_eclass, baserel);
+ 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).
+ */
+ tuple = SearchSysCache4(AMOPSTRATEGY,
+ ObjectIdGetDatum(pathkey->pk_opfamily),
+ em->em_datatype,
+ em->em_datatype,
+ pathkey->pk_strategy);
+ if (!HeapTupleIsValid(tuple))
+ {
+ elog(ERROR, "cache lookup failed for operator family %u", pathkey->pk_opfamily);
+ }
+
+ oprid = DatumGetObjectId(SysCacheGetAttr(AMOPSTRATEGY, tuple,
+ Anum_pg_amop_amopopr, &isNull));
+ ReleaseSysCache(tuple);
+
Assert(em_expr != NULL);
appendStringInfoString(buf, delim);
deparseExpr(em_expr, context);
- if (pathkey->pk_strategy == BTLessStrategyNumber)
- appendStringInfoString(buf, " ASC");
- else
- appendStringInfoString(buf, " DESC");
+
+ /*
+ * Here we need to use the expression type to compare against the
+ * default btree sort operator
+ */
+ appendOrderbyUsingClause(oprid, exprType((Node *) em_expr), context);
if (pathkey->pk_nulls_first)
appendStringInfoString(buf, " NULLS FIRST");
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index ed25e7a743..66bada908f 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -3168,6 +3168,18 @@ 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 will not be pushed either
+explain verbose select * from ft2 order by c1 using operator(public.<^);
+ QUERY PLAN
+-------------------------------------------------------------------------------
+ Sort (cost=190.83..193.33 rows=1000 width=142)
+ Output: c1, c2, c3, c4, c5, c6, c7, c8
+ Sort Key: ft2.c1 USING <^
+ -> Foreign Scan on public.ft2 (cost=100.00..141.00 rows=1000 width=47)
+ 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
@@ -3195,6 +3207,15 @@ 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)
+-- And this will be pushed too
+explain verbose select * from ft2 order by c1 using operator(public.<^);
+ QUERY PLAN
+-----------------------------------------------------------------------------------------------------------------------------
+ Foreign Scan on public.ft2 (cost=170.83..193.33 rows=1000 width=47)
+ 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 f15c97ad7a..26c54d6a11 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -916,8 +916,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
@@ -925,13 +923,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;
@@ -6510,9 +6503,9 @@ add_foreign_ordered_paths(PlannerInfo *root, RelOptInfo *input_rel,
return;
/* Get the sort expression for the pathkey_ec */
- sort_expr = find_em_expr_for_input_target(root,
+ sort_expr = find_em_for_input_target(root,
pathkey_ec,
- input_rel->reltarget);
+ input_rel->reltarget)->em_expr;
/* If it's unsafe to remote, we cannot push down the final sort */
if (!is_foreign_expr(root, input_rel, sort_expr))
@@ -7250,11 +7243,11 @@ conversion_error_callback(void *arg)
}
/*
- * Find an equivalence class member expression to be computed as a sort column
+ * Find an equivalence class member to be computed as a sort column
* in the given target.
*/
-Expr *
-find_em_expr_for_input_target(PlannerInfo *root,
+EquivalenceMember*
+find_em_for_input_target(PlannerInfo *root,
EquivalenceClass *ec,
PathTarget *target)
{
@@ -7301,7 +7294,7 @@ find_em_expr_for_input_target(PlannerInfo *root,
em_expr = ((RelabelType *) em_expr)->arg;
if (equal(em_expr, expr))
- return em->em_expr;
+ return em;
}
i++;
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 9591c0f6c2..a0d5c859da 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -165,12 +165,17 @@ extern void classifyConditions(PlannerInfo *root,
List *input_conds,
List **remote_conds,
List **local_conds);
+
extern bool is_foreign_expr(PlannerInfo *root,
RelOptInfo *baserel,
Expr *expr);
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,
@@ -212,8 +217,8 @@ 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,
+extern EquivalenceMember *find_em_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
+extern EquivalenceMember *find_em_for_input_target(PlannerInfo *root,
EquivalenceClass *ec,
PathTarget *target);
extern List *build_tlist_to_deparse(RelOptInfo *foreignrel);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 02a6b15a13..841fac84e4 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -873,6 +873,9 @@ 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 will not be pushed either
+explain verbose select * from ft2 order by c1 using operator(public.<^);
+
-- Update local stats on ft2
ANALYZE ft2;
@@ -890,6 +893,9 @@ 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;
+-- And this will be pushed too
+explain verbose 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 6f1abbe47d..001296597f 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -932,11 +932,11 @@ is_exprlist_member(Expr *node, List *exprs)
}
/*
- * Find an equivalence class member expression, all of whose Vars, come from
+ * Find an equivalence class member, all of whose Vars, come from
* the indicated relation.
*/
-Expr *
-find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+EquivalenceMember *
+find_em_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
{
ListCell *lc_em;
@@ -952,7 +952,7 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
* taken entirely from this relation, we'll be content to choose
* any one of those.
*/
- return em->em_expr;
+ return em;
}
}
@@ -960,6 +960,17 @@ find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
return NULL;
}
+/*
+ * Simple wrapper around find_em_for_rel returning it's expression.
+ */
+Expr *
+find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel)
+{
+ EquivalenceMember *em = find_em_for_rel(ec, rel);
+ return em ? em->em_expr : 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 f1d111063c..bc17114ba3 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -144,6 +144,7 @@ extern EquivalenceMember *find_computable_ec_member(PlannerInfo *root,
Relids relids,
bool require_parallel_safe);
extern Expr *find_em_expr_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
+extern EquivalenceMember *find_em_for_rel(EquivalenceClass *ec, RelOptInfo *rel);
extern bool relation_can_be_sorted_early(PlannerInfo *root, RelOptInfo *rel,
EquivalenceClass *ec,
bool require_parallel_safe);
