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

Commit8cad5ad

Browse files
committed
Avoid postgres_fdw crash for a targetlist entry that's just a Param.
foreign_grouping_ok() is willing to put fairly arbitrary expressions intothe targetlist of a remote SELECT that's doing grouping or aggregation onthe remote side, including expressions that have no foreign component tothem at all. This is possibly a bit dubious from an efficiency standpoint;but it rises to the level of a crash-causing bug if the expression is justa Param or non-foreign Var. In that case, the expression will necessarilyalso appear in the fdw_exprs list of values we need to send to the remoteserver, and then setrefs.c's set_foreignscan_references will mistakenlyreplace the fdw_exprs entry with a Var referencing the targetlist result.The root cause of this problem is bad design in commite7cb7ee: it putlogic into set_foreignscan_references that IMV is postgres_fdw-specific,and yet this bug shows that it isn't postgres_fdw-specific enough. Thetransformation being done on fdw_exprs assumes that fdw_exprs is to beevaluated with the fdw_scan_tlist as input, which is not how postgres_fdwuses it; yet it could be the right thing for some other FDW. (In thebigger picture, setrefs.c has no business assuming this for the otherexpression fields of a ForeignScan either.)The right fix therefore would be to expand the FDW API so that theFDW could inform setrefs.c how it intends to evaluate these variousexpressions. We can't change that in the back branches though, and wealso can't just summarily change setrefs.c's behavior there, or we'relikely to break external FDWs.As a stopgap, therefore, hack up postgres_fdw so that it won't attemptto send targetlist entries that look exactly like the fdw_exprs entriesthey'd produce. In most cases this actually produces a superior plan,IMO, with less data needing to be transmitted and returned; so we probablyought to think harder about whether we should ship tlist expressions atall when they don't contain any foreign Vars or Aggs. But that's anoptimization not a bug fix so I left it for later. One case where thisproduces an inferior plan is where the expression in question is actuallya GROUP BY expression: then the restriction prevents us from using remotegrouping. It might be possible to work around that (since that wouldreduce to group-by-a-constant on the remote side); but it seems like apretty unlikely corner case, so I'm not sure it's worth expending effortsolely to improve that. In any case the right long-term answer is to fixthe API as sketched above, and then revert this hack.Per bug #15781 from Sean Johnston. Back-patch to v10 where the problemwas introduced.Discussion:https://postgr.es/m/15781-2601b1002bad087c@postgresql.org
1 parent29046c4 commit8cad5ad

File tree

5 files changed

+129
-5
lines changed

5 files changed

+129
-5
lines changed

‎contrib/postgres_fdw/deparse.c

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -841,6 +841,55 @@ foreign_expr_walker(Node *node,
841841
return true;
842842
}
843843

844+
/*
845+
* Returns true if given expr is something we'd have to send the value of
846+
* to the foreign server.
847+
*
848+
* This should return true when the expression is a shippable node that
849+
* deparseExpr would add to context->params_list. Note that we don't care
850+
* if the expression *contains* such a node, only whether one appears at top
851+
* level. We need this to detect cases where setrefs.c would recognize a
852+
* false match between an fdw_exprs item (which came from the params_list)
853+
* and an entry in fdw_scan_tlist (which we're considering putting the given
854+
* expression into).
855+
*/
856+
bool
857+
is_foreign_param(PlannerInfo*root,
858+
RelOptInfo*baserel,
859+
Expr*expr)
860+
{
861+
if (expr==NULL)
862+
return false;
863+
864+
switch (nodeTag(expr))
865+
{
866+
caseT_Var:
867+
{
868+
/* It would have to be sent unless it's a foreign Var */
869+
Var*var= (Var*)expr;
870+
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*) (baserel->fdw_private);
871+
Relidsrelids;
872+
873+
if (IS_UPPER_REL(baserel))
874+
relids=fpinfo->outerrel->relids;
875+
else
876+
relids=baserel->relids;
877+
878+
if (bms_is_member(var->varno,relids)&&var->varlevelsup==0)
879+
return false;/* foreign Var, so not a param */
880+
else
881+
return true;/* it'd have to be a param */
882+
break;
883+
}
884+
caseT_Param:
885+
/* Params always have to be sent to the foreign server */
886+
return true;
887+
default:
888+
break;
889+
}
890+
return false;
891+
}
892+
844893
/*
845894
* Convert type OID + typmod info into a type name we can ship to the remote
846895
* server. Someplace else had better have verified that this type name is

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2707,6 +2707,46 @@ select sum(c1) from ft1 group by c2 having avg(c1 * (random() <= 1)::int) > 100
27072707
Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1"
27082708
(10 rows)
27092709

2710+
-- Remote aggregate in combination with a local Param (for the output
2711+
-- of an initplan) can be trouble, per bug #15781
2712+
explain (verbose, costs off)
2713+
select exists(select 1 from pg_enum), sum(c1) from ft1;
2714+
QUERY PLAN
2715+
--------------------------------------------------
2716+
Foreign Scan
2717+
Output: $0, (sum(ft1.c1))
2718+
Relations: Aggregate on (public.ft1)
2719+
Remote SQL: SELECT sum("C 1") FROM "S 1"."T 1"
2720+
InitPlan 1 (returns $0)
2721+
-> Seq Scan on pg_catalog.pg_enum
2722+
(6 rows)
2723+
2724+
select exists(select 1 from pg_enum), sum(c1) from ft1;
2725+
exists | sum
2726+
--------+--------
2727+
t | 500500
2728+
(1 row)
2729+
2730+
explain (verbose, costs off)
2731+
select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1;
2732+
QUERY PLAN
2733+
---------------------------------------------------
2734+
GroupAggregate
2735+
Output: ($0), sum(ft1.c1)
2736+
Group Key: $0
2737+
InitPlan 1 (returns $0)
2738+
-> Seq Scan on pg_catalog.pg_enum
2739+
-> Foreign Scan on public.ft1
2740+
Output: $0, ft1.c1
2741+
Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
2742+
(8 rows)
2743+
2744+
select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1;
2745+
exists | sum
2746+
--------+--------
2747+
t | 500500
2748+
(1 row)
2749+
27102750
-- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates
27112751
-- ORDER BY within aggregate, same column used to order
27122752
explain (verbose, costs off)

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5486,7 +5486,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
54865486
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*)grouped_rel->fdw_private;
54875487
PathTarget*grouping_target=grouped_rel->reltarget;
54885488
PgFdwRelationInfo*ofpinfo;
5489-
List*aggvars;
54905489
ListCell*lc;
54915490
inti;
54925491
List*tlist=NIL;
@@ -5512,6 +5511,15 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
55125511
* server. All GROUP BY expressions will be part of the grouping target
55135512
* and thus there is no need to search for them separately. Add grouping
55145513
* expressions into target list which will be passed to foreign server.
5514+
*
5515+
* A tricky fine point is that we must not put any expression into the
5516+
* target list that is just a foreign param (that is, something that
5517+
* deparse.c would conclude has to be sent to the foreign server). If we
5518+
* do, the expression will also appear in the fdw_exprs list of the plan
5519+
* node, and setrefs.c will get confused and decide that the fdw_exprs
5520+
* entry is actually a reference to the fdw_scan_tlist entry, resulting in
5521+
* a broken plan. Somewhat oddly, it's OK if the expression contains such
5522+
* a node, as long as it's not at top level; then no match is possible.
55155523
*/
55165524
i=0;
55175525
foreach(lc,grouping_target->exprs)
@@ -5532,6 +5540,13 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
55325540
if (!is_foreign_expr(root,grouped_rel,expr))
55335541
return false;
55345542

5543+
/*
5544+
* If it would be a foreign param, we can't put it into the tlist,
5545+
* so we have to fail.
5546+
*/
5547+
if (is_foreign_param(root,grouped_rel,expr))
5548+
return false;
5549+
55355550
/*
55365551
* Pushable, so add to tlist. We need to create a TLE for this
55375552
* expression and apply the sortgroupref to it. We cannot use
@@ -5547,22 +5562,28 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
55475562
else
55485563
{
55495564
/*
5550-
* Non-grouping expression we need to compute. Is it shippable?
5565+
* Non-grouping expression we need to compute. Can we ship it
5566+
* as-is to the foreign server?
55515567
*/
5552-
if (is_foreign_expr(root,grouped_rel,expr))
5568+
if (is_foreign_expr(root,grouped_rel,expr)&&
5569+
!is_foreign_param(root,grouped_rel,expr))
55535570
{
55545571
/* Yes, so add to tlist as-is; OK to suppress duplicates */
55555572
tlist=add_to_flat_tlist(tlist,list_make1(expr));
55565573
}
55575574
else
55585575
{
55595576
/* Not pushable as a whole; extract its Vars and aggregates */
5577+
List*aggvars;
5578+
55605579
aggvars=pull_var_clause((Node*)expr,
55615580
PVC_INCLUDE_AGGREGATES);
55625581

55635582
/*
55645583
* If any aggregate expression is not shippable, then we
5565-
* cannot push down aggregation to the foreign server.
5584+
* cannot push down aggregation to the foreign server. (We
5585+
* don't have to check is_foreign_param, since that certainly
5586+
* won't return true for any such expression.)
55665587
*/
55675588
if (!is_foreign_expr(root,grouped_rel, (Expr*)aggvars))
55685589
return false;
@@ -5649,7 +5670,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
56495670
* If aggregates within local conditions are not safe to push
56505671
* down, then we cannot push down the query. Vars are already
56515672
* part of GROUP BY clause which are checked above, so no need to
5652-
* access them again here.
5673+
* access them again here. Again, we need not check
5674+
* is_foreign_param for a foreign aggregate.
56535675
*/
56545676
if (IsA(expr,Aggref))
56555677
{

‎contrib/postgres_fdw/postgres_fdw.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,9 @@ extern void classifyConditions(PlannerInfo *root,
146146
externboolis_foreign_expr(PlannerInfo*root,
147147
RelOptInfo*baserel,
148148
Expr*expr);
149+
externboolis_foreign_param(PlannerInfo*root,
150+
RelOptInfo*baserel,
151+
Expr*expr);
149152
externvoiddeparseInsertSql(StringInfobuf,RangeTblEntry*rte,
150153
Indexrtindex,Relationrel,
151154
List*targetAttrs,booldoNothing,

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,16 @@ select count(*) from (select c5, count(c1) from ft1 group by c5, sqrt(c2) having
685685
explain (verbose, costs off)
686686
selectsum(c1)from ft1group by c2havingavg(c1* (random()<=1)::int)>100order by1;
687687

688+
-- Remote aggregate in combination with a local Param (for the output
689+
-- of an initplan) can be trouble, per bug #15781
690+
explain (verbose, costs off)
691+
select exists(select1from pg_enum),sum(c1)from ft1;
692+
select exists(select1from pg_enum),sum(c1)from ft1;
693+
694+
explain (verbose, costs off)
695+
select exists(select1from pg_enum),sum(c1)from ft1group by1;
696+
select exists(select1from pg_enum),sum(c1)from ft1group by1;
697+
688698

689699
-- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates
690700

0 commit comments

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