Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit79df1d2

Browse files
committed
Fix postgres_fdw to check shippability of sort clauses properly.
postgres_fdw would push ORDER BY clauses to the remote side withoutverifying that the sort operator is safe to ship. Moreover, it failedto print a suitable USING clause if the sort operator isn't defaultfor the sort expression's type. The net result of this is that theremote 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 anaggregate function call in commit7012b13, but failed to noticethat query-level ORDER BY was broken. Thus, much of the necessarylogic already existed, but it requires refactoring to be usablein both cases.Back-patch to all supported branches. In HEAD only, remove thecore code's copy of find_em_expr_for_rel, which is no longer usedand really should never have been pushed into equivclass.c in thefirst place.Ronan Dunklau, per report from David Rowley;reviews by David Rowley, Ranier Vilela, and myselfDiscussion:https://postgr.es/m/CAApHDvr4OeC2DBVY--zVP83-K=bYrTD7F8SZDhN4g+pj2f2S-A@mail.gmail.com
1 parentfb1d7f4 commit79df1d2

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"
@@ -179,6 +180,8 @@ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root,
179180
Indexignore_rel,List**ignore_conds,List**params_list);
180181
staticvoiddeparseAggref(Aggref*node,deparse_expr_cxt*context);
181182
staticvoidappendGroupByClause(List*tlist,deparse_expr_cxt*context);
183+
staticvoidappendOrderBySuffix(Oidsortop,Oidsortcoltype,boolnulls_first,
184+
deparse_expr_cxt*context);
182185
staticvoidappendAggOrderBy(List*orderList,List*targetList,
183186
deparse_expr_cxt*context);
184187
staticvoidappendFunctionName(Oidfuncid,deparse_expr_cxt*context);
@@ -904,6 +907,33 @@ is_foreign_param(PlannerInfo *root,
904907
return false;
905908
}
906909

910+
/*
911+
* Returns true if it's safe to push down the sort expression described by
912+
* 'pathkey' to the foreign server.
913+
*/
914+
bool
915+
is_foreign_pathkey(PlannerInfo*root,
916+
RelOptInfo*baserel,
917+
PathKey*pathkey)
918+
{
919+
EquivalenceClass*pathkey_ec=pathkey->pk_eclass;
920+
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*)baserel->fdw_private;
921+
922+
/*
923+
* is_foreign_expr would detect volatile expressions as well, but checking
924+
* ec_has_volatile here saves some cycles.
925+
*/
926+
if (pathkey_ec->ec_has_volatile)
927+
return false;
928+
929+
/* can't push down the sort if the pathkey's opfamily is not shippable */
930+
if (!is_shippable(pathkey->pk_opfamily,OperatorFamilyRelationId,fpinfo))
931+
return false;
932+
933+
/* can push if a suitable EC member exists */
934+
return (find_em_for_rel(root,pathkey_ec,baserel)!=NULL);
935+
}
936+
907937
/*
908938
* Convert type OID + typmod info into a type name we can ship to the remote
909939
* server. Someplace else had better have verified that this type name is
@@ -3070,44 +3100,59 @@ appendAggOrderBy(List *orderList, List *targetList, deparse_expr_cxt *context)
30703100
{
30713101
SortGroupClause*srt= (SortGroupClause*)lfirst(lc);
30723102
Node*sortexpr;
3073-
Oidsortcoltype;
3074-
TypeCacheEntry*typentry;
30753103

30763104
if (!first)
30773105
appendStringInfoString(buf,", ");
30783106
first= false;
30793107

3108+
/* Deparse the sort expression proper. */
30803109
sortexpr=deparseSortGroupClause(srt->tleSortGroupRef,targetList,
30813110
false,context);
3082-
sortcoltype=exprType(sortexpr);
3083-
/* See whether operator is default < or > for datatype */
3084-
typentry=lookup_type_cache(sortcoltype,
3085-
TYPECACHE_LT_OPR |TYPECACHE_GT_OPR);
3086-
if (srt->sortop==typentry->lt_opr)
3087-
appendStringInfoString(buf," ASC");
3088-
elseif (srt->sortop==typentry->gt_opr)
3089-
appendStringInfoString(buf," DESC");
3090-
else
3091-
{
3092-
HeapTupleopertup;
3093-
Form_pg_operatoroperform;
3094-
3095-
appendStringInfoString(buf," USING ");
3096-
3097-
/* Append operator name. */
3098-
opertup=SearchSysCache1(OPEROID,ObjectIdGetDatum(srt->sortop));
3099-
if (!HeapTupleIsValid(opertup))
3100-
elog(ERROR,"cache lookup failed for operator %u",srt->sortop);
3101-
operform= (Form_pg_operator)GETSTRUCT(opertup);
3102-
deparseOperatorName(buf,operform);
3103-
ReleaseSysCache(opertup);
3104-
}
3111+
/* Add decoration as needed. */
3112+
appendOrderBySuffix(srt->sortop,exprType(sortexpr),srt->nulls_first,
3113+
context);
3114+
}
3115+
}
31053116

3106-
if (srt->nulls_first)
3107-
appendStringInfoString(buf," NULLS FIRST");
3108-
else
3109-
appendStringInfoString(buf," NULLS LAST");
3117+
/*
3118+
* Append the ASC, DESC, USING <OPERATOR> and NULLS FIRST / NULLS LAST parts
3119+
* of an ORDER BY clause.
3120+
*/
3121+
staticvoid
3122+
appendOrderBySuffix(Oidsortop,Oidsortcoltype,boolnulls_first,
3123+
deparse_expr_cxt*context)
3124+
{
3125+
StringInfobuf=context->buf;
3126+
TypeCacheEntry*typentry;
3127+
3128+
/* See whether operator is default < or > for sort expr's datatype. */
3129+
typentry=lookup_type_cache(sortcoltype,
3130+
TYPECACHE_LT_OPR |TYPECACHE_GT_OPR);
3131+
3132+
if (sortop==typentry->lt_opr)
3133+
appendStringInfoString(buf," ASC");
3134+
elseif (sortop==typentry->gt_opr)
3135+
appendStringInfoString(buf," DESC");
3136+
else
3137+
{
3138+
HeapTupleopertup;
3139+
Form_pg_operatoroperform;
3140+
3141+
appendStringInfoString(buf," USING ");
3142+
3143+
/* Append operator name. */
3144+
opertup=SearchSysCache1(OPEROID,ObjectIdGetDatum(sortop));
3145+
if (!HeapTupleIsValid(opertup))
3146+
elog(ERROR,"cache lookup failed for operator %u",sortop);
3147+
operform= (Form_pg_operator)GETSTRUCT(opertup);
3148+
deparseOperatorName(buf,operform);
3149+
ReleaseSysCache(opertup);
31103150
}
3151+
3152+
if (nulls_first)
3153+
appendStringInfoString(buf," NULLS FIRST");
3154+
else
3155+
appendStringInfoString(buf," NULLS LAST");
31113156
}
31123157

31133158
/*
@@ -3190,18 +3235,21 @@ appendGroupByClause(List *tlist, deparse_expr_cxt *context)
31903235
}
31913236

31923237
/*
3193-
* Deparse ORDER BY clause according to the given pathkeys for given base
3194-
* relation. From given pathkeys expressions belonging entirely to the given
3195-
* base relation are obtained and deparsed.
3238+
* Deparse ORDER BY clause defined by the given pathkeys.
3239+
*
3240+
* The clause should use Vars from context->scanrel if !has_final_sort,
3241+
* or from context->foreignrel's targetlist if has_final_sort.
3242+
*
3243+
* We find a suitable pathkey expression (some earlier step
3244+
* should have verified that there is one) and deparse it.
31963245
*/
31973246
staticvoid
31983247
appendOrderByClause(List*pathkeys,boolhas_final_sort,
31993248
deparse_expr_cxt*context)
32003249
{
32013250
ListCell*lcell;
32023251
intnestlevel;
3203-
char*delim=" ";
3204-
RelOptInfo*baserel=context->scanrel;
3252+
constchar*delim=" ";
32053253
StringInfobuf=context->buf;
32063254

32073255
/* Make sure any constants in the exprs are printed portably */
@@ -3211,34 +3259,58 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
32113259
foreach(lcell,pathkeys)
32123260
{
32133261
PathKey*pathkey=lfirst(lcell);
3262+
EquivalenceMember*em;
32143263
Expr*em_expr;
3264+
Oidoprid;
32153265

32163266
if (has_final_sort)
32173267
{
32183268
/*
32193269
* By construction, context->foreignrel is the input relation to
32203270
* the final sort.
32213271
*/
3222-
em_expr=find_em_expr_for_input_target(context->root,
3223-
pathkey->pk_eclass,
3224-
context->foreignrel->reltarget);
3272+
em=find_em_for_rel_target(context->root,
3273+
pathkey->pk_eclass,
3274+
context->foreignrel);
32253275
}
32263276
else
3227-
em_expr=find_em_expr_for_rel(pathkey->pk_eclass,baserel);
3277+
em=find_em_for_rel(context->root,
3278+
pathkey->pk_eclass,
3279+
context->scanrel);
3280+
3281+
/*
3282+
* We don't expect any error here; it would mean that shippability
3283+
* wasn't verified earlier. For the same reason, we don't recheck
3284+
* shippability of the sort operator.
3285+
*/
3286+
if (em==NULL)
3287+
elog(ERROR,"could not find pathkey item to sort");
3288+
3289+
em_expr=em->em_expr;
32283290

3229-
Assert(em_expr!=NULL);
3291+
/*
3292+
* Lookup the operator corresponding to the strategy in the opclass.
3293+
* The datatype used by the opfamily is not necessarily the same as
3294+
* the expression type (for array types for example).
3295+
*/
3296+
oprid=get_opfamily_member(pathkey->pk_opfamily,
3297+
em->em_datatype,
3298+
em->em_datatype,
3299+
pathkey->pk_strategy);
3300+
if (!OidIsValid(oprid))
3301+
elog(ERROR,"missing operator %d(%u,%u) in opfamily %u",
3302+
pathkey->pk_strategy,em->em_datatype,em->em_datatype,
3303+
pathkey->pk_opfamily);
32303304

32313305
appendStringInfoString(buf,delim);
32323306
deparseExpr(em_expr,context);
3233-
if (pathkey->pk_strategy==BTLessStrategyNumber)
3234-
appendStringInfoString(buf," ASC");
3235-
else
3236-
appendStringInfoString(buf," DESC");
32373307

3238-
if (pathkey->pk_nulls_first)
3239-
appendStringInfoString(buf," NULLS FIRST");
3240-
else
3241-
appendStringInfoString(buf," NULLS LAST");
3308+
/*
3309+
* Here we need to use the expression's actual type to discover
3310+
* whether the desired operator will be the default or not.
3311+
*/
3312+
appendOrderBySuffix(oprid,exprType((Node*)em_expr),
3313+
pathkey->pk_nulls_first,context);
32423314

32433315
delim=", ";
32443316
}

‎contrib/postgres_fdw/expected/postgres_fdw.out

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

3163+
-- This should not be pushed either.
3164+
explain (verbose, costs off)
3165+
select * from ft2 order by c1 using operator(public.<^);
3166+
QUERY PLAN
3167+
-------------------------------------------------------------------------------
3168+
Sort
3169+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3170+
Sort Key: ft2.c1 USING <^
3171+
-> Foreign Scan on public.ft2
3172+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3173+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
3174+
(6 rows)
3175+
31633176
-- Update local stats on ft2
31643177
ANALYZE ft2;
31653178
-- Add into extension
@@ -3187,6 +3200,16 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
31873200
{6,16,26,36,46,56,66,76,86,96}
31883201
(1 row)
31893202

3203+
-- This should be pushed too.
3204+
explain (verbose, costs off)
3205+
select * from ft2 order by c1 using operator(public.<^);
3206+
QUERY PLAN
3207+
-----------------------------------------------------------------------------------------------------------------------------
3208+
Foreign Scan on public.ft2
3209+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3210+
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
3211+
(3 rows)
3212+
31903213
-- Remove from extension
31913214
alter extension postgres_fdw drop operator class my_op_class using btree;
31923215
alter extension postgres_fdw drop function my_op_cmp(a int, b int);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp