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

Commitf3dd9fe

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 parent28bdfa2 commitf3dd9fe

File tree

7 files changed

+234
-109
lines changed

7 files changed

+234
-109
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"
@@ -183,6 +184,8 @@ static void deparseRangeTblRef(StringInfo buf, PlannerInfo *root,
183184
Indexignore_rel,List**ignore_conds,List**params_list);
184185
staticvoiddeparseAggref(Aggref*node,deparse_expr_cxt*context);
185186
staticvoidappendGroupByClause(List*tlist,deparse_expr_cxt*context);
187+
staticvoidappendOrderBySuffix(Oidsortop,Oidsortcoltype,boolnulls_first,
188+
deparse_expr_cxt*context);
186189
staticvoidappendAggOrderBy(List*orderList,List*targetList,
187190
deparse_expr_cxt*context);
188191
staticvoidappendFunctionName(Oidfuncid,deparse_expr_cxt*context);
@@ -1038,6 +1041,33 @@ is_foreign_param(PlannerInfo *root,
10381041
return false;
10391042
}
10401043

1044+
/*
1045+
* Returns true if it's safe to push down the sort expression described by
1046+
* 'pathkey' to the foreign server.
1047+
*/
1048+
bool
1049+
is_foreign_pathkey(PlannerInfo*root,
1050+
RelOptInfo*baserel,
1051+
PathKey*pathkey)
1052+
{
1053+
EquivalenceClass*pathkey_ec=pathkey->pk_eclass;
1054+
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*)baserel->fdw_private;
1055+
1056+
/*
1057+
* is_foreign_expr would detect volatile expressions as well, but checking
1058+
* ec_has_volatile here saves some cycles.
1059+
*/
1060+
if (pathkey_ec->ec_has_volatile)
1061+
return false;
1062+
1063+
/* can't push down the sort if the pathkey's opfamily is not shippable */
1064+
if (!is_shippable(pathkey->pk_opfamily,OperatorFamilyRelationId,fpinfo))
1065+
return false;
1066+
1067+
/* can push if a suitable EC member exists */
1068+
return (find_em_for_rel(root,pathkey_ec,baserel)!=NULL);
1069+
}
1070+
10411071
/*
10421072
* Convert type OID + typmod info into a type name we can ship to the remote
10431073
* 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)
34453475
{
34463476
SortGroupClause*srt= (SortGroupClause*)lfirst(lc);
34473477
Node*sortexpr;
3448-
Oidsortcoltype;
3449-
TypeCacheEntry*typentry;
34503478

34513479
if (!first)
34523480
appendStringInfoString(buf,", ");
34533481
first= false;
34543482

3483+
/* Deparse the sort expression proper. */
34553484
sortexpr=deparseSortGroupClause(srt->tleSortGroupRef,targetList,
34563485
false,context);
3457-
sortcoltype=exprType(sortexpr);
3458-
/* See whether operator is default < or > for datatype */
3459-
typentry=lookup_type_cache(sortcoltype,
3460-
TYPECACHE_LT_OPR |TYPECACHE_GT_OPR);
3461-
if (srt->sortop==typentry->lt_opr)
3462-
appendStringInfoString(buf," ASC");
3463-
elseif (srt->sortop==typentry->gt_opr)
3464-
appendStringInfoString(buf," DESC");
3465-
else
3466-
{
3467-
HeapTupleopertup;
3468-
Form_pg_operatoroperform;
3469-
3470-
appendStringInfoString(buf," USING ");
3471-
3472-
/* Append operator name. */
3473-
opertup=SearchSysCache1(OPEROID,ObjectIdGetDatum(srt->sortop));
3474-
if (!HeapTupleIsValid(opertup))
3475-
elog(ERROR,"cache lookup failed for operator %u",srt->sortop);
3476-
operform= (Form_pg_operator)GETSTRUCT(opertup);
3477-
deparseOperatorName(buf,operform);
3478-
ReleaseSysCache(opertup);
3479-
}
3486+
/* Add decoration as needed. */
3487+
appendOrderBySuffix(srt->sortop,exprType(sortexpr),srt->nulls_first,
3488+
context);
3489+
}
3490+
}
34803491

3481-
if (srt->nulls_first)
3482-
appendStringInfoString(buf," NULLS FIRST");
3483-
else
3484-
appendStringInfoString(buf," NULLS LAST");
3492+
/*
3493+
* Append the ASC, DESC, USING <OPERATOR> and NULLS FIRST / NULLS LAST parts
3494+
* of an ORDER BY clause.
3495+
*/
3496+
staticvoid
3497+
appendOrderBySuffix(Oidsortop,Oidsortcoltype,boolnulls_first,
3498+
deparse_expr_cxt*context)
3499+
{
3500+
StringInfobuf=context->buf;
3501+
TypeCacheEntry*typentry;
3502+
3503+
/* See whether operator is default < or > for sort expr's datatype. */
3504+
typentry=lookup_type_cache(sortcoltype,
3505+
TYPECACHE_LT_OPR |TYPECACHE_GT_OPR);
3506+
3507+
if (sortop==typentry->lt_opr)
3508+
appendStringInfoString(buf," ASC");
3509+
elseif (sortop==typentry->gt_opr)
3510+
appendStringInfoString(buf," DESC");
3511+
else
3512+
{
3513+
HeapTupleopertup;
3514+
Form_pg_operatoroperform;
3515+
3516+
appendStringInfoString(buf," USING ");
3517+
3518+
/* Append operator name. */
3519+
opertup=SearchSysCache1(OPEROID,ObjectIdGetDatum(sortop));
3520+
if (!HeapTupleIsValid(opertup))
3521+
elog(ERROR,"cache lookup failed for operator %u",sortop);
3522+
operform= (Form_pg_operator)GETSTRUCT(opertup);
3523+
deparseOperatorName(buf,operform);
3524+
ReleaseSysCache(opertup);
34853525
}
3526+
3527+
if (nulls_first)
3528+
appendStringInfoString(buf," NULLS FIRST");
3529+
else
3530+
appendStringInfoString(buf," NULLS LAST");
34863531
}
34873532

34883533
/*
@@ -3565,18 +3610,21 @@ appendGroupByClause(List *tlist, deparse_expr_cxt *context)
35653610
}
35663611

35673612
/*
3568-
* Deparse ORDER BY clause according to the given pathkeys for given base
3569-
* relation. From given pathkeys expressions belonging entirely to the given
3570-
* base relation are obtained and deparsed.
3613+
* Deparse ORDER BY clause defined by the given pathkeys.
3614+
*
3615+
* The clause should use Vars from context->scanrel if !has_final_sort,
3616+
* or from context->foreignrel's targetlist if has_final_sort.
3617+
*
3618+
* We find a suitable pathkey expression (some earlier step
3619+
* should have verified that there is one) and deparse it.
35713620
*/
35723621
staticvoid
35733622
appendOrderByClause(List*pathkeys,boolhas_final_sort,
35743623
deparse_expr_cxt*context)
35753624
{
35763625
ListCell*lcell;
35773626
intnestlevel;
3578-
char*delim=" ";
3579-
RelOptInfo*baserel=context->scanrel;
3627+
constchar*delim=" ";
35803628
StringInfobuf=context->buf;
35813629

35823630
/* Make sure any constants in the exprs are printed portably */
@@ -3586,34 +3634,58 @@ appendOrderByClause(List *pathkeys, bool has_final_sort,
35863634
foreach(lcell,pathkeys)
35873635
{
35883636
PathKey*pathkey=lfirst(lcell);
3637+
EquivalenceMember*em;
35893638
Expr*em_expr;
3639+
Oidoprid;
35903640

35913641
if (has_final_sort)
35923642
{
35933643
/*
35943644
* By construction, context->foreignrel is the input relation to
35953645
* the final sort.
35963646
*/
3597-
em_expr=find_em_expr_for_input_target(context->root,
3598-
pathkey->pk_eclass,
3599-
context->foreignrel->reltarget);
3647+
em=find_em_for_rel_target(context->root,
3648+
pathkey->pk_eclass,
3649+
context->foreignrel);
36003650
}
36013651
else
3602-
em_expr=find_em_expr_for_rel(pathkey->pk_eclass,baserel);
3652+
em=find_em_for_rel(context->root,
3653+
pathkey->pk_eclass,
3654+
context->scanrel);
3655+
3656+
/*
3657+
* We don't expect any error here; it would mean that shippability
3658+
* wasn't verified earlier. For the same reason, we don't recheck
3659+
* shippability of the sort operator.
3660+
*/
3661+
if (em==NULL)
3662+
elog(ERROR,"could not find pathkey item to sort");
3663+
3664+
em_expr=em->em_expr;
36033665

3604-
Assert(em_expr!=NULL);
3666+
/*
3667+
* Lookup the operator corresponding to the strategy in the opclass.
3668+
* The datatype used by the opfamily is not necessarily the same as
3669+
* the expression type (for array types for example).
3670+
*/
3671+
oprid=get_opfamily_member(pathkey->pk_opfamily,
3672+
em->em_datatype,
3673+
em->em_datatype,
3674+
pathkey->pk_strategy);
3675+
if (!OidIsValid(oprid))
3676+
elog(ERROR,"missing operator %d(%u,%u) in opfamily %u",
3677+
pathkey->pk_strategy,em->em_datatype,em->em_datatype,
3678+
pathkey->pk_opfamily);
36053679

36063680
appendStringInfoString(buf,delim);
36073681
deparseExpr(em_expr,context);
3608-
if (pathkey->pk_strategy==BTLessStrategyNumber)
3609-
appendStringInfoString(buf," ASC");
3610-
else
3611-
appendStringInfoString(buf," DESC");
36123682

3613-
if (pathkey->pk_nulls_first)
3614-
appendStringInfoString(buf," NULLS FIRST");
3615-
else
3616-
appendStringInfoString(buf," NULLS LAST");
3683+
/*
3684+
* Here we need to use the expression's actual type to discover
3685+
* whether the desired operator will be the default or not.
3686+
*/
3687+
appendOrderBySuffix(oprid,exprType((Node*)em_expr),
3688+
pathkey->pk_nulls_first,context);
36173689

36183690
delim=", ";
36193691
}

‎contrib/postgres_fdw/expected/postgres_fdw.out

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

3265+
-- This should not be pushed either.
3266+
explain (verbose, costs off)
3267+
select * from ft2 order by c1 using operator(public.<^);
3268+
QUERY PLAN
3269+
-------------------------------------------------------------------------------
3270+
Sort
3271+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3272+
Sort Key: ft2.c1 USING <^
3273+
-> Foreign Scan on public.ft2
3274+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3275+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
3276+
(6 rows)
3277+
32653278
-- Update local stats on ft2
32663279
ANALYZE ft2;
32673280
-- Add into extension
@@ -3289,6 +3302,16 @@ select array_agg(c1 order by c1 using operator(public.<^)) from ft2 where c2 = 6
32893302
{6,16,26,36,46,56,66,76,86,96}
32903303
(1 row)
32913304

3305+
-- This should be pushed too.
3306+
explain (verbose, costs off)
3307+
select * from ft2 order by c1 using operator(public.<^);
3308+
QUERY PLAN
3309+
-----------------------------------------------------------------------------------------------------------------------------
3310+
Foreign Scan on public.ft2
3311+
Output: c1, c2, c3, c4, c5, c6, c7, c8
3312+
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
3313+
(3 rows)
3314+
32923315
-- Remove from extension
32933316
alter extension postgres_fdw drop operator class my_op_class using btree;
32943317
alter extension postgres_fdw drop function my_op_cmp(a int, b int);

0 commit comments

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