Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 7465871

Browse files
tglsfdctglsfdc
committed
Repair bogus EPQ plans generated for postgres_fdw foreign joins.
postgres_fdw's postgresGetForeignPlan() assumes without checking that the outer_plan it's given for a join relation must have a NestLoop, MergeJoin, or HashJoin node at the top. That's been wrong at least since commit 4bbf6ed (which could cause insertion of a Sort node on top) and it seems like a pretty unsafe thing to Just Assume even without that. Through blind good fortune, this doesn't seem to have any worse consequences today than strange EXPLAIN output, but it's clearly trouble waiting to happen. To fix, test the node type explicitly before touching Join-specific fields, and avoid jamming the new tlist into a node type that can't do projection. Export a new support function from createplan.c to avoid building low-level knowledge about the latter into FDWs. Back-patch to 9.6 where the faulty coding was added. Note that the associated regression test cases don't show any changes before v11, apparently because the tests back-patched with 4bbf6ed don't actually exercise the problem case before then (there's no top-level Sort in those plans). Discussion: https://postgr.es/m/8946.1544644803@sss.pgh.pa.us
1 parent 302d4ee commit 7465871

File tree

4 files changed

+140
-89
lines changed

4 files changed

+140
-89
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 72 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1709,25 +1709,27 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
17091709
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
17101710
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
17111711
Remote SQL: SELECT r1."C 1", r2."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
1712-
-> Sort
1713-
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1714-
Sort Key: t1.c3 USING <, t1.c1
1715-
-> Merge Join
1712+
-> Result
1713+
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
1714+
-> Sort
17161715
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1717-
Merge Cond: (t1.c1 = t2.c1)
1718-
-> Sort
1719-
Output: t1.c1, t1.c3, t1.*
1720-
Sort Key: t1.c1
1721-
-> Foreign Scan on public.ft1 t1
1716+
Sort Key: t1.c3, t1.c1
1717+
-> Merge Join
1718+
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1719+
Merge Cond: (t1.c1 = t2.c1)
1720+
-> Sort
17221721
Output: t1.c1, t1.c3, t1.*
1723-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1724-
-> Sort
1725-
Output: t2.c1, t2.*
1726-
Sort Key: t2.c1
1727-
-> Foreign Scan on public.ft2 t2
1722+
Sort Key: t1.c1
1723+
-> Foreign Scan on public.ft1 t1
1724+
Output: t1.c1, t1.c3, t1.*
1725+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1726+
-> Sort
17281727
Output: t2.c1, t2.*
1729-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
1730-
(26 rows)
1728+
Sort Key: t2.c1
1729+
-> Foreign Scan on public.ft2 t2
1730+
Output: t2.c1, t2.*
1731+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
1732+
(28 rows)
17311733

17321734
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
17331735
c1 | c1
@@ -1756,25 +1758,27 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
17561758
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
17571759
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
17581760
Remote SQL: SELECT r1."C 1", r2."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR UPDATE OF r1 FOR UPDATE OF r2
1759-
-> Sort
1760-
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1761-
Sort Key: t1.c3 USING <, t1.c1
1762-
-> Merge Join
1761+
-> Result
1762+
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
1763+
-> Sort
17631764
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1764-
Merge Cond: (t1.c1 = t2.c1)
1765-
-> Sort
1766-
Output: t1.c1, t1.c3, t1.*
1767-
Sort Key: t1.c1
1768-
-> Foreign Scan on public.ft1 t1
1765+
Sort Key: t1.c3, t1.c1
1766+
-> Merge Join
1767+
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1768+
Merge Cond: (t1.c1 = t2.c1)
1769+
-> Sort
17691770
Output: t1.c1, t1.c3, t1.*
1770-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1771-
-> Sort
1772-
Output: t2.c1, t2.*
1773-
Sort Key: t2.c1
1774-
-> Foreign Scan on public.ft2 t2
1771+
Sort Key: t1.c1
1772+
-> Foreign Scan on public.ft1 t1
1773+
Output: t1.c1, t1.c3, t1.*
1774+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1775+
-> Sort
17751776
Output: t2.c1, t2.*
1776-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1777-
(26 rows)
1777+
Sort Key: t2.c1
1778+
-> Foreign Scan on public.ft2 t2
1779+
Output: t2.c1, t2.*
1780+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
1781+
(28 rows)
17781782

17791783
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE;
17801784
c1 | c1
@@ -1804,25 +1808,27 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
18041808
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
18051809
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
18061810
Remote SQL: SELECT r1."C 1", r2."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR SHARE OF r1
1807-
-> Sort
1808-
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1809-
Sort Key: t1.c3 USING <, t1.c1
1810-
-> Merge Join
1811+
-> Result
1812+
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
1813+
-> Sort
18111814
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1812-
Merge Cond: (t1.c1 = t2.c1)
1813-
-> Sort
1814-
Output: t1.c1, t1.c3, t1.*
1815-
Sort Key: t1.c1
1816-
-> Foreign Scan on public.ft1 t1
1815+
Sort Key: t1.c3, t1.c1
1816+
-> Merge Join
1817+
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1818+
Merge Cond: (t1.c1 = t2.c1)
1819+
-> Sort
18171820
Output: t1.c1, t1.c3, t1.*
1818-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1819-
-> Sort
1820-
Output: t2.c1, t2.*
1821-
Sort Key: t2.c1
1822-
-> Foreign Scan on public.ft2 t2
1821+
Sort Key: t1.c1
1822+
-> Foreign Scan on public.ft1 t1
1823+
Output: t1.c1, t1.c3, t1.*
1824+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1825+
-> Sort
18231826
Output: t2.c1, t2.*
1824-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
1825-
(26 rows)
1827+
Sort Key: t2.c1
1828+
-> Foreign Scan on public.ft2 t2
1829+
Output: t2.c1, t2.*
1830+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
1831+
(28 rows)
18261832

18271833
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR SHARE OF t1;
18281834
c1 | c1
@@ -1851,25 +1857,27 @@ SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
18511857
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
18521858
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
18531859
Remote SQL: SELECT r1."C 1", r2."C 1", r1.c3, CASE WHEN (r1.*)::text IS NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8) END, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) ORDER BY r1.c3 ASC NULLS LAST, r1."C 1" ASC NULLS LAST FOR SHARE OF r1 FOR SHARE OF r2
1854-
-> Sort
1855-
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1856-
Sort Key: t1.c3 USING <, t1.c1
1857-
-> Merge Join
1860+
-> Result
1861+
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
1862+
-> Sort
18581863
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1859-
Merge Cond: (t1.c1 = t2.c1)
1860-
-> Sort
1861-
Output: t1.c1, t1.c3, t1.*
1862-
Sort Key: t1.c1
1863-
-> Foreign Scan on public.ft1 t1
1864+
Sort Key: t1.c3, t1.c1
1865+
-> Merge Join
1866+
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
1867+
Merge Cond: (t1.c1 = t2.c1)
1868+
-> Sort
18641869
Output: t1.c1, t1.c3, t1.*
1865-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1866-
-> Sort
1867-
Output: t2.c1, t2.*
1868-
Sort Key: t2.c1
1869-
-> Foreign Scan on public.ft2 t2
1870+
Sort Key: t1.c1
1871+
-> Foreign Scan on public.ft1 t1
1872+
Output: t1.c1, t1.c3, t1.*
1873+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1874+
-> Sort
18701875
Output: t2.c1, t2.*
1871-
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1872-
(26 rows)
1876+
Sort Key: t2.c1
1877+
-> Foreign Scan on public.ft2 t2
1878+
Output: t2.c1, t2.*
1879+
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
1880+
(28 rows)
18731881

18741882
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR SHARE;
18751883
c1 | c1

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,11 +1228,9 @@ postgresGetForeignPlan(PlannerInfo *root,
12281228

12291229
/*
12301230
* Ensure that the outer plan produces a tuple whose descriptor
1231-
* matches our scan tuple slot. This is safe because all scans and
1232-
* joins support projection, so we never need to insert a Result node.
1233-
* Also, remove the local conditions from outer plan's quals, lest
1234-
* they will be evaluated twice, once by the local plan and once by
1235-
* the scan.
1231+
* matches our scan tuple slot. Also, remove the local conditions
1232+
* from outer plan's quals, lest they be evaluated twice, once by the
1233+
* local plan and once by the scan.
12361234
*/
12371235
if (outer_plan)
12381236
{
@@ -1245,23 +1243,42 @@ postgresGetForeignPlan(PlannerInfo *root,
12451243
*/
12461244
Assert(!IS_UPPER_REL(foreignrel));
12471245

1248-
outer_plan->targetlist = fdw_scan_tlist;
1249-
1246+
/*
1247+
* First, update the plan's qual list if possible. In some cases
1248+
* the quals might be enforced below the topmost plan level, in
1249+
* which case we'll fail to remove them; it's not worth working
1250+
* harder than this.
1251+
*/
12501252
foreach(lc, local_exprs)
12511253
{
1252-
Join *join_plan = (Join *) outer_plan;
12531254
Node *qual = lfirst(lc);
12541255

12551256
outer_plan->qual = list_delete(outer_plan->qual, qual);
12561257

12571258
/*
12581259
* For an inner join the local conditions of foreign scan plan
1259-
* can be part of the joinquals as well.
1260+
* can be part of the joinquals as well. (They might also be
1261+
* in the mergequals or hashquals, but we can't touch those
1262+
* without breaking the plan.)
12601263
*/
1261-
if (join_plan->jointype == JOIN_INNER)
1262-
join_plan->joinqual = list_delete(join_plan->joinqual,
1263-
qual);
1264+
if (IsA(outer_plan, NestLoop) ||
1265+
IsA(outer_plan, MergeJoin) ||
1266+
IsA(outer_plan, HashJoin))
1267+
{
1268+
Join *join_plan = (Join *) outer_plan;
1269+
1270+
if (join_plan->jointype == JOIN_INNER)
1271+
join_plan->joinqual = list_delete(join_plan->joinqual,
1272+
qual);
1273+
}
12641274
}
1275+
1276+
/*
1277+
* Now fix the subplan's tlist --- this might result in inserting
1278+
* a Result node atop the plan tree.
1279+
*/
1280+
outer_plan = change_plan_targetlist(outer_plan, fdw_scan_tlist,
1281+
best_path->path.parallel_safe);
12651282
}
12661283
}
12671284

‎src/backend/optimizer/plan/createplan.c

Lines changed: 37 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,20 +1384,10 @@ create_unique_plan(PlannerInfo *root, UniquePath *best_path, int flags)
13841384
}
13851385
}
13861386

1387+
/* Use change_plan_targetlist in case we need to insert a Result node */
13871388
if (newitems || best_path->umethod == UNIQUE_PATH_SORT)
1388-
{
1389-
/*
1390-
* If the top plan node can't do projections and its existing target
1391-
* list isn't already what we need, we need to add a Result node to
1392-
* help it along.
1393-
*/
1394-
if (!is_projection_capable_plan(subplan) &&
1395-
!tlist_same_exprs(newtlist, subplan->targetlist))
1396-
subplan = inject_projection_plan(subplan, newtlist,
1397-
best_path->path.parallel_safe);
1398-
else
1399-
subplan->targetlist = newtlist;
1400-
}
1389+
subplan = change_plan_targetlist(subplan, newtlist,
1390+
best_path->path.parallel_safe);
14011391

14021392
/*
14031393
* Build control information showing which subplan output columns are to
@@ -1739,6 +1729,40 @@ inject_projection_plan(Plan *subplan, List *tlist, bool parallel_safe)
17391729
return plan;
17401730
}
17411731

1732+
/*
1733+
* change_plan_targetlist
1734+
* Externally available wrapper for inject_projection_plan.
1735+
*
1736+
* This is meant for use by FDW plan-generation functions, which might
1737+
* want to adjust the tlist computed by some subplan tree. In general,
1738+
* a Result node is needed to compute the new tlist, but we can optimize
1739+
* some cases.
1740+
*
1741+
* In most cases, tlist_parallel_safe can just be passed as the parallel_safe
1742+
* flag of the FDW's own Path node.
1743+
*/
1744+
Plan *
1745+
change_plan_targetlist(Plan *subplan, List *tlist, bool tlist_parallel_safe)
1746+
{
1747+
/*
1748+
* If the top plan node can't do projections and its existing target list
1749+
* isn't already what we need, we need to add a Result node to help it
1750+
* along.
1751+
*/
1752+
if (!is_projection_capable_plan(subplan) &&
1753+
!tlist_same_exprs(tlist, subplan->targetlist))
1754+
subplan = inject_projection_plan(subplan, tlist,
1755+
subplan->parallel_safe &&
1756+
tlist_parallel_safe);
1757+
else
1758+
{
1759+
/* Else we can just replace the plan node's tlist */
1760+
subplan->targetlist = tlist;
1761+
subplan->parallel_safe &= tlist_parallel_safe;
1762+
}
1763+
return subplan;
1764+
}
1765+
17421766
/*
17431767
* create_sort_plan
17441768
*

‎src/include/optimizer/planmain.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
5353
Index scanrelid, List *fdw_exprs, List *fdw_private,
5454
List *fdw_scan_tlist, List *fdw_recheck_quals,
5555
Plan *outer_plan);
56+
extern Plan *change_plan_targetlist(Plan *subplan, List *tlist,
57+
bool tlist_parallel_safe);
5658
extern Plan *materialize_finished_plan(Plan *subplan);
5759
extern bool is_projection_capable_path(Path *path);
5860
extern bool is_projection_capable_plan(Plan *plan);

0 commit comments

Comments
 (0)