Skip to content

Commit 9f9489a

Browse files
tglsfdctglsfdc
committed
Fix postgres_fdw to check shippability of sort clauses properly.
postgres_fdw would push ORDER BY clauses to the remote side without verifying that the sort operator is safe to ship. Moreover, it failed to print a suitable USING clause if the sort operator isn't default for the sort expression's type. The net result of this is that the remote sort might not have anywhere near the semantics we expect, which'd be disastrous for locally-performed merge joins in particular. We addressed similar issues in the context of ORDER BY within an aggregate function call in commit 7012b13, but failed to notice that query-level ORDER BY was broken. Thus, much of the necessary logic already existed, but it requires refactoring to be usable in both cases. Back-patch to all supported branches. In HEAD only, remove the core code's copy of find_em_expr_for_rel, which is no longer used and really should never have been pushed into equivclass.c in the first place. Ronan Dunklau, per report from David Rowley; reviews by David Rowley, Ranier Vilela, and myself Discussion: https://postgr.es/m/CAApHDvr4OeC2DBVY--zVP83-K=bYrTD7F8SZDhN4g+pj2f2S-A@mail.gmail.com
1 parent 402279a commit 9f9489a

File tree

5 files changed

+234
-79
lines changed

5 files changed

+234
-79
lines changed

‎contrib/postgres_fdw/deparse.c

Lines changed: 119 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
#include "catalog/pg_collation.h"
4141
#include "catalog/pg_namespace.h"
4242
#include "catalog/pg_operator.h"
43+
#include "catalog/pg_opfamily.h"
4344
#include "catalog/pg_proc.h"
4445
#include "catalog/pg_type.h"
4546
#include "commands/defrem.h"
@@ -180,6 +181,8 @@ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root,
180181
Index ignore_rel, List **ignore_conds, List **params_list);
181182
static void deparseAggref(Aggref *node, deparse_expr_cxt *context);
182183
static void appendGroupByClause(List *tlist, deparse_expr_cxt *context);
184+
static void appendOrderBySuffix(Oid sortop, Oid sortcoltype, bool nulls_first,
185+
deparse_expr_cxt *context);
183186
static void appendAggOrderBy(List *orderList, List *targetList,
184187
deparse_expr_cxt *context);
185188
static void appendFunctionName(Oid funcid, deparse_expr_cxt *context);
@@ -910,6 +913,33 @@ is_foreign_param(PlannerInfo *root,
910913
return false;
911914
}
912915

916+
/*
917+
* Returns true if it's safe to push down the sort expression described by
918+
* 'pathkey' to the foreign server.
919+
*/
920+
bool
921+
is_foreign_pathkey(PlannerInfo *root,
922+
RelOptInfo *baserel,
923+
PathKey *pathkey)
924+
{
925+
EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
926+
PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) baserel->fdw_private;
927+
928+
/*
929+
* is_foreign_expr would detect volatile expressions as well, but checking
930+
* ec_has_volatile here saves some cycles.
931+
*/
932+
if (pathkey_ec->ec_has_volatile)
933+
return false;
934+
935+
/* can't push down the sort if the pathkey's opfamily is not shippable */
936+
if (!is_shippable(pathkey->pk_opfamily, OperatorFamilyRelationId, fpinfo))
937+
return false;
938+
939+
/* can push if a suitable EC member exists */
940+
return (find_em_for_rel(root, pathkey_ec, baserel) != NULL);
941+
}
942+
913943
/*
914944
* Convert type OID + typmod info into a type name we can ship to the remote
915945
* server. Someplace else had better have verified that this type name is
@@ -3165,44 +3195,59 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context)
31653195
{
31663196
SortGroupClause *srt = (SortGroupClause *) lfirst(lc);
31673197
Node *sortexpr;
3168-
Oid sortcoltype;
3169-
TypeCacheEntry *typentry;
31703198

31713199
if (!first)
31723200
appendStringInfoString(buf, ", ");
31733201
first = false;
31743202

3203+
/* Deparse the sort expression proper. */
31753204
sortexpr = deparseSortGroupClause(srt->tleSortGroupRef, targetList,
31763205
false, context);
3177-
sortcoltype = exprType(sortexpr);
3178-
/* See whether operator is default < or > for datatype */
3179-
typentry = lookup_type_cache(sortcoltype,
3180-
TYPECACHE_LT_OPR | TYPECACHE_GT_OPR);
3181-
if (srt->sortop == typentry->lt_opr)
3182-
appendStringInfoString(buf, " ASC");
3183-
else if (srt->sortop == typentry->gt_opr)
3184-
appendStringInfoString(buf, " DESC");
3185-
else
3186-
{
3187-
HeapTuple opertup;
3188-
Form_pg_operator operform;
3189-
3190-
appendStringInfoString(buf, " USING ");
3191-
3192-
/* Append operator name. */
3193-
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(srt->sortop));
3194-
if (!HeapTupleIsValid(opertup))
3195-
elog(ERROR, "cache lookup failed for operator %u", srt->sortop);
3196-
operform = (Form_pg_operator) GETSTRUCT(opertup);
3197-
deparseOperatorName(buf, operform);
3198-
ReleaseSysCache(opertup);
3199-
}
3206+
/* Add decoration as needed. */
3207+
appendOrderBySuffix(srt->sortop, exprType(sortexpr), srt->nulls_first,
3208+
context);
3209+
}
3210+
}
32003211

3201-
if (srt->nulls_first)
3202-
appendStringInfoString(buf, " NULLS FIRST");
3203-
else
3204-
appendStringInfoString(buf, " NULLS LAST");
3212+
/*
3213+
* Append the ASC, DESC, USING <OPERATOR> and NULLS FIRST / NULLS LAST parts
3214+
* of an ORDER BY clause.
3215+
*/
3216+
static void
3217+
appendOrderBySuffix(Oid sortop, Oid sortcoltype, bool nulls_first,
3218+
deparse_expr_cxt *context)
3219+
{
3220+
StringInfo buf = context->buf;
3221+
TypeCacheEntry *typentry;
3222+
3223+
/* See whether operator is default < or > for sort expr's datatype. */
3224+
typentry = lookup_type_cache(sortcoltype,
3225+
TYPECACHE_LT_OPR | TYPECACHE_GT_OPR);
3226+
3227+
if (sortop == typentry->lt_opr)
3228+
appendStringInfoString(buf, " ASC");
3229+
else if (sortop == typentry->gt_opr)
3230+
appendStringInfoString(buf, " DESC");
3231+
else
3232+
{
3233+
HeapTuple opertup;
3234+
Form_pg_operator operform;
3235+
3236+
appendStringInfoString(buf, " USING ");
3237+
3238+
/* Append operator name. */
3239+
opertup = SearchSysCache1(OPEROID, ObjectIdGetDatum(sortop));
3240+
if (!HeapTupleIsValid(opertup))
3241+
elog(ERROR, "cache lookup failed for operator %u", sortop);
3242+
operform = (Form_pg_operator) GETSTRUCT(opertup);
3243+
deparseOperatorName(buf, operform);
3244+
ReleaseSysCache(opertup);
32053245
}
3246+
3247+
if (nulls_first)
3248+
appendStringInfoString(buf, " NULLS FIRST");
3249+
else
3250+
appendStringInfoString(buf, " NULLS LAST");
32063251
}
32073252

32083253
/*
@@ -3285,18 +3330,21 @@ appendGroupByClause(List *tlist, deparse_expr_cxt *context)
32853330
}
32863331

32873332
/*
3288-
* Deparse ORDER BY clause according to the given pathkeys for given base
3289-
* relation. From given pathkeys expressions belonging entirely to the given
3290-
* base relation are obtained and deparsed.
3333+
* Deparse ORDER BY clause defined by the given pathkeys.
3334+
*
3335+
* The clause should use Vars from context->scanrel if !has_final_sort,
3336+
* or from context->foreignrel's targetlist if has_final_sort.
3337+
*
3338+
* We find a suitable pathkey expression (some earlier step
3339+
* should have verified that there is one) and deparse it.
32913340
*/
32923341
static void
32933342
appendOrderByClause(List *pathkeys, bool has_final_sort,
32943343
deparse_expr_cxt *context)
32953344
{
32963345
ListCell *lcell;
32973346
int nestlevel;
3298-
char *delim = " ";
3299-
RelOptInfo *baserel = context->scanrel;
3347+
const char *delim = " ";
33003348
StringInfo buf = context->buf;
33013349

33023350
/* Make sure any constants in the exprs are printed portably */
@@ -3306,34 +3354,58 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
33063354
foreach(lcell, pathkeys)
33073355
{
33083356
PathKey *pathkey = lfirst(lcell);
3357+
EquivalenceMember *em;
33093358
Expr *em_expr;
3359+
Oid oprid;
33103360

33113361
if (has_final_sort)
33123362
{
33133363
/*
33143364
* By construction, context->foreignrel is the input relation to
33153365
* the final sort.
33163366
*/
3317-
em_expr = find_em_expr_for_input_target(context->root,
3318-
pathkey->pk_eclass,
3319-
context->foreignrel->reltarget);
3367+
em = find_em_for_rel_target(context->root,
3368+
pathkey->pk_eclass,
3369+
context->foreignrel);
33203370
}
33213371
else
3322-
em_expr = find_em_expr_for_rel(pathkey->pk_eclass, baserel);
3372+
em = find_em_for_rel(context->root,
3373+
pathkey->pk_eclass,
3374+
context->scanrel);
3375+
3376+
/*
3377+
* We don't expect any error here; it would mean that shippability
3378+
* wasn't verified earlier. For the same reason, we don't recheck
3379+
* shippability of the sort operator.
3380+
*/
3381+
if (em == NULL)
3382+
elog(ERROR, "could not find pathkey item to sort");
3383+
3384+
em_expr = em->em_expr;
33233385

3324-
Assert(em_expr != NULL);
3386+
/*
3387+
* Lookup the operator corresponding to the strategy in the opclass.
3388+
* The datatype used by the opfamily is not necessarily the same as
3389+
* the expression type (for array types for example).
3390+
*/
3391+
oprid = get_opfamily_member(pathkey->pk_opfamily,
3392+
em->em_datatype,
3393+
em->em_datatype,
3394+
pathkey->pk_strategy);
3395+
if (!OidIsValid(oprid))
3396+
elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
3397+
pathkey->pk_strategy, em->em_datatype, em->em_datatype,
3398+
pathkey->pk_opfamily);
33253399

33263400
appendStringInfoString(buf, delim);
33273401
deparseExpr(em_expr, context);
3328-
if (pathkey->pk_strategy == BTLessStrategyNumber)
3329-
appendStringInfoString(buf, " ASC");
3330-
else
3331-
appendStringInfoString(buf, " DESC");
33323402

3333-
if (pathkey->pk_nulls_first)
3334-
appendStringInfoString(buf, " NULLS FIRST");
3335-
else
3336-
appendStringInfoString(buf, " NULLS LAST");
3403+
/*
3404+
* Here we need to use the expression's actual type to discover
3405+
* whether the desired operator will be the default or not.
3406+
*/
3407+
appendOrderBySuffix(oprid, exprType((Node *) em_expr),
3408+
pathkey->pk_nulls_first, context);
33373409

33383410
delim = ", ";
33393411
}

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3169,6 +3169,19 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
31693169
Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1" WHERE (("C 1" < 100)) AND ((c2 = 6))
31703170
(6 rows)
31713171

3172+
-- This should not be pushed either.
3173+
explain (verbose, costs off)
3174+
select * from ft2 order by c1 using operator(public.<^);
3175+
QUERY PLAN
3176+
-------------------------------------------------------------------------------
3177+
Sort
3178+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3179+
Sort Key: ft2.c1 USING <^
3180+
-> Foreign Scan on public.ft2
3181+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3182+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
3183+
(6 rows)
3184+
31723185
-- Update local stats on ft2
31733186
ANALYZE ft2;
31743187
-- Add into extension
@@ -3196,6 +3209,16 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
31963209
{6,16,26,36,46,56,66,76,86,96}
31973210
(1 row)
31983211

3212+
-- This should be pushed too.
3213+
explain (verbose, costs off)
3214+
select * from ft2 order by c1 using operator(public.<^);
3215+
QUERY PLAN
3216+
-----------------------------------------------------------------------------------------------------------------------------
3217+
Foreign Scan on public.ft2
3218+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3219+
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
3220+
(3 rows)
3221+
31993222
-- Remove from extension
32003223
alter extension postgres_fdw drop operator class my_op_class using btree;
32013224
alter extension postgres_fdw drop function my_op_cmp(a int, b int);

0 commit comments

Comments
 (0)