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

Commitc25e638

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 parentfc732e0 commitc25e638

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
@@ -836,6 +836,55 @@ foreign_expr_walker(Node *node,
836836
return true;
837837
}
838838

839+
/*
840+
* Returns true if given expr is something we'd have to send the value of
841+
* to the foreign server.
842+
*
843+
* This should return true when the expression is a shippable node that
844+
* deparseExpr would add to context->params_list. Note that we don't care
845+
* if the expression *contains* such a node, only whether one appears at top
846+
* level. We need this to detect cases where setrefs.c would recognize a
847+
* false match between an fdw_exprs item (which came from the params_list)
848+
* and an entry in fdw_scan_tlist (which we're considering putting the given
849+
* expression into).
850+
*/
851+
bool
852+
is_foreign_param(PlannerInfo*root,
853+
RelOptInfo*baserel,
854+
Expr*expr)
855+
{
856+
if (expr==NULL)
857+
return false;
858+
859+
switch (nodeTag(expr))
860+
{
861+
caseT_Var:
862+
{
863+
/* It would have to be sent unless it's a foreign Var */
864+
Var*var= (Var*)expr;
865+
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*) (baserel->fdw_private);
866+
Relidsrelids;
867+
868+
if (IS_UPPER_REL(baserel))
869+
relids=fpinfo->outerrel->relids;
870+
else
871+
relids=baserel->relids;
872+
873+
if (bms_is_member(var->varno,relids)&&var->varlevelsup==0)
874+
return false;/* foreign Var, so not a param */
875+
else
876+
return true;/* it'd have to be a param */
877+
break;
878+
}
879+
caseT_Param:
880+
/* Params always have to be sent to the foreign server */
881+
return true;
882+
default:
883+
break;
884+
}
885+
return false;
886+
}
887+
839888
/*
840889
* Convert type OID + typmod info into a type name we can ship to the remote
841890
* 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
@@ -2814,6 +2814,46 @@ select sum(c1) from ft1 group by c2 having avg(c1 * (random() <= 1)::int) > 100
28142814
Remote SQL: SELECT "C 1", c2 FROM "S 1"."T 1"
28152815
(10 rows)
28162816

2817+
-- Remote aggregate in combination with a local Param (for the output
2818+
-- of an initplan) can be trouble, per bug #15781
2819+
explain (verbose, costs off)
2820+
select exists(select 1 from pg_enum), sum(c1) from ft1;
2821+
QUERY PLAN
2822+
--------------------------------------------------
2823+
Foreign Scan
2824+
Output: $0, (sum(ft1.c1))
2825+
Relations: Aggregate on (public.ft1)
2826+
Remote SQL: SELECT sum("C 1") FROM "S 1"."T 1"
2827+
InitPlan 1 (returns $0)
2828+
-> Seq Scan on pg_catalog.pg_enum
2829+
(6 rows)
2830+
2831+
select exists(select 1 from pg_enum), sum(c1) from ft1;
2832+
exists | sum
2833+
--------+--------
2834+
t | 500500
2835+
(1 row)
2836+
2837+
explain (verbose, costs off)
2838+
select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1;
2839+
QUERY PLAN
2840+
---------------------------------------------------
2841+
GroupAggregate
2842+
Output: ($0), sum(ft1.c1)
2843+
Group Key: $0
2844+
InitPlan 1 (returns $0)
2845+
-> Seq Scan on pg_catalog.pg_enum
2846+
-> Foreign Scan on public.ft1
2847+
Output: $0, ft1.c1
2848+
Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
2849+
(8 rows)
2850+
2851+
select exists(select 1 from pg_enum), sum(c1) from ft1 group by 1;
2852+
exists | sum
2853+
--------+--------
2854+
t | 500500
2855+
(1 row)
2856+
28172857
-- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates
28182858
-- ORDER BY within aggregate, same column used to order
28192859
explain (verbose, costs off)

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4637,7 +4637,6 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
46374637
PathTarget*grouping_target=root->upper_targets[UPPERREL_GROUP_AGG];
46384638
PgFdwRelationInfo*fpinfo= (PgFdwRelationInfo*)grouped_rel->fdw_private;
46394639
PgFdwRelationInfo*ofpinfo;
4640-
List*aggvars;
46414640
ListCell*lc;
46424641
inti;
46434642
List*tlist=NIL;
@@ -4663,6 +4662,15 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
46634662
* server. All GROUP BY expressions will be part of the grouping target
46644663
* and thus there is no need to search for them separately. Add grouping
46654664
* expressions into target list which will be passed to foreign server.
4665+
*
4666+
* A tricky fine point is that we must not put any expression into the
4667+
* target list that is just a foreign param (that is, something that
4668+
* deparse.c would conclude has to be sent to the foreign server). If we
4669+
* do, the expression will also appear in the fdw_exprs list of the plan
4670+
* node, and setrefs.c will get confused and decide that the fdw_exprs
4671+
* entry is actually a reference to the fdw_scan_tlist entry, resulting in
4672+
* a broken plan. Somewhat oddly, it's OK if the expression contains such
4673+
* a node, as long as it's not at top level; then no match is possible.
46664674
*/
46674675
i=0;
46684676
foreach(lc,grouping_target->exprs)
@@ -4683,6 +4691,13 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
46834691
if (!is_foreign_expr(root,grouped_rel,expr))
46844692
return false;
46854693

4694+
/*
4695+
* If it would be a foreign param, we can't put it into the tlist,
4696+
* so we have to fail.
4697+
*/
4698+
if (is_foreign_param(root,grouped_rel,expr))
4699+
return false;
4700+
46864701
/*
46874702
* Pushable, so add to tlist. We need to create a TLE for this
46884703
* expression and apply the sortgroupref to it. We cannot use
@@ -4698,22 +4713,28 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
46984713
else
46994714
{
47004715
/*
4701-
* Non-grouping expression we need to compute. Is it shippable?
4716+
* Non-grouping expression we need to compute. Can we ship it
4717+
* as-is to the foreign server?
47024718
*/
4703-
if (is_foreign_expr(root,grouped_rel,expr))
4719+
if (is_foreign_expr(root,grouped_rel,expr)&&
4720+
!is_foreign_param(root,grouped_rel,expr))
47044721
{
47054722
/* Yes, so add to tlist as-is; OK to suppress duplicates */
47064723
tlist=add_to_flat_tlist(tlist,list_make1(expr));
47074724
}
47084725
else
47094726
{
47104727
/* Not pushable as a whole; extract its Vars and aggregates */
4728+
List*aggvars;
4729+
47114730
aggvars=pull_var_clause((Node*)expr,
47124731
PVC_INCLUDE_AGGREGATES);
47134732

47144733
/*
47154734
* If any aggregate expression is not shippable, then we
4716-
* cannot push down aggregation to the foreign server.
4735+
* cannot push down aggregation to the foreign server. (We
4736+
* don't have to check is_foreign_param, since that certainly
4737+
* won't return true for any such expression.)
47174738
*/
47184739
if (!is_foreign_expr(root,grouped_rel, (Expr*)aggvars))
47194740
return false;
@@ -4800,7 +4821,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel)
48004821
* If aggregates within local conditions are not safe to push
48014822
* down, then we cannot push down the query. Vars are already
48024823
* part of GROUP BY clause which are checked above, so no need to
4803-
* access them again here.
4824+
* access them again here. Again, we need not check
4825+
* is_foreign_param for a foreign aggregate.
48044826
*/
48054827
if (IsA(expr,Aggref))
48064828
{

‎contrib/postgres_fdw/postgres_fdw.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ extern void classifyConditions(PlannerInfo *root,
140140
externboolis_foreign_expr(PlannerInfo*root,
141141
RelOptInfo*baserel,
142142
Expr*expr);
143+
externboolis_foreign_param(PlannerInfo*root,
144+
RelOptInfo*baserel,
145+
Expr*expr);
143146
externvoiddeparseInsertSql(StringInfobuf,PlannerInfo*root,
144147
Indexrtindex,Relationrel,
145148
List*targetAttrs,booldoNothing,List*returningList,

‎contrib/postgres_fdw/sql/postgres_fdw.sql

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

671+
-- Remote aggregate in combination with a local Param (for the output
672+
-- of an initplan) can be trouble, per bug #15781
673+
explain (verbose, costs off)
674+
select exists(select1from pg_enum),sum(c1)from ft1;
675+
select exists(select1from pg_enum),sum(c1)from ft1;
676+
677+
explain (verbose, costs off)
678+
select exists(select1from pg_enum),sum(c1)from ft1group by1;
679+
select exists(select1from pg_enum),sum(c1)from ft1group by1;
680+
671681

672682
-- Testing ORDER BY, DISTINCT, FILTER, Ordered-sets and VARIADIC within aggregates
673683

0 commit comments

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