Subject: Re: [HACKERS] postgres_fdw bug in 9.6

From: Tom Lane <[email-masked]>
Date: Sun, 03 Dec 2017 14:56:33 -0500

Robert Haas <[email-masked]> writes:
> But have a look at this:
> http://postgr.es/m/[email-masked]
> That shows a case where, before
> 5fc4c26db5120bd90348b6ee3101fcddfdf54800, a query that required the
> foreign table to do an EPQ recheck produced an unambiguously wrong
> answer; the query stipulates that inttab.a = ft1.a but the row
> returned lacks that property.  I think this clearly shows that EPQ
> rechecks are needed at least for FDW paths that are parameterized.

Certainly; that's what I said before.

> However, I guess that doesn't actually refute your point, which IIUC
> is that maybe we don't need these checks for FDW join paths, because
> we don't build parameterized paths for those yet. Now I think it would
> still be a good idea to have a way of handling that case, because
> somebody's likely going want to fix that /* XXX Consider parameterized
> paths for the join relation */ eventually, but I guess for the
> back-branches just setting epq_path = NULL instead of calling
> GetExistingLocalJoinPath() might be the way to go... assuming that
> your contention that this won't break anything is indeed correct.

I thought some more about this.  While it seems clear that we don't
actually need to recheck anything within a postgres_fdw foreign join,
there's still a problem: we have to be able to regurgitate the join
row during postgresRecheckForeignScan.  Since, in the current system
design, the only available data is scan-level rowmarks (that is,
whole-row values from each base foreign relation), there isn't any
very good way to produce the join row except to insert the rowmark
values into dummy scan nodes and then re-execute the join locally.
So really, that is what the outerPlan infrastructure is doing for us.

We could imagine that, instead of the scan-level rowmarks, the foreign
join node could produce a ROW() expression containing the values it
actually has to output, and then use that as a "join-level rowmark" to
reconstitute the join row without any of the outerPlan infrastructure.
This would have some pretty significant advantages, notably that we
could (in principle) avoid fetching all the columns of remote tables
we no longer needed scan-level rowmarks for.

However, to do that, we'd have to dynamically decide which rowmark
expressions actually need to wind up getting computed in the final join
plan, based on which joins we choose to do remotely.  The current scheme
of statically adding rowmarks to the query during base relation setup
doesn't cut it.  So that's sounding like a nontrivial rewrite (and one
that would break the related FDW APIs, although the patch at hand does
that too).

As for the question of a future extension to parameterized paths,
I think that it would likely work to recheck the parameterized qual
at the join node, making it basically just like rechecks for scan-level
nodes.  The objection to this is that it might not produce the same
answer if the parameterized qual references relations that are on the
insides of outer joins ... but I think that in practice we could just
blow off that case (ie, reject producing parameterized paths involving
such quals).  The reason is that I think such cases arise only very
rarely.  It could only happen for non-strict qual expressions, because
a strict qual would have resulted in the outer join getting strength
reduced to a regular join.

Not sure where that leaves us for the patch at hand.  It feels to me
like it's a temporary band-aid for code that we want to get rid of
in the not-too-long run.  As such, it'd be okay if it were smaller,
but it seems bigger and more invasive than I could wish for a band-aid.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 8 Dec 2017 15:53:42 -0500

> but it seems bigger and more invasive than I could wish for a band-aid.

Well, the bug report is a year old today, so we should try to do
something.  The patch needs a rebase, but reading through it, I'm not
convinced that it's particularly risky.  I mean, it's going to break
FDWs that are calling GetExistingLocalJoinPath(), but there are
probably very few third-party FDWs that do that.  Any that do are
precisely the ones that need this fix, so I think there's a good
chance they'll forgive of us for requiring them to make a code change.
I think we can contain the risk of anything else getting broken to an
acceptable level because there's not really very much other code
changing.  The changes to joinpath.c need to be carefully audited to
make sure that they are not changing the semantics, but that seems
like it shouldn't take more than an hour of code-reading to check.
The new fields in JoinPathExtraData can be moved to the end of the
struct to minimize ABI breakage.  I don't see what else there is that
could break apart from the EPQ rechecks themselves, and if that
weren't broken already, this patch wouldn't exist.

Now, if you're still super-concerned about this breaking something, we
could commit it only to master, where it will have 9 months to bake
before it gets released.  I think that's overly conservative, but I
think it's still better than waiting for the rewrite you'd like to see
happen.  We don't know when or if anyone is going to undertake that,
and if we wait, we may easing release a v11 that's got the same defect
as v9.6 and now v10.  And I don't see that we lose much by committing
this now even if that rewrite does happen in time for v11.  Ripping
out CreateLocalJoinPath() won't be any harder than ripping out
GetExistingLocalJoinPath().

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 11 Dec 2017 16:25:15 +0900

(2017/12/09 5:53), Robert Haas wrote:
> On Sun, Dec 3, 2017 at 2:56 PM, Tom Lane<[email-masked]>  wrote:

Agreed.  Attached is an rebased version which moved the new fields in 
JoinPathExtraData to the end of that struct.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 1701,1722 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Merge Join
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Merge Cond: (t1.c1 = t2.c1)
!                      ->  Sort
                             Output: t1.c1, t1.c3, t1.*
!                            Sort Key: t1.c1
!                            ->  Foreign Scan on public.ft1 t1
!                                  Output: t1.c1, t1.c3, t1.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!                      ->  Sort
                             Output: t2.c1, t2.*
!                            Sort Key: t2.c1
!                            ->  Foreign Scan on public.ft2 t2
!                                  Output: t2.c1, t2.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
  
  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;
   c1  | c1  
--- 1701,1716 ----
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Nested Loop
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Join Filter: (t1.c1 = t2.c1)
!                      ->  Foreign Scan on public.ft1 t1
                             Output: t1.c1, t1.c3, t1.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!                      ->  Foreign Scan on public.ft2 t2
                             Output: t2.c1, t2.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (17 rows)
  
  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;
   c1  | c1  
***************
*** 1745,1766 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Merge Join
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Merge Cond: (t1.c1 = t2.c1)
!                      ->  Sort
                             Output: t1.c1, t1.c3, t1.*
!                            Sort Key: t1.c1
!                            ->  Foreign Scan on public.ft1 t1
!                                  Output: t1.c1, t1.c3, t1.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!                      ->  Sort
                             Output: t2.c1, t2.*
!                            Sort Key: t2.c1
!                            ->  Foreign Scan on public.ft2 t2
!                                  Output: t2.c1, t2.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
! (23 rows)
  
  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;
   c1  | c1  
--- 1739,1754 ----
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Nested Loop
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Join Filter: (t1.c1 = t2.c1)
!                      ->  Foreign Scan on public.ft1 t1
                             Output: t1.c1, t1.c3, t1.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!                      ->  Foreign Scan on public.ft2 t2
                             Output: t2.c1, t2.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
! (17 rows)
  
  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;
   c1  | c1  
***************
*** 1790,1811 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Merge Join
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Merge Cond: (t1.c1 = t2.c1)
!                      ->  Sort
                             Output: t1.c1, t1.c3, t1.*
!                            Sort Key: t1.c1
!                            ->  Foreign Scan on public.ft1 t1
!                                  Output: t1.c1, t1.c3, t1.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
!                      ->  Sort
                             Output: t2.c1, t2.*
!                            Sort Key: t2.c1
!                            ->  Foreign Scan on public.ft2 t2
!                                  Output: t2.c1, t2.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
  
  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;
   c1  | c1  
--- 1778,1793 ----
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Nested Loop
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Join Filter: (t1.c1 = t2.c1)
!                      ->  Foreign Scan on public.ft1 t1
                             Output: t1.c1, t1.c3, t1.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
!                      ->  Foreign Scan on public.ft2 t2
                             Output: t2.c1, t2.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
! (17 rows)
  
  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;
   c1  | c1  
***************
*** 1834,1855 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Merge Join
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Merge Cond: (t1.c1 = t2.c1)
!                      ->  Sort
                             Output: t1.c1, t1.c3, t1.*
!                            Sort Key: t1.c1
!                            ->  Foreign Scan on public.ft1 t1
!                                  Output: t1.c1, t1.c3, t1.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
!                      ->  Sort
                             Output: t2.c1, t2.*
!                            Sort Key: t2.c1
!                            ->  Foreign Scan on public.ft2 t2
!                                  Output: t2.c1, t2.*
!                                  Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
! (23 rows)
  
  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;
   c1  | c1  
--- 1816,1831 ----
                 Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
                 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
                 Remote SQL: SELECT r1."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, r2."C 1", 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
!                ->  Nested Loop
                       Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
!                      Join Filter: (t1.c1 = t2.c1)
!                      ->  Foreign Scan on public.ft1 t1
                             Output: t1.c1, t1.c3, t1.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
!                      ->  Foreign Scan on public.ft2 t2
                             Output: t2.c1, t2.*
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR SHARE
! (17 rows)
  
  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;
   c1  | c1  
***************
*** 1866,1871 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
--- 1842,1943 ----
   110 | 110
  (10 rows)
  
+ -- FOR UPDATE/SHARE in situations where a full join is pushed down
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (t2.c1 = t3.c1)) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
+                                                                                                                                                      QUERY PLAN                                                                                                                                                      
+ ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+  LockRows
+    Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+    ->  Nested Loop
+          Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+          ->  Foreign Scan
+                Output: t2.c1, t2.*, t3.c1, t3.*
+                Relations: (public.ft4 t2) FULL JOIN (public.ft5 t3)
+                Remote SQL: SELECT r2.c1, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END, r3.c1, CASE WHEN (r3.*)::text IS NOT NULL THEN ROW(r3.c1, r3.c2, r3.c3) END FROM ("S 1"."T 3" r2 FULL JOIN "S 1"."T 4" r3 ON (((r2.c1 = r3.c1)))) ORDER BY r2.c1 ASC NULLS LAST, r3.c1 ASC NULLS LAST
+                ->  Hash Full Join
+                      Output: t2.c1, t2.*, t3.c1, t3.*
+                      Hash Cond: (t2.c1 = t3.c1)
+                      ->  Foreign Scan on public.ft4 t2
+                            Output: t2.c1, t2.*
+                            Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+                      ->  Hash
+                            Output: t3.c1, t3.*
+                            ->  Foreign Scan on public.ft5 t3
+                                  Output: t3.c1, t3.*
+                                  Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+          ->  Materialize
+                Output: "T 3".c1, "T 3".ctid
+                ->  Seq Scan on "S 1"."T 3"
+                      Output: "T 3".c1, "T 3".ctid
+                      Filter: ("T 3".c1 = 50)
+ (24 rows)
+ 
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (t2.c1::bit(16) = t3.c1::bit(16))) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
+                                                                                                                                        QUERY PLAN                                                                                                                                        
+ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+  LockRows
+    Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+    ->  Sort
+          Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+          Sort Key: t2.c1, t3.c1
+          ->  Nested Loop
+                Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+                ->  Seq Scan on "S 1"."T 3"
+                      Output: "T 3".c1, "T 3".ctid
+                      Filter: ("T 3".c1 = 50)
+                ->  Foreign Scan
+                      Output: t2.c1, t2.*, t3.c1, t3.*
+                      Relations: (public.ft4 t2) FULL JOIN (public.ft5 t3)
+                      Remote SQL: SELECT r2.c1, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END, r3.c1, CASE WHEN (r3.*)::text IS NOT NULL THEN ROW(r3.c1, r3.c2, r3.c3) END FROM ("S 1"."T 3" r2 FULL JOIN "S 1"."T 4" r3 ON (((r2.c1::bit(16) = r3.c1::bit(16)))))
+                      ->  Merge Full Join
+                            Output: t2.c1, t2.*, t3.c1, t3.*
+                            Merge Cond: (((t2.c1)::bit(16)) = ((t3.c1)::bit(16)))
+                            ->  Sort
+                                  Output: t2.c1, t2.*, ((t2.c1)::bit(16))
+                                  Sort Key: ((t2.c1)::bit(16))
+                                  ->  Foreign Scan on public.ft4 t2
+                                        Output: t2.c1, t2.*, (t2.c1)::bit(16)
+                                        Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+                            ->  Sort
+                                  Output: t3.c1, t3.*, ((t3.c1)::bit(16))
+                                  Sort Key: ((t3.c1)::bit(16))
+                                  ->  Foreign Scan on public.ft5 t3
+                                        Output: t3.c1, t3.*, (t3.c1)::bit(16)
+                                        Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+ (29 rows)
+ 
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (FALSE)) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
+                                                                                                                                                 QUERY PLAN                                                                                                                                                 
+ -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
+  LockRows
+    Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+    ->  Nested Loop
+          Output: "T 3".c1, t2.c1, t3.c1, "T 3".ctid, t2.*, t3.*
+          ->  Foreign Scan
+                Output: t2.c1, t2.*, t3.c1, t3.*
+                Relations: (public.ft4 t2) FULL JOIN (public.ft5 t3)
+                Remote SQL: SELECT r2.c1, CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2.c1, r2.c2, r2.c3) END, r3.c1, CASE WHEN (r3.*)::text IS NOT NULL THEN ROW(r3.c1, r3.c2, r3.c3) END FROM ("S 1"."T 3" r2 FULL JOIN "S 1"."T 4" r3 ON ((false))) ORDER BY r2.c1 ASC NULLS LAST, r3.c1 ASC NULLS LAST
+                ->  Merge Full Join
+                      Output: t2.c1, t2.*, t3.c1, t3.*
+                      Join Filter: false
+                      ->  Foreign Scan on public.ft4 t2
+                            Output: t2.c1, t2.*
+                            Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
+                      ->  Materialize
+                            Output: t3.c1, t3.*
+                            ->  Foreign Scan on public.ft5 t3
+                                  Output: t3.c1, t3.*
+                                  Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4"
+          ->  Materialize
+                Output: "T 3".c1, "T 3".ctid
+                ->  Seq Scan on "S 1"."T 3"
+                      Output: "T 3".c1, "T 3".ctid
+                      Filter: ("T 3".c1 = 50)
+ (24 rows)
+ 
  -- join in CTE
  EXPLAIN (VERBOSE, COSTS OFF)
  WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
***************
*** 4300,4317 **** UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
           Output: ft2.c1, (ft2.c2 + 500), NULL::integer, (ft2.c3 || '_update9'::text), ft2.c4, ft2.c5, ft2.c6, 'ft2       '::character(10), ft2.c8, ft2.ctid, ft1.*
           Relations: (public.ft2) INNER JOIN (public.ft1)
           Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c8, r1.ctid, 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.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9)))) FOR UPDATE OF r1
!          ->  Hash Join
                 Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c8, ft2.ctid, ft1.*
!                Hash Cond: (ft2.c2 = ft1.c1)
                 ->  Foreign Scan on public.ft2
                       Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c8, ft2.ctid
                       Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c8, ctid FROM "S 1"."T 1" FOR UPDATE
!                ->  Hash
                       Output: ft1.*, ft1.c1
!                      ->  Foreign Scan on public.ft1
!                            Output: ft1.*, ft1.c1
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((("C 1" % 10) = 9))
! (17 rows)
  
  UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
    FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
--- 4372,4387 ----
           Output: ft2.c1, (ft2.c2 + 500), NULL::integer, (ft2.c3 || '_update9'::text), ft2.c4, ft2.c5, ft2.c6, 'ft2       '::character(10), ft2.c8, ft2.ctid, ft1.*
           Relations: (public.ft2) INNER JOIN (public.ft1)
           Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c8, r1.ctid, 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.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 9)))) FOR UPDATE OF r1
!          ->  Nested Loop
                 Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c8, ft2.ctid, ft1.*
!                Join Filter: (ft2.c2 = ft1.c1)
                 ->  Foreign Scan on public.ft2
                       Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c8, ft2.ctid
                       Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c8, ctid FROM "S 1"."T 1" FOR UPDATE
!                ->  Foreign Scan on public.ft1
                       Output: ft1.*, ft1.c1
!                      Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((("C 1" % 10) = 9))
! (15 rows)
  
  UPDATE ft2 SET c2 = ft2.c2 + 500, c3 = ft2.c3 || '_update9', c7 = DEFAULT
    FROM ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 9;
***************
*** 4443,4460 **** DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
           Output: ft2.ctid, ft1.*
           Relations: (public.ft2) INNER JOIN (public.ft1)
           Remote SQL: SELECT r1.ctid, 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.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 2)))) FOR UPDATE OF r1
!          ->  Hash Join
                 Output: ft2.ctid, ft1.*
!                Hash Cond: (ft2.c2 = ft1.c1)
                 ->  Foreign Scan on public.ft2
                       Output: ft2.ctid, ft2.c2
                       Remote SQL: SELECT c2, ctid FROM "S 1"."T 1" FOR UPDATE
!                ->  Hash
                       Output: ft1.*, ft1.c1
!                      ->  Foreign Scan on public.ft1
!                            Output: ft1.*, ft1.c1
!                            Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((("C 1" % 10) = 2))
! (17 rows)
  
  DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
  SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
--- 4513,4528 ----
           Output: ft2.ctid, ft1.*
           Relations: (public.ft2) INNER JOIN (public.ft1)
           Remote SQL: SELECT r1.ctid, 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.c2 = r2."C 1")) AND (((r2."C 1" % 10) = 2)))) FOR UPDATE OF r1
!          ->  Nested Loop
                 Output: ft2.ctid, ft1.*
!                Join Filter: (ft2.c2 = ft1.c1)
                 ->  Foreign Scan on public.ft2
                       Output: ft2.ctid, ft2.c2
                       Remote SQL: SELECT c2, ctid FROM "S 1"."T 1" FOR UPDATE
!                ->  Foreign Scan on public.ft1
                       Output: ft1.*, ft1.c1
!                      Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((("C 1" % 10) = 2))
! (15 rows)
  
  DELETE FROM ft2 USING ft1 WHERE ft1.c1 = ft2.c2 AND ft1.c1 % 10 = 2;
  SELECT c1,c2,c3,c4 FROM ft2 ORDER BY c1;
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 4494,4514 **** postgresGetForeignJoinPaths(PlannerInfo *root,
  	/*
  	 * If there is a possibility that EvalPlanQual will be executed, we need
  	 * to be able to reconstruct the row using scans of the base relations.
! 	 * GetExistingLocalJoinPath will find a suitable path for this purpose in
! 	 * the path list of the joinrel, if one exists.  We must be careful to
! 	 * call it before adding any ForeignPath, since the ForeignPath might
! 	 * dominate the only suitable local path available.  We also do it before
! 	 * calling foreign_join_ok(), since that function updates fpinfo and marks
! 	 * it as pushable if the join is found to be pushable.
  	 */
  	if (root->parse->commandType == CMD_DELETE ||
  		root->parse->commandType == CMD_UPDATE ||
  		root->rowMarks)
  	{
! 		epq_path = GetExistingLocalJoinPath(joinrel);
  		if (!epq_path)
  		{
! 			elog(DEBUG3, "could not push down foreign join because a local path suitable for EPQ checks was not found");
  			return;
  		}
  	}
--- 4494,4514 ----
  	/*
  	 * If there is a possibility that EvalPlanQual will be executed, we need
  	 * to be able to reconstruct the row using scans of the base relations.
! 	 * CreateLocalJoinPath will build an alternative local join path for this
! 	 * purpose.  We must be careful to call it before calling foreign_join_ok,
! 	 * since that function updates fpinfo and marks it as pushable if the join
! 	 * is found to be pushable.
  	 */
  	if (root->parse->commandType == CMD_DELETE ||
  		root->parse->commandType == CMD_UPDATE ||
  		root->rowMarks)
  	{
! 		/* Create a local join path */
! 		epq_path = CreateLocalJoinPath(root, joinrel, outerrel, innerrel,
! 									   jointype, extra);
  		if (!epq_path)
  		{
! 			elog(DEBUG3, "could not push down foreign join because a local path suitable for EPQ checks could not be created");
  			return;
  		}
  	}
***************
*** 4517,4523 **** postgresGetForeignJoinPaths(PlannerInfo *root,
  
  	if (!foreign_join_ok(root, joinrel, jointype, outerrel, innerrel, extra))
  	{
! 		/* Free path required for EPQ if we copied one; we don't need it now */
  		if (epq_path)
  			pfree(epq_path);
  		return;
--- 4517,4523 ----
  
  	if (!foreign_join_ok(root, joinrel, jointype, outerrel, innerrel, extra))
  	{
! 		/* Free path required for EPQ if we created one; we don't need it now */
  		if (epq_path)
  			pfree(epq_path);
  		return;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 493,498 **** SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t
--- 493,505 ----
  EXPLAIN (VERBOSE, COSTS OFF)
  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;
  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;
+ -- FOR UPDATE/SHARE in situations where a full join is pushed down
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (t2.c1 = t3.c1)) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (t2.c1::bit(16) = t3.c1::bit(16))) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT t1.c1, t2.c1, t3.c1 FROM (SELECT c1 FROM "S 1"."T 3" WHERE c1 = 50) t1 INNER JOIN (ft4 t2 FULL JOIN ft5 t3 ON (FALSE)) ON (TRUE) ORDER BY t1.c1, t2.c1, t3.c1 FOR UPDATE OF t1;
  -- join in CTE
  EXPLAIN (VERBOSE, COSTS OFF)
  WITH t (c1_1, c1_3, c2_1) AS (SELECT t1.c1, t1.c3, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1)) SELECT c1_1, c2_1 FROM t ORDER BY c1_3, c1_1 OFFSET 100 LIMIT 10;
*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***************
*** 999,1009 **** RecheckForeignScan(ForeignScanState *node,
       can be executed and the resulting tuple can be stored in the slot.
       This plan need not be efficient since no base table will return more
       than one row; for example, it may implement all joins as nested loops.
!      The function <literal>GetExistingLocalJoinPath</literal> may be used to search
!      existing paths for a suitable local join path, which can be used as the
!      alternative local join plan.  <literal>GetExistingLocalJoinPath</literal>
!      searches for an unparameterized path in the path list of the specified
!      join relation.  (If it does not find such a path, it returns NULL, in
       which case a foreign data wrapper may build the local path by itself or
       may choose not to create access paths for that join.)
      </para>
--- 999,1007 ----
       can be executed and the resulting tuple can be stored in the slot.
       This plan need not be efficient since no base table will return more
       than one row; for example, it may implement all joins as nested loops.
!      The function <literal>CreateLocalJoinPath</literal> may be used to build
!      a suitable local join path, which can be used as the alternative
!      local join plan.  (If it does not build such a path, it returns NULL, in
       which case a foreign data wrapper may build the local path by itself or
       may choose not to create access paths for that join.)
      </para>
*** a/src/backend/foreign/foreign.c
--- b/src/backend/foreign/foreign.c
***************
*** 22,27 ****
--- 22,30 ----
  #include "foreign/foreign.h"
  #include "lib/stringinfo.h"
  #include "miscadmin.h"
+ #include "optimizer/cost.h"
+ #include "optimizer/pathnode.h"
+ #include "optimizer/paths.h"
  #include "utils/builtins.h"
  #include "utils/memutils.h"
  #include "utils/rel.h"
***************
*** 689,801 **** get_foreign_server_oid(const char *servername, bool missing_ok)
  	return oid;
  }
  
  /*
!  * Get a copy of an existing local path for a given join relation.
!  *
!  * This function is usually helpful to obtain an alternate local path for EPQ
!  * checks.
!  *
!  * Right now, this function only supports unparameterized foreign joins, so we
!  * only search for unparameterized path in the given list of paths. Since we
!  * are searching for a path which can be used to construct an alternative local
!  * plan for a foreign join, we look for only MergeJoin, HashJoin or NestLoop
!  * paths.
!  *
!  * If the inner or outer subpath of the chosen path is a ForeignScan, we
!  * replace it with its outer subpath.  For this reason, and also because the
!  * planner might free the original path later, the path returned by this
!  * function is a shallow copy of the original.  There's no need to copy
!  * the substructure, so we don't.
   *
!  * Since the plan created using this path will presumably only be used to
!  * execute EPQ checks, efficiency of the path is not a concern. But since the
!  * path list in RelOptInfo is anyway sorted by total cost we are likely to
!  * choose the most efficient path, which is all for the best.
   */
! extern Path *
! GetExistingLocalJoinPath(RelOptInfo *joinrel)
  {
! 	ListCell   *lc;
  
! 	Assert(IS_JOIN_REL(joinrel));
  
! 	foreach(lc, joinrel->pathlist)
  	{
! 		Path	   *path = (Path *) lfirst(lc);
! 		JoinPath   *joinpath = NULL;
  
! 		/* Skip parameterized paths. */
! 		if (path->param_info != NULL)
! 			continue;
  
! 		switch (path->pathtype)
! 		{
! 			case T_HashJoin:
  				{
! 					HashPath   *hash_path = makeNode(HashPath);
! 
! 					memcpy(hash_path, path, sizeof(HashPath));
! 					joinpath = (JoinPath *) hash_path;
  				}
! 				break;
! 
! 			case T_NestLoop:
  				{
! 					NestPath   *nest_path = makeNode(NestPath);
! 
! 					memcpy(nest_path, path, sizeof(NestPath));
! 					joinpath = (JoinPath *) nest_path;
! 				}
! 				break;
  
! 			case T_MergeJoin:
! 				{
! 					MergePath  *merge_path = makeNode(MergePath);
  
! 					memcpy(merge_path, path, sizeof(MergePath));
! 					joinpath = (JoinPath *) merge_path;
  				}
! 				break;
! 
! 			default:
! 
! 				/*
! 				 * Just skip anything else. We don't know if corresponding
! 				 * plan would build the output row from whole-row references
! 				 * of base relations and execute the EPQ checks.
! 				 */
! 				break;
! 		}
! 
! 		/* This path isn't good for us, check next. */
! 		if (!joinpath)
! 			continue;
! 
! 		/*
! 		 * If either inner or outer path is a ForeignPath corresponding to a
! 		 * pushed down join, replace it with the fdw_outerpath, so that we
! 		 * maintain path for EPQ checks built entirely of local join
! 		 * strategies.
! 		 */
! 		if (IsA(joinpath->outerjoinpath, ForeignPath))
! 		{
! 			ForeignPath *foreign_path;
! 
! 			foreign_path = (ForeignPath *) joinpath->outerjoinpath;
! 			if (IS_JOIN_REL(foreign_path->path.parent))
! 				joinpath->outerjoinpath = foreign_path->fdw_outerpath;
! 		}
! 
! 		if (IsA(joinpath->innerjoinpath, ForeignPath))
! 		{
! 			ForeignPath *foreign_path;
! 
! 			foreign_path = (ForeignPath *) joinpath->innerjoinpath;
! 			if (IS_JOIN_REL(foreign_path->path.parent))
! 				joinpath->innerjoinpath = foreign_path->fdw_outerpath;
! 		}
! 
! 		return (Path *) joinpath;
  	}
! 	return NULL;
  }
--- 692,873 ----
  	return oid;
  }
  
+ 
  /*
!  * Try to build an alternative local join path to reconstruct a join tuple for
!  * the foreign join in EvalPlanQual
   *
!  * 'joinrel' is the join relation
!  * 'outerrel' is the outer join relation
!  * 'innerrel' is the inner join relation
!  * 'jointype' is the type of join to do
!  * 'extra' contains additional input values
   */
! Path *
! CreateLocalJoinPath(PlannerInfo *root,
! 					RelOptInfo *joinrel,
! 					RelOptInfo *outerrel,
! 					RelOptInfo *innerrel,
! 					JoinType jointype,
! 					JoinPathExtraData *extra)
  {
! 	Path	   *result;
! 	Path	   *outer_path = outerrel->cheapest_total_path;
! 	Path	   *inner_path = innerrel->cheapest_total_path;
  
! 	/*
! 	 * We don't try to build the alternative local join path if the cheapest
! 	 * total outer and inner paths are parameterized; that seems unlikely in
! 	 * normal case.
! 	 */
! 	if (outer_path->param_info != NULL || inner_path->param_info != NULL)
! 		return NULL;
  
! 	switch (jointype)
  	{
! 		case JOIN_INNER:
! 		case JOIN_LEFT:
! 		case JOIN_SEMI:
! 		case JOIN_ANTI:
! 			{
! 				JoinCostWorkspace workspace;
  
! 				/* Get an initial estimate */
! 				initial_cost_nestloop(root, &workspace, jointype,
! 									  outer_path, inner_path, extra);
! 				/* Generate a nestloop path */
! 				result = (Path *) create_nestloop_path(root,
! 													   joinrel,
! 													   jointype,
! 													   &workspace,
! 													   extra,
! 													   outer_path,
! 													   inner_path,
! 													   extra->restrictlist,
! 													   NIL,
! 													   NULL);
! 			}
! 			break;
! 		case JOIN_RIGHT:
! 		case JOIN_FULL:
! 			{
! 				JoinCostWorkspace workspace;
  
! 				/* Generate a hashjoin or mergejoin path, if possible */
! 				if (extra->hashclause_list)
  				{
! 					/* Get an initial estimate */
! 					initial_cost_hashjoin(root, &workspace, jointype,
! 										  extra->hashclause_list,
! 										  outer_path, inner_path, extra);
! 					/* Generate a hashjoin path */
! 					result = (Path *) create_hashjoin_path(root,
! 														   joinrel,
! 														   jointype,
! 														   &workspace,
! 														   extra,
! 														   outer_path,
! 														   inner_path,
! 														   extra->restrictlist,
! 														   NULL,
! 														   extra->hashclause_list);
  				}
! 				else if (extra->mergejoin_allowed)
  				{
! 					/*
! 					 * If right or full join with no mergejoinable clauses,
! 					 * create a clauseless mergejoin path.  Otherwise create a
! 					 * mergejoin path by explicitly sorting both the outer and
! 					 * inner relations.
! 					 */
! 					if (!extra->mergeclause_list)
! 					{
! 						/* Get an initial estimate */
! 						initial_cost_mergejoin(root, &workspace, jointype,
! 											   NIL, outer_path, inner_path,
! 											   NIL, NIL, extra);
! 						/* Generate a mergejoin path */
! 						result = (Path *) create_mergejoin_path(root,
! 																joinrel,
! 																jointype,
! 																&workspace,
! 																extra,
! 																outer_path,
! 																inner_path,
! 																extra->restrictlist,
! 																NIL,
! 																NULL,
! 																NIL,
! 																NIL,
! 																NIL);
! 					}
! 					else
! 					{
! 						List	   *mergeclauses;
! 						List	   *outerkeys;
! 						List	   *innerkeys;
  
! 						/* Build sort pathkeys for the outer side */
! 						outerkeys = select_outer_pathkeys_for_merge(root,
! 																	extra->mergeclause_list,
! 																	joinrel);
! 						/* Sort the mergeclauses into the corresponding ordering */
! 						mergeclauses = find_mergeclauses_for_pathkeys(root,
! 																	  outerkeys,
! 																	  true,
! 																	  extra->mergeclause_list);
! 						/* Should have used them all... */
! 						Assert(list_length(mergeclauses) == list_length(extra->mergeclause_list));
! 						/* Build sort pathkeys for the inner side */
! 						innerkeys = make_inner_pathkeys_for_merge(root,
! 																  mergeclauses,
! 																  outerkeys);
! 						/*
! 						 * It's possible that the cheapest total paths will
! 						 * already be sorted properly; if so, suppress an
! 						 * explicit sort.
! 						 */
! 						if (outerkeys &&
! 							pathkeys_contained_in(outerkeys,
! 												  outer_path->pathkeys))
! 							outerkeys = NIL;
! 						if (innerkeys &&
! 							pathkeys_contained_in(innerkeys,
! 												  inner_path->pathkeys))
! 							innerkeys = NIL;
  
! 						/* Get an initial estimate */
! 						initial_cost_mergejoin(root, &workspace, jointype,
! 											   mergeclauses,
! 											   outer_path, inner_path,
! 											   outerkeys, innerkeys, extra);
! 						/* Generate a mergejoin path */
! 						result = (Path *) create_mergejoin_path(root,
! 																joinrel,
! 																jointype,
! 																&workspace,
! 																extra,
! 																outer_path,
! 																inner_path,
! 																extra->restrictlist,
! 																NIL,
! 																NULL,
! 																mergeclauses,
! 																outerkeys,
! 																innerkeys);
! 					}
  				}
! 				else
! 					result = NULL;
! 			}
! 			break;
! 		default:
! 			/* other values not expected here */
! 			elog(ERROR, "unrecognized join type: %d",
! 				 (int) jointype);
! 			result = NULL; /* keep compiler quiet */
! 			break;
  	}
! 
! 	return result;
  }
*** a/src/backend/optimizer/path/joinpath.c
--- b/src/backend/optimizer/path/joinpath.c
***************
*** 78,83 **** static List *select_mergejoin_clauses(PlannerInfo *root,
--- 78,87 ----
  						 List *restrictlist,
  						 JoinType jointype,
  						 bool *mergejoin_allowed);
+ static List *select_hashjoin_clauses(RelOptInfo *outerrel,
+ 						RelOptInfo *innerrel,
+ 						List *restrictlist,
+ 						JoinType jointype);
  static void generate_mergejoin_paths(PlannerInfo *root,
  						 RelOptInfo *joinrel,
  						 RelOptInfo *innerrel,
***************
*** 123,129 **** add_paths_to_joinrel(PlannerInfo *root,
  					 List *restrictlist)
  {
  	JoinPathExtraData extra;
- 	bool		mergejoin_allowed = true;
  	ListCell   *lc;
  	Relids		joinrelids;
  
--- 127,132 ----
***************
*** 143,148 **** add_paths_to_joinrel(PlannerInfo *root,
--- 146,153 ----
  	extra.mergeclause_list = NIL;
  	extra.sjinfo = sjinfo;
  	extra.param_source_rels = NULL;
+ 	extra.mergejoin_allowed = true;
+ 	extra.hashclause_list = NIL;
  
  	/*
  	 * See if the inner relation is provably unique for this outer rel.
***************
*** 200,206 **** add_paths_to_joinrel(PlannerInfo *root,
  														  innerrel,
  														  restrictlist,
  														  jointype,
! 														  &mergejoin_allowed);
  
  	/*
  	 * If it's SEMI, ANTI, or inner_unique join, compute correction factors
--- 205,226 ----
  														  innerrel,
  														  restrictlist,
  														  jointype,
! 													&extra.mergejoin_allowed);
! 
! 	/*
! 	 * Find hashjoin clauses.  As above, skip this if we are not interested in
! 	 * doing a hashjoin, but disregard enable_hashjoin for full joins, because
! 	 * there may be no other alternative.
! 	 *
! 	 * Note: we need to build only one hashclause list for any given pair of
! 	 * outer and inner relations; all of the hashable clauses will be used as
! 	 * keys.
! 	 */
! 	if (enable_hashjoin || jointype == JOIN_FULL)
! 		extra.hashclause_list = select_hashjoin_clauses(outerrel,
! 														innerrel,
! 														restrictlist,
! 														jointype);
  
  	/*
  	 * If it's SEMI, ANTI, or inner_unique join, compute correction factors
***************
*** 263,269 **** add_paths_to_joinrel(PlannerInfo *root,
  	 * 1. Consider mergejoin paths where both relations must be explicitly
  	 * sorted.  Skip this if we can't mergejoin.
  	 */
! 	if (mergejoin_allowed)
  		sort_inner_and_outer(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  
--- 283,289 ----
  	 * 1. Consider mergejoin paths where both relations must be explicitly
  	 * sorted.  Skip this if we can't mergejoin.
  	 */
! 	if (extra.mergejoin_allowed)
  		sort_inner_and_outer(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  
***************
*** 274,280 **** add_paths_to_joinrel(PlannerInfo *root,
  	 * (That's okay because we know that nestloop can't handle right/full
  	 * joins at all, so it wouldn't work in the prohibited cases either.)
  	 */
! 	if (mergejoin_allowed)
  		match_unsorted_outer(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  
--- 294,300 ----
  	 * (That's okay because we know that nestloop can't handle right/full
  	 * joins at all, so it wouldn't work in the prohibited cases either.)
  	 */
! 	if (extra.mergejoin_allowed)
  		match_unsorted_outer(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  
***************
*** 291,297 **** add_paths_to_joinrel(PlannerInfo *root,
  	 * those made by match_unsorted_outer when add_paths_to_joinrel() is
  	 * invoked with the two rels given in the other order.
  	 */
! 	if (mergejoin_allowed)
  		match_unsorted_inner(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  #endif
--- 311,317 ----
  	 * those made by match_unsorted_outer when add_paths_to_joinrel() is
  	 * invoked with the two rels given in the other order.
  	 */
! 	if (extra.mergejoin_allowed)
  		match_unsorted_inner(root, joinrel, outerrel, innerrel,
  							 jointype, &extra);
  #endif
***************
*** 1675,1715 **** hash_inner_and_outer(PlannerInfo *root,
  					 JoinPathExtraData *extra)
  {
  	JoinType	save_jointype = jointype;
! 	bool		isouterjoin = IS_OUTER_JOIN(jointype);
! 	List	   *hashclauses;
! 	ListCell   *l;
! 
! 	/*
! 	 * We need to build only one hashclauses list for any given pair of outer
! 	 * and inner relations; all of the hashable clauses will be used as keys.
! 	 *
! 	 * Scan the join's restrictinfo list to find hashjoinable clauses that are
! 	 * usable with this pair of sub-relations.
! 	 */
! 	hashclauses = NIL;
! 	foreach(l, extra->restrictlist)
! 	{
! 		RestrictInfo *restrictinfo = (RestrictInfo *) lfirst(l);
! 
! 		/*
! 		 * If processing an outer join, only use its own join clauses for
! 		 * hashing.  For inner joins we need not be so picky.
! 		 */
! 		if (isouterjoin && restrictinfo->is_pushed_down)
! 			continue;
! 
! 		if (!restrictinfo->can_join ||
! 			restrictinfo->hashjoinoperator == InvalidOid)
! 			continue;			/* not hashjoinable */
! 
! 		/*
! 		 * Check if clause has the form "outer op inner" or "inner op outer".
! 		 */
! 		if (!clause_sides_match_join(restrictinfo, outerrel, innerrel))
! 			continue;			/* no good for these input relations */
! 
! 		hashclauses = lappend(hashclauses, restrictinfo);
! 	}
  
  	/* If we found any usable hashclauses, make paths */
  	if (hashclauses)
--- 1695,1701 ----
  					 JoinPathExtraData *extra)
  {
  	JoinType	save_jointype = jointype;
! 	List	   *hashclauses = extra->hashclause_list;
  
  	/* If we found any usable hashclauses, make paths */
  	if (hashclauses)
***************
*** 1996,1998 **** select_mergejoin_clauses(PlannerInfo *root,
--- 1982,2034 ----
  
  	return result_list;
  }
+ 
+ /*
+  * select_hashjoin_clauses
+  *	  Select hashjoin clauses that are usable for a particular join.
+  *	  Returns a list of RestrictInfo nodes for those clauses.
+  *
+  * We mark each selected RestrictInfo to show which side is currently
+  * being considered as outer.  These are transient markings that are only
+  * good for the duration of the current add_paths_to_joinrel() call!
+  *
+  * We examine each restrictinfo clause known for the join to see
+  * if it is hashjoinable and involves vars from the two sub-relations
+  * currently of interest.
+  */
+ static List *
+ select_hashjoin_clauses(RelOptInfo *outerrel,
+ 						RelOptInfo *innerrel,
+ 						List *restrictlist,
+ 						JoinType jointype)
+ {
+ 	List	   *hashclauses = NIL;
+ 	bool		isouterjoin = IS_OUTER_JOIN(jointype);
+ 	ListCell   *l;
+ 
+ 	foreach(l, restrictlist)
+ 	{
+ 		RestrictInfo *restrictinfo = (RestrictInfo *) lfirst(l);
+ 
+ 		/*
+ 		 * If processing an outer join, only use its own join clauses for
+ 		 * hashing.  For inner joins we need not be so picky.
+ 		 */
+ 		if (isouterjoin && restrictinfo->is_pushed_down)
+ 			continue;
+ 
+ 		if (!restrictinfo->can_join ||
+ 			restrictinfo->hashjoinoperator == InvalidOid)
+ 			continue;			/* not hashjoinable */
+ 
+ 		/*
+ 		 * Check if clause has the form "outer op inner" or "inner op outer".
+ 		 */
+ 		if (!clause_sides_match_join(restrictinfo, outerrel, innerrel))
+ 			continue;			/* no good for these input relations */
+ 
+ 		hashclauses = lappend(hashclauses, restrictinfo);
+ 	}
+ 
+ 	return hashclauses;
+ }
*** a/src/include/foreign/fdwapi.h
--- b/src/include/foreign/fdwapi.h
***************
*** 247,252 **** extern FdwRoutine *GetFdwRoutineByRelId(Oid relid);
  extern FdwRoutine *GetFdwRoutineForRelation(Relation relation, bool makecopy);
  extern bool IsImportableForeignTable(const char *tablename,
  						 ImportForeignSchemaStmt *stmt);
! extern Path *GetExistingLocalJoinPath(RelOptInfo *joinrel);
  
  #endif							/* FDWAPI_H */
--- 247,257 ----
  extern FdwRoutine *GetFdwRoutineForRelation(Relation relation, bool makecopy);
  extern bool IsImportableForeignTable(const char *tablename,
  						 ImportForeignSchemaStmt *stmt);
! extern Path *CreateLocalJoinPath(PlannerInfo *root,
! 					RelOptInfo *joinrel,
! 					RelOptInfo *outerrel,
! 					RelOptInfo *innerrel,
! 					JoinType jointype,
! 					JoinPathExtraData *extra);
  
  #endif							/* FDWAPI_H */
*** a/src/include/nodes/relation.h
--- b/src/include/nodes/relation.h
***************
*** 2270,2275 **** typedef struct SemiAntiJoinFactors
--- 2270,2278 ----
   * sjinfo is extra info about special joins for selectivity estimation
   * semifactors is as shown above (only valid for SEMI/ANTI/inner_unique joins)
   * param_source_rels are OK targets for parameterization of result paths
+  * mergejoin_allowed is a flag to indicate whether mergejoins are allowed
+  * hashclause_list is a list of RestrictInfo nodes for available
+  *		hashjoin clauses in this join
   */
  typedef struct JoinPathExtraData
  {
***************
*** 2279,2284 **** typedef struct JoinPathExtraData
--- 2282,2289 ----
  	SpecialJoinInfo *sjinfo;
  	SemiAntiJoinFactors semifactors;
  	Relids		param_source_rels;
+ 	bool		mergejoin_allowed;
+ 	List	   *hashclause_list;
  } JoinPathExtraData;
  
  /*

---

From: Thomas Munro <[email-masked]>
Date: Fri, 12 Jan 2018 14:41:06 +1300

> JoinPathExtraData to the end of that struct.

FYI this doesn't compile anymore, because initial_cost_hashjoin() and
create_hashjoin_path() changed in master.

-- 
Thomas Munro
http://www.enterprisedb.com

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 12 Jan 2018 16:15:22 +0900

> create_hashjoin_path() changed in master.

Thank you for letting me know about that!  Here is an updated version.

Best regards,
Etsuro Fujita

  

  

  

  

  
  
  /*

  

! 			case T_MergeJoin:
! 				{
  
--- 692,875 ----
  	return oid;
  }
  
+ 
  /*
  
! 	/*
  
! 	switch (jointype)
  	{
! 		case JOIN_INNER:
! 		case JOIN_LEFT:
! 		case JOIN_SEMI:
! 		case JOIN_ANTI:
! 			{

! 										  extra, false);
! 														   false,

  
--- 127,132 ----
***************
  
  	/*
  
  	/*
  
  	/*
  
--- 283,289 ----
  
***************
  
--- 294,300 ----
  
***************
*** 1682,1722 **** hash_inner_and_outer(PlannerInfo *root,
  
--- 1702,1708 ----
  
  	/* If we found any usable hashclauses, make paths */
  	if (hashclauses)
***************
*** 2024,2026 **** select_mergejoin_clauses(PlannerInfo *root,
--- 2010,2062 ----
  
  	return result_list;
  }
+ 
+ /*
  
  #endif							/* FDWAPI_H */
--- 247,257 ----
  
  #endif							/* FDWAPI_H */
*** 2271,2276 **** typedef struct SemiAntiJoinFactors
--- 2271,2279 ----
*** 2280,2285 **** typedef struct JoinPathExtraData
--- 2283,2290 ----
  
  /*

---

From: Robert Haas <[email-masked]>
Date: Fri, 12 Jan 2018 16:07:09 -0500

> So really, that is what the outerPlan infrastructure is doing for us.

I started to look at this patch again today and got cold feet.  It
seems to me that this entire design on the posted patch is based on
your remarks in http://postgr.es/m/[email-masked] --

# One way that we could make things better is to rely on the knowledge
# that EPQ isn't asked to evaluate joins for more than one row per input
# relation, and therefore using merge or hash join technology is really
# overkill.  We could make a tree of simple nestloop joins, which aren't
# going to care about sort order, if we could identify the correct join
# clauses to apply.

However, those remarks are almost entirely wrong.  It's true that the
relations for which we have EPQ tuples will only ever output a single
tuple, but because of what you said in the quoted text above, this
does NOT imply that the recheck plan never needs to worry about
dealing with a large number of rows, as the following example shows.
(The part about making a tree of simple nestloop joins is wrong not
only for this reason but because it won't work in the executor, as
previously discussed.)

rhaas=# explain select * from foo join (bar join baz on bar.a = baz.a)
on foo.a = bar.a where foo.b = 'hello' for update;
                                          QUERY PLAN
-----------------------------------------------------------------------------------------------
 LockRows  (cost=109.02..121.46 rows=1 width=162)
   ->  Merge Join  (cost=109.02..121.45 rows=1 width=162)
         Merge Cond: (bar.a = foo.a)
         ->  Foreign Scan  (cost=100.58..4914.58 rows=40000 width=119)
               Relations: (public.bar) INNER JOIN (public.baz)
               ->  Hash Join  (cost=2556.00..4962.00 rows=40000 width=119)
                     Hash Cond: (bar.a = baz.a)
                     ->  Foreign Scan on bar  (cost=100.00..1956.00
rows=40000 width=28)
                     ->  Hash  (cost=1956.00..1956.00 rows=40000 width=27)
                           ->  Foreign Scan on baz
(cost=100.00..1956.00 rows=40000 width=27)
         ->  Sort  (cost=8.44..8.45 rows=1 width=28)
               Sort Key: foo.a
               ->  Index Scan using foo_b_idx on foo  (cost=0.41..8.43
rows=1 width=28)
                     Index Cond: (b = 'hello'::text)

There's really nothing to prevent the bar/baz join from producing
arbitrarily many rows, which means that it's not good to turn the
recheck plan into a Nested Loop unconditionally -- and come to think
of it, I'm pretty sure this kind of case is why
GetExistingLocalJoinPath() tries to using an existing path: it wants a
path that isn't going suck if it gets executed.

After some thought, it seems that there's a much simpler way that we
could fix the problem you identified in that original email: if the
EPQ path isn't properly sorted, have postgres_fdw's
add_paths_with_pathkeys_for_rel stick a Sort node on top of it.  I
tried this and it does indeed fix Jeff Janes' initial test case.
Patch attached.  One could do better here if one wanted to revise
GetExistingLocalJoinPath() to take Last *pathkeys as an additional
argument; we could then try to find the optimal EPQ path for each
possible set of sortkeys.  But I don't feel the need to work on that
right now, and it would be nicer not to back-patch a change to the
signature of GetExistingLocalJoinPath(), as has already been noted.

It is altogether possible that this doesn't fix every issue with the
GetExistingLocalJoinPath interface, and perhaps that is due for a
broader rewrite.  However, I think that more recent discussion has
cast some doubt on the fiery criticism of that previous note.  That
note asserted that we'd have trouble with join nests containing more
than 2 joins, but as was pointed out at the time, that's not really
true, because if we happen to pull a join out of the pathlist that has
ForeignScan children, we'll fetch out the EPQ-recheck subpaths that
they created and use those instead.  And there are other problems with
the analysis in that email too as noted above.  The only problem that
*has actually been demonstrated* is that we can end up using as an EPQ
path something that doesn't generate the right sort order, and that is
easily fixed by making it do so, which the attached patch does.  So I
propose we commit and back-patch this and move on.

---

From: Etsuro Fujita <[email-masked]>
Date: Mon, 15 Jan 2018 17:09:19 +0900

> # going to care about sort order, if we could identify the correct join

That's right.

> on foo.a = bar.a where foo.b = 'hello' for update;

Yeah, but I don't think the above example is good enough to explain 
that, because I think the bar/baz join would produce at most one tuple 
in an EPQ recheck since we would have only one EPQ tuple for both bar 
and baz in that recheck, and the join is inner.  I think such an example 
would probably be given e.g., by a modified version of the SQL where we 
have a full join of bar and baz, not an inner join.

> Patch attached.

The patch looks good to me.

> One could do better here if one wanted to revise

That makes sense to me.

> The only problem that

Agreed.

Best regards,
Etsuro Fujita

---

From: Robert Haas <[email-masked]>
Date: Mon, 15 Jan 2018 11:47:43 -0500

On Mon, Jan 15, 2018 at 3:09 AM, Etsuro Fujita
<[email-masked]> wrote:
> Yeah, but I don't think the above example is good enough to explain that,
> because I think the bar/baz join would produce at most one tuple in an EPQ
> recheck since we would have only one EPQ tuple for both bar and baz in that
> recheck, and the join is inner.  I think such an example would probably be
> given e.g., by a modified version of the SQL where we have a full join of
> bar and baz, not an inner join.

Hmm, I was thinking that bar and baz wouldn't be constrained to return
just one tuple in that case, but I'm wrong: there would just be one
tuple per relation in that case.  However, that would also be true for
a full join, wouldn't it?

Regardless of that, the patch fixes the reported problem with very
little code change, and somebody can always improve it further later.

---

From: Tom Lane <[email-masked]>
Date: Mon, 15 Jan 2018 12:31:17 -0500

> However, those remarks are almost entirely wrong.

Hm, one of us is confused.  Possibly it's me, but:

> It's true that the
> tuple,

Yup, and that is all of them.  We should have a tuple identity
(ctid or whole-row) for every base relation in an EPQ test.

> but because of what you said in the quoted text above, this

This example doesn't prove anything whatsoever about what happens during
EPQ, though, or at least it shouldn't.  If we're examining more than one
row then we're doing it wrong, because the point of the EPQ evaluation
is only to determine whether one single joined row (still) meets the
query conditions.

> tried this and it does indeed fix Jeff Janes' initial test case.

Hm.  Simple is certainly good, but if there's multiple rows coming
back during an EPQ recheck then I think we have a performance problem.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 15 Jan 2018 14:10:36 -0500

> back during an EPQ recheck then I think we have a performance problem.

You are correct ... I was wrong about that part, and said so in an
email on this thread sent about 45 minutes before yours.  However, I
still think the patch is a good fix for the immediate issue, unless
you see some problem with it.  It's simple and back-patchable and does
not preclude further work anybody, including you, might want to do in
the future.

---

From: Tom Lane <[email-masked]>
Date: Mon, 15 Jan 2018 16:38:04 -0500

> not preclude further work anybody, including you, might want to do in

I spent some more time poking at this.  I confirmed that there is *not*
a performance problem: as I'd thought, during an EPQ recheck each base
scan relation will return its single EPQ tuple, without any communication
with the remote server, whether or not the rel was marked FOR UPDATE.

There *is* a problem with GetExistingLocalJoinPath not honoring its
API spec: it will sometimes return join nests that include remote
joins at levels below the top, as I'd speculated to begin with.
This turns out not to be a problem for postgres_fdw, because any
such remote-join node will just recursively fob off EPQ checks
onto its own outerpath, so that eventually you get down to base
scan relations anyway, and everything works.  However, because
GetExistingLocalJoinPath is claimed to be a general purpose function
useful for other FDWs, the fact that its API contract is a lie
seems to me to be a problem anyway.  Another FDW might have a stronger
dependency on there not being remote sub-joins.  (Reviewing the thread
history, I see that Ashutosh agreed this was possible at
<CAFjFpRfYETZLiKERpxCeAN1MsiJypsyNd6x9FQ7OWjY=[email-masked]>
and suggested that we just change the function's commentary to not
make promises it won't keep.  Maybe that's enough, but I think just
walking away isn't enough.)

I'm also still pretty unhappy with the amount of useless planning work
caused by doing GetExistingLocalJoinPath during path creation.  It strikes
me that we could likely replace the entire thing with some code that just
reconstructs the join node's output tuple during EPQ using the rowmark
data for all the base relations.  Outer joins aren't really a problem:
we could tell which relations were replaced by nulls because the rowmark
values that bubbled up to the top went to nulls themselves.  However,
that's a nontrivial amount of work and probably wouldn't result in
something we cared to back-patch, especially since it's not really a bug
fix.

Anyway, I agree with you that we know of no observable bug here except
for Jeff's original complaint, and forcing sorting of the EPQ paths
would be enough to fix that.

Your patch seems OK codewise, but I think the comment is not nearly
adequate.  Maybe something like "The EPQ path must be at least as well
sorted as the path itself, in case it gets used as input to a mergejoin."

Also, a regression test case would be appropriate perhaps.  I tried
to reproduce Jeff's complaint in the postgres_fdw regression database,
and it actually failed on the very first multi-way join I tried:

contrib_regression=# explain select * from ft1,ft2,ft4,ft5 where ft1.c1=ft2.c1 and ft1.c1=ft4.c1 and ft1.c1=ft5.c1 for update;
ERROR:  outer pathkeys do not match mergeclauses

Also, for the archives' sake, here's an example showing that
GetExistingLocalJoinPath is not doing what it says on the tin:

contrib_regression=# set from_collapse_limit TO 1;
SET
contrib_regression=# set enable_hashjoin TO 0;
SET
contrib_regression=# set enable_mergejoin TO 0;
SET
contrib_regression=# explain select ft1.c1,ft4.* from ft1,ft2 left join ft4 on ft2.c1=ft4.c1,ft5,loc1 where ft1.c1=ft2.c1 and ft1.c1=ft5.c1 and ft1.c1=loc1.f1 for update of ft1,loc1;
                                                       QUERY PLAN                                                        
-------------------------------------------------------------------------------------------------------------------------
 LockRows  (cost=105.72..781.42 rows=210 width=286)
   ->  Nested Loop  (cost=105.72..779.32 rows=210 width=286)
         Join Filter: (ft2.c1 = loc1.f1)
         ->  Seq Scan on loc1  (cost=0.00..22.70 rows=1270 width=10)
         ->  Materialize  (cost=105.72..128.06 rows=33 width=179)
               ->  Foreign Scan  (cost=105.72..127.89 rows=33 width=179)
                     Relations: ((public.ft1) INNER JOIN ((public.ft2) LEFT JOIN (public.ft4))) INNER JOIN (public.ft5)
                     ->  Nested Loop  (cost=233.62..721.53 rows=136 width=179)
                           Join Filter: (ft2.c1 = ft5.c1)
                           ->  Nested Loop  (cost=202.12..12631.43 rows=822 width=137)
                                 Join Filter: (ft1.c1 = ft2.c1)
                                 ->  Foreign Scan on ft1  (cost=100.00..141.00 rows=1000 width=75)
                                 ->  Materialize  (cost=102.12..162.48 rows=822 width=83)
                                       ->  Foreign Scan  (cost=102.12..158.37 rows=822 width=83)
                                             Relations: (public.ft2) LEFT JOIN (public.ft4)
                                             ->  Nested Loop Left Join  (cost=200.00..856.78 rows=822 width=83)
                                                   Join Filter: (ft2.c1 = ft4.c1)
                                                   ->  Foreign Scan on ft2  (cost=100.00..137.66 rows=822 width=54)
                                                   ->  Materialize  (cost=100.00..102.75 rows=50 width=54)
                                                         ->  Foreign Scan on ft4  (cost=100.00..102.50 rows=50 width=54)
                           ->  Materialize  (cost=100.00..102.16 rows=33 width=43)
                                 ->  Foreign Scan on ft5  (cost=100.00..101.99 rows=33 width=43)
(22 rows)

I don't think this latter example needs to become a regression test
in this form.  If we wanted to gear up an isolation test that
actually exercised EPQ with postgres_fdw, maybe it'd be useful
in that context, to prove that nested remote joins work for EPQ.

			regards, tom lane

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 16 Jan 2018 10:57:49 +0900

> a full join, wouldn't it?

Consider:

postgres=# create table bar (a int, b text);
postgres=# create table baz (a int, b text);
postgres=# insert into bar values (1, 'bar');
postgres=# insert into baz values (2, 'baz');
postgres=# select * from bar full join baz on bar.a = baz.a;
  a |  b  | a |  b
---+-----+---+-----
  1 | bar |   |
    |     | 2 | baz
(2 rows)

Both relations have one tuple, but the full join produces two join 
tuples.  I think it would be possible that something like this happens 
when executing a local join plan for a foreign join that performs a full 
join remotely.

Agreed.

Best regards,
Etsuro Fujita

---

From: Tom Lane <[email-masked]>
Date: Mon, 15 Jan 2018 21:17:39 -0500

> when executing a local join plan for a foreign join that performs a full 

Doesn't really matter though, does it?  Each of those join rows will
be processed as a separate EPQ event.

			regards, tom lane

---

From: Etsuro Fujita <[email-masked]>
Date: Tue, 16 Jan 2018 12:00:53 +0900

(2018/01/16 11:17), Tom Lane wrote:
> Etsuro Fujita<[email-masked]>  writes:

I assume that such a local join plan is executed as part of a FOR UPDATE 
query like the one shown by Robert (the bar/baz foreign join part in 
that query), so I am thinking that those join rows will be processed as 
a single event.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 17 Jan 2018 18:02:49 +0900

> that query), so I am thinking that those join rows will be processed as

I realized I am wrong; the local join execution plan would never produce 
multiple tuples in a single event.

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 17 Jan 2018 19:32:35 +0900

(2018/01/16 6:38), Tom Lane wrote:
> Robert Haas<[email-masked]>  writes:
>> On Mon, Jan 15, 2018 at 12:31 PM, Tom Lane<[email-masked]>  wrote:
>> email on this thread sent about 45 minutes before yours.

I was wrong too.

>> However, I

I still think so too.

> data for all the base relations.

The join tuple would be reconstructed without a local join execution plan?

> Outer joins aren't really a problem:
> values that bubbled up to the top went to nulls themselves.

Yeah, but we would need null-extension or projection...

> However,

What do you think about a future extension to parameterized foreign paths?

Best regards,
Etsuro Fujita

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2018 13:46:02 -0500

> The join tuple would be reconstructed without a local join execution plan?

It seems clear to me that this could be done in principle.

> Yeah, but we would need null-extension or projection...

Yes, the question is how complicated it would be to get the projection
right.  As long as the join node's scan tuple only needs to contain values
(possibly whole-row Vars) propagated up from base relations and possibly
nulled out, it seems like it's not very hard.  But if we ever try to
implement actual calculation of expressions in intermediate join nodes,
it could get to be a mess.  (I haven't looked at the aggregate-pushdown
patch in this connection, although probably that's not a hazard since
we don't support aggregation together with FOR UPDATE.  Perhaps we could
likewise simply refuse to push expressions into foreign join nests if
FOR UPDATE is active.)

As against that, the existing EPQ implementation is pretty messy already,
on top of consuming a lot of planning cycles.

> What do you think about a future extension to parameterized foreign paths?

Don't have an opinion on that; but it doesn't seem like EPQ is the
principal blocking factor there.  You could always skip generating
parameterized foreign paths if FOR UPDATE is needed.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Wed, 17 Jan 2018 15:02:10 -0500

On Mon, Jan 15, 2018 at 4:38 PM, Tom Lane <[email-masked]> wrote:
> seems to me to be a problem anyway.

Yeah, that's not good.  The idea, as the comments inside
GetExistingLocalJoinPath say, is that if we find an existing join
whose inner or outer side is a ForeignPath for a join, we fish out the
outer path which should be that subpath's EPQ path.  I'm not 100% sure
why that's not happening in your example, but my first guess is that
it's because the inner path has a Materialize node on top of the
join-ForeignPath, and GetExistingLocalJoinPath just sees the
Materialize node and concludes that it doesn't need to look further.
If that's correct, bet we could fix that by telling it to drill
through any MaterialPath it sees and use the sub-path instead; given
that there is only one tuple per relation, the Materialize step can't
be important for performance.

> something we cared to back-patch, especially since it's not really a bug

Yes, I believe something like this would be possible.  I did consider
that approach, but this seemed easier to implement and, as it was, it
took two release cycles to get join pushdown committed.  And multiple
hundreds of emails, most of them about EPQ, if I remember correctly.
I'm happy to have somebody implement something better, but I will
probably get a bit hypoxic if I hold my breath waiting for it to
happen.

> ERROR:  outer pathkeys do not match mergeclauses

Thanks for the review.  Updated patch attached.

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2018 15:37:34 -0500

> Thanks for the review.  Updated patch attached.

Looks OK to me.  Would it be worth annotating the added regression test
case with a comment that this once caused EPQ-related planning problems?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Wed, 17 Jan 2018 16:06:48 -0500

> case with a comment that this once caused EPQ-related planning problems?

I tend to think somebody who is curious about the origin of any
particular test can just use 'git blame' and/or 'git log -Gwhatever'
to figure out which commits added it, and that therefore it's not
worth including that in the comment explicitly.  But I don't care
deeply.

---

From: Tom Lane <[email-masked]>
Date: Wed, 17 Jan 2018 16:08:40 -0500

> worth including that in the comment explicitly.  But I don't care

It's debatable perhaps -- I tend to err in the other direction.
But likewise, I don't care deeply.  Just push it ...

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Wed, 17 Jan 2018 17:09:58 -0500

> But likewise, I don't care deeply.  Just push it ...

Done.

---

From: Etsuro Fujita <[email-masked]>
Date: Thu, 18 Jan 2018 15:40:04 +0900

(2018/01/18 7:09), Robert Haas wrote:
> On Wed, Jan 17, 2018 at 4:08 PM, Tom Lane<[email-masked]>  wrote:

Thanks for taking the time to work on this issue, Robert and Tom!

Best regards,
Etsuro Fujita

---

From: Etsuro Fujita <[email-masked]>
Date: Fri, 19 Jan 2018 15:53:12 +0900

>>> But likewise, I don't care deeply. Just push it ...

I noticed that this test case added by the patch is not appropriate:

+-- multi-way join involving multiple merge joins
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
+    AND ft1.c1 = ft5.c1 FOR UPDATE;
+SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
+    AND ft1.c1 = ft5.c1 FOR UPDATE;

because it doesn't inject extra Sort nodes into EPQ recheck plans, so it 
works well without the fix.  I modified this to inject a Sort into the 
recheck plan of the very first foreign join.  Attached is a patch for that.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 2330,2367 **** SELECT ft5, ft5.c1, ft5.c2, ft5.c3, ft4.c1, ft4.c2 FROM ft5 left join ft4 on ft5
  (4 rows)
  
  -- multi-way join involving multiple merge joins
  EXPLAIN (VERBOSE, COSTS OFF)
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
!     AND ft1.c1 = ft5.c1 FOR UPDATE;
!                                                                                                                                                                                                                                                                                                                                                                                                          QUERY PLAN                                                                                                                                                                                                                                                                                                                                                                                                         
! ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   LockRows
     Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3, ft1.*, ft2.*, ft4.*, ft5.*
     ->  Foreign Scan
           Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3, ft1.*, ft2.*, ft4.*, ft5.*
           Relations: (((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5)
!          Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8, 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, r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8, 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, r3.c1, r3.c2, r3.c3, CASE WHEN (r3.*)::text IS NOT NULL THEN ROW(r3.c1, r3.c2, r3.c3) END, r4.c1, r4.c2, r4.c3, CASE WHEN (r4.*)::text IS NOT NULL THEN ROW(r4.c1, r4.c2, r4.c3) END FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")))) INNER JOIN "S 1"."T 3" r3 ON (((r1."C 1" = r3.c1)))) INNER JOIN "S 1"."T 4" r4 ON (((r1."C 1" = r4.c1)))) FOR UPDATE OF r1 FOR UPDATE OF r2 FOR UPDATE OF r3 FOR UPDATE OF r4
           ->  Merge Join
                 Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*, ft4.c1, ft4.c2, ft4.c3, ft4.*, ft5.c1, ft5.c2, ft5.c3, ft5.*
!                Merge Cond: (ft1.c1 = ft5.c1)
                 ->  Merge Join
                       Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*, ft4.c1, ft4.c2, ft4.c3, ft4.*
!                      Merge Cond: (ft1.c1 = ft4.c1)
!                      ->  Merge Join
                             Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                            Merge Cond: (ft1.c1 = ft2.c1)
!                            ->  Sort
!                                  Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*
!                                  Sort Key: ft1.c1
!                                  ->  Foreign Scan on public.ft1
                                         Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*
!                                        Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
!                            ->  Sort
!                                  Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                                  Sort Key: ft2.c1
!                                  ->  Foreign Scan on public.ft2
                                         Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                                        Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
                       ->  Sort
                             Output: ft4.c1, ft4.c2, ft4.c3, ft4.*
                             Sort Key: ft4.c1
--- 2330,2371 ----
  (4 rows)
  
  -- multi-way join involving multiple merge joins
+ SET enable_nestloop TO false;
+ SET enable_hashjoin TO false;
  EXPLAIN (VERBOSE, COSTS OFF)
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c2 = ft4.c1
!     AND ft1.c2 = ft5.c1 AND ft1.c1 < 100 AND ft2.c1 < 100 FOR UPDATE;
!                                                                                                                                                                                                                                                                                                                                                                                                                              QUERY PLAN                                                                                                                                                                                                                                                                                                                                                                                                                             
! --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   LockRows
     Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3, ft1.*, ft2.*, ft4.*, ft5.*
     ->  Foreign Scan
           Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft4.c1, ft4.c2, ft4.c3, ft5.c1, ft5.c2, ft5.c3, ft1.*, ft2.*, ft4.*, ft5.*
           Relations: (((public.ft1) INNER JOIN (public.ft2)) INNER JOIN (public.ft4)) INNER JOIN (public.ft5)
!          Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8, 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, r2."C 1", r2.c2, r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8, 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, r3.c1, r3.c2, r3.c3, CASE WHEN (r3.*)::text IS NOT NULL THEN ROW(r3.c1, r3.c2, r3.c3) END, r4.c1, r4.c2, r4.c3, CASE WHEN (r4.*)::text IS NOT NULL THEN ROW(r4.c1, r4.c2, r4.c3) END FROM ((("S 1"."T 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1")) AND ((r2."C 1" < 100)) AND ((r1."C 1" < 100)))) INNER JOIN "S 1"."T 3" r3 ON (((r1.c2 = r3.c1)))) INNER JOIN "S 1"."T 4" r4 ON (((r1.c2 = r4.c1)))) FOR UPDATE OF r1 FOR UPDATE OF r2 FOR UPDATE OF r3 FOR UPDATE OF r4
           ->  Merge Join
                 Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*, ft4.c1, ft4.c2, ft4.c3, ft4.*, ft5.c1, ft5.c2, ft5.c3, ft5.*
!                Merge Cond: (ft1.c2 = ft5.c1)
                 ->  Merge Join
                       Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*, ft4.c1, ft4.c2, ft4.c3, ft4.*
!                      Merge Cond: (ft1.c2 = ft4.c1)
!                      ->  Sort
                             Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                            Sort Key: ft1.c2
!                            ->  Merge Join
!                                  Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*, ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                                  Merge Cond: (ft1.c1 = ft2.c1)
!                                  ->  Sort
                                         Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*
!                                        Sort Key: ft1.c1
!                                        ->  Foreign Scan on public.ft1
!                                              Output: ft1.c1, ft1.c2, ft1.c3, ft1.c4, ft1.c5, ft1.c6, ft1.c7, ft1.c8, ft1.*
!                                              Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" < 100)) FOR UPDATE
!                                  ->  Materialize
                                         Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                                        ->  Foreign Scan on public.ft2
!                                              Output: ft2.c1, ft2.c2, ft2.c3, ft2.c4, ft2.c5, ft2.c6, ft2.c7, ft2.c8, ft2.*
!                                              Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" < 100)) ORDER BY "C 1" ASC NULLS LAST FOR UPDATE
                       ->  Sort
                             Output: ft4.c1, ft4.c2, ft4.c3, ft4.*
                             Sort Key: ft4.c1
***************
*** 2374,2403 **** SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
                       ->  Foreign Scan on public.ft5
                             Output: ft5.c1, ft5.c2, ft5.c3, ft5.*
                             Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" FOR UPDATE
! (39 rows)
  
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
!     AND ft1.c1 = ft5.c1 FOR UPDATE;
   c1 | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  | c1 | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  | c1 | c2 |   c3   | c1 | c2 |   c3   
  ----+----+-------+------------------------------+--------------------------+----+------------+-----+----+----+-------+------------------------------+--------------------------+----+------------+-----+----+----+--------+----+----+--------
    6 |  6 | 00006 | Wed Jan 07 00:00:00 1970 PST | Wed Jan 07 00:00:00 1970 | 6  | 6          | foo |  6 |  6 | 00006 | Wed Jan 07 00:00:00 1970 PST | Wed Jan 07 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  12 |  2 | 00012 | Tue Jan 13 00:00:00 1970 PST | Tue Jan 13 00:00:00 1970 | 2  | 2          | foo | 12 |  2 | 00012 | Tue Jan 13 00:00:00 1970 PST | Tue Jan 13 00:00:00 1970 | 2  | 2          | foo | 12 | 13 | AAA012 | 12 | 13 | AAA012
!  18 |  8 | 00018 | Mon Jan 19 00:00:00 1970 PST | Mon Jan 19 00:00:00 1970 | 8  | 8          | foo | 18 |  8 | 00018 | Mon Jan 19 00:00:00 1970 PST | Mon Jan 19 00:00:00 1970 | 8  | 8          | foo | 18 | 19 | AAA018 | 18 | 19 | 
!  24 |  4 | 00024 | Sun Jan 25 00:00:00 1970 PST | Sun Jan 25 00:00:00 1970 | 4  | 4          | foo | 24 |  4 | 00024 | Sun Jan 25 00:00:00 1970 PST | Sun Jan 25 00:00:00 1970 | 4  | 4          | foo | 24 | 25 | AAA024 | 24 | 25 | AAA024
!  30 |  0 | 00030 | Sat Jan 31 00:00:00 1970 PST | Sat Jan 31 00:00:00 1970 | 0  | 0          | foo | 30 |  0 | 00030 | Sat Jan 31 00:00:00 1970 PST | Sat Jan 31 00:00:00 1970 | 0  | 0          | foo | 30 | 31 | AAA030 | 30 | 31 | AAA030
!  36 |  6 | 00036 | Fri Feb 06 00:00:00 1970 PST | Fri Feb 06 00:00:00 1970 | 6  | 6          | foo | 36 |  6 | 00036 | Fri Feb 06 00:00:00 1970 PST | Fri Feb 06 00:00:00 1970 | 6  | 6          | foo | 36 | 37 | AAA036 | 36 | 37 | 
!  42 |  2 | 00042 | Thu Feb 12 00:00:00 1970 PST | Thu Feb 12 00:00:00 1970 | 2  | 2          | foo | 42 |  2 | 00042 | Thu Feb 12 00:00:00 1970 PST | Thu Feb 12 00:00:00 1970 | 2  | 2          | foo | 42 | 43 | AAA042 | 42 | 43 | AAA042
!  48 |  8 | 00048 | Wed Feb 18 00:00:00 1970 PST | Wed Feb 18 00:00:00 1970 | 8  | 8          | foo | 48 |  8 | 00048 | Wed Feb 18 00:00:00 1970 PST | Wed Feb 18 00:00:00 1970 | 8  | 8          | foo | 48 | 49 | AAA048 | 48 | 49 | AAA048
!  54 |  4 | 00054 | Tue Feb 24 00:00:00 1970 PST | Tue Feb 24 00:00:00 1970 | 4  | 4          | foo | 54 |  4 | 00054 | Tue Feb 24 00:00:00 1970 PST | Tue Feb 24 00:00:00 1970 | 4  | 4          | foo | 54 | 55 | AAA054 | 54 | 55 | 
!  60 |  0 | 00060 | Mon Mar 02 00:00:00 1970 PST | Mon Mar 02 00:00:00 1970 | 0  | 0          | foo | 60 |  0 | 00060 | Mon Mar 02 00:00:00 1970 PST | Mon Mar 02 00:00:00 1970 | 0  | 0          | foo | 60 | 61 | AAA060 | 60 | 61 | AAA060
!  66 |  6 | 00066 | Sun Mar 08 00:00:00 1970 PST | Sun Mar 08 00:00:00 1970 | 6  | 6          | foo | 66 |  6 | 00066 | Sun Mar 08 00:00:00 1970 PST | Sun Mar 08 00:00:00 1970 | 6  | 6          | foo | 66 | 67 | AAA066 | 66 | 67 | AAA066
!  72 |  2 | 00072 | Sat Mar 14 00:00:00 1970 PST | Sat Mar 14 00:00:00 1970 | 2  | 2          | foo | 72 |  2 | 00072 | Sat Mar 14 00:00:00 1970 PST | Sat Mar 14 00:00:00 1970 | 2  | 2          | foo | 72 | 73 | AAA072 | 72 | 73 | 
!  78 |  8 | 00078 | Fri Mar 20 00:00:00 1970 PST | Fri Mar 20 00:00:00 1970 | 8  | 8          | foo | 78 |  8 | 00078 | Fri Mar 20 00:00:00 1970 PST | Fri Mar 20 00:00:00 1970 | 8  | 8          | foo | 78 | 79 | AAA078 | 78 | 79 | AAA078
!  84 |  4 | 00084 | Thu Mar 26 00:00:00 1970 PST | Thu Mar 26 00:00:00 1970 | 4  | 4          | foo | 84 |  4 | 00084 | Thu Mar 26 00:00:00 1970 PST | Thu Mar 26 00:00:00 1970 | 4  | 4          | foo | 84 | 85 | AAA084 | 84 | 85 | AAA084
!  90 |  0 | 00090 | Wed Apr 01 00:00:00 1970 PST | Wed Apr 01 00:00:00 1970 | 0  | 0          | foo | 90 |  0 | 00090 | Wed Apr 01 00:00:00 1970 PST | Wed Apr 01 00:00:00 1970 | 0  | 0          | foo | 90 | 91 | AAA090 | 90 | 91 | 
!  96 |  6 | 00096 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6  | 6          | foo | 96 |  6 | 00096 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6  | 6          | foo | 96 | 97 | AAA096 | 96 | 97 | AAA096
! (16 rows)
  
  -- check join pushdown in situations where multiple userids are involved
  CREATE ROLE regress_view_owner SUPERUSER;
  CREATE USER MAPPING FOR regress_view_owner SERVER loopback;
--- 2378,2403 ----
                       ->  Foreign Scan on public.ft5
                             Output: ft5.c1, ft5.c2, ft5.c3, ft5.*
                             Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 4" FOR UPDATE
! (41 rows)
  
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c2 = ft4.c1
!     AND ft1.c2 = ft5.c1 AND ft1.c1 < 100 AND ft2.c1 < 100 FOR UPDATE;
   c1 | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  | c1 | c2 |  c3   |              c4              |            c5            | c6 |     c7     | c8  | c1 | c2 |   c3   | c1 | c2 |   c3   
  ----+----+-------+------------------------------+--------------------------+----+------------+-----+----+----+-------+------------------------------+--------------------------+----+------------+-----+----+----+--------+----+----+--------
    6 |  6 | 00006 | Wed Jan 07 00:00:00 1970 PST | Wed Jan 07 00:00:00 1970 | 6  | 6          | foo |  6 |  6 | 00006 | Wed Jan 07 00:00:00 1970 PST | Wed Jan 07 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  16 |  6 | 00016 | Sat Jan 17 00:00:00 1970 PST | Sat Jan 17 00:00:00 1970 | 6  | 6          | foo | 16 |  6 | 00016 | Sat Jan 17 00:00:00 1970 PST | Sat Jan 17 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  26 |  6 | 00026 | Tue Jan 27 00:00:00 1970 PST | Tue Jan 27 00:00:00 1970 | 6  | 6          | foo | 26 |  6 | 00026 | Tue Jan 27 00:00:00 1970 PST | Tue Jan 27 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  36 |  6 | 00036 | Fri Feb 06 00:00:00 1970 PST | Fri Feb 06 00:00:00 1970 | 6  | 6          | foo | 36 |  6 | 00036 | Fri Feb 06 00:00:00 1970 PST | Fri Feb 06 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  46 |  6 | 00046 | Mon Feb 16 00:00:00 1970 PST | Mon Feb 16 00:00:00 1970 | 6  | 6          | foo | 46 |  6 | 00046 | Mon Feb 16 00:00:00 1970 PST | Mon Feb 16 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  56 |  6 | 00056 | Thu Feb 26 00:00:00 1970 PST | Thu Feb 26 00:00:00 1970 | 6  | 6          | foo | 56 |  6 | 00056 | Thu Feb 26 00:00:00 1970 PST | Thu Feb 26 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  66 |  6 | 00066 | Sun Mar 08 00:00:00 1970 PST | Sun Mar 08 00:00:00 1970 | 6  | 6          | foo | 66 |  6 | 00066 | Sun Mar 08 00:00:00 1970 PST | Sun Mar 08 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  76 |  6 | 00076 | Wed Mar 18 00:00:00 1970 PST | Wed Mar 18 00:00:00 1970 | 6  | 6          | foo | 76 |  6 | 00076 | Wed Mar 18 00:00:00 1970 PST | Wed Mar 18 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  86 |  6 | 00086 | Sat Mar 28 00:00:00 1970 PST | Sat Mar 28 00:00:00 1970 | 6  | 6          | foo | 86 |  6 | 00086 | Sat Mar 28 00:00:00 1970 PST | Sat Mar 28 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
!  96 |  6 | 00096 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6  | 6          | foo | 96 |  6 | 00096 | Tue Apr 07 00:00:00 1970 PST | Tue Apr 07 00:00:00 1970 | 6  | 6          | foo |  6 |  7 | AAA006 |  6 |  7 | AAA006
! (10 rows)
  
+ RESET enable_nestloop;
+ RESET enable_hashjoin;
  -- check join pushdown in situations where multiple userids are involved
  CREATE ROLE regress_view_owner SUPERUSER;
  CREATE USER MAPPING FOR regress_view_owner SERVER loopback;
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 560,570 **** SELECT ft5, ft5.c1, ft5.c2, ft5.c3, ft4.c1, ft4.c2 FROM ft5 left join ft4 on ft5
  SELECT ft5, ft5.c1, ft5.c2, ft5.c3, ft4.c1, ft4.c2 FROM ft5 left join ft4 on ft5.c1 = ft4.c1 WHERE ft4.c1 BETWEEN 10 and 30 ORDER BY ft5.c1, ft4.c1;
  
  -- multi-way join involving multiple merge joins
  EXPLAIN (VERBOSE, COSTS OFF)
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
!     AND ft1.c1 = ft5.c1 FOR UPDATE;
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
!     AND ft1.c1 = ft5.c1 FOR UPDATE;
  
  -- check join pushdown in situations where multiple userids are involved
  CREATE ROLE regress_view_owner SUPERUSER;
--- 560,574 ----
  SELECT ft5, ft5.c1, ft5.c2, ft5.c3, ft4.c1, ft4.c2 FROM ft5 left join ft4 on ft5.c1 = ft4.c1 WHERE ft4.c1 BETWEEN 10 and 30 ORDER BY ft5.c1, ft4.c1;
  
  -- multi-way join involving multiple merge joins
+ SET enable_nestloop TO false;
+ SET enable_hashjoin TO false;
  EXPLAIN (VERBOSE, COSTS OFF)
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c2 = ft4.c1
!     AND ft1.c2 = ft5.c1 AND ft1.c1 < 100 AND ft2.c1 < 100 FOR UPDATE;
! SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c2 = ft4.c1
!     AND ft1.c2 = ft5.c1 AND ft1.c1 < 100 AND ft2.c1 < 100 FOR UPDATE;
! RESET enable_nestloop;
! RESET enable_hashjoin;
  
  -- check join pushdown in situations where multiple userids are involved
  CREATE ROLE regress_view_owner SUPERUSER;

---

From: Robert Haas <[email-masked]>
Date: Fri, 19 Jan 2018 10:17:53 -0500

> recheck plan of the very first foreign join.  Attached is a patch for that.

Mumble.  Tom provided me with that example and said it failed without
the patch.  I didn't check, I just believed him.  But I'm surprised if
he was wrong; Tom usually tries to avoid being wrong...

---

From: Tom Lane <[email-masked]>
Date: Fri, 19 Jan 2018 10:40:36 -0500

> he was wrong; Tom usually tries to avoid being wrong...

Hm.  It did fail as advertised when I connected to the contrib_regression
database (after installcheck) and entered the query by hand; I
copied-and-pasted the result of that to show you.  It's possible that it
would not have failed in the particular spot where you chose to insert it
in the regression script, if for example there were nondefault planner GUC
settings active at that spot.  Did you check that the script produced the
expected failure against unpatched code?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 19 Jan 2018 10:50:13 -0500

> expected failure against unpatched code?

No.  I guess I'll have to go debug this.  Argh.

---

From: Robert Haas <[email-masked]>
Date: Fri, 19 Jan 2018 20:11:44 -0500

> No.  I guess I'll have to go debug this.  Argh.

Well, I'm a little stumped.  When I do it the way you did it, it fails
with the same error you got:

contrib_regression=# EXPLAIN (VERBOSE, COSTS OFF)
SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
    AND ft1.c1 = ft5.c1 FOR UPDATE;
ERROR:  outer pathkeys do not match mergeclauses

But when I add the same command to postgres_fdw.sql and run it as part
of the regression tests, it works.  I tried adding it at the end with
\c beforehand so that there wouldn't be any temporary settings in
effect.  It still didn't generate an error.  So then I ran 'SHOW ALL'
from the regression test file and from my psql terminal and comparing
the results:

<   application_name                    | pg_regress/postgres_fdw
       | Sets the application name to be reported in statistics and
logs.
>  application_name                    | psql                                | Sets the application name to be reported in statistics and logs.
<   DateStyle                           | Postgres, MDY
       | Sets the display format for date and time values.
>  DateStyle                           | ISO, MDY                            | Sets the display format for date and time values.
<   IntervalStyle                       | postgres_verbose
       | Sets the display format for interval values.
>  IntervalStyle                       | postgres                            | Sets the display format for interval values.
<   TimeZone                            | PST8PDT
       | Sets the time zone for displaying and interpreting time
stamps.
>  TimeZone                            | US/Eastern                          | Sets the time zone for displaying and interpreting time stamps.

I then tried setting all of those GUCs to have the same value in my
psql session that they had in the script, but I still got the same
error.  So, with the exact same settings in effect, this query
reliable produces the error from psql and reliably fails to produce
the error from pg_regress.  Furthermore, it remains true even when the
query is sent from pg_regress as the first query in a brand new
session, so the problem can't be some kind of session state left
behind by the previous postgres_fdw tests.  In fact, if I extract the
exact psql command that pg_regress runs and execute it by hand, then
the query errors out:

"/Users/rhaas/install/dev/bin/psql" -X -a -q -d "contrib_regression" <
"/Users/rhaas/pgsql/contrib/postgres_fdw/sql/postgres_fdw.sql"

But the exact same command run by pg_regress does not error out.
Maybe the environment variables that pg_regress sets up affect
something?  But what would they affect on the server, other than the
values of GUCs?  I'm probably missing something dumb here, but I can't
think of what it is at the moment.

---

From: Tom Lane <[email-masked]>
Date: Sat, 20 Jan 2018 00:20:44 -0500

> effect.

I duplicated those results, and then added an ANALYZE, and then it fails:

***************
*** 7548,7550 ****
--- 7595,7604 ----
  (4 rows)
  
  RESET enable_partition_wise_join;
+ \c -
+ analyze "S 1"."T 1";
+ -- multi-way join involving multiple merge joins
+ EXPLAIN (VERBOSE, COSTS OFF)
+ SELECT * FROM ft1, ft2, ft4, ft5 WHERE ft1.c1 = ft2.c1 AND ft1.c1 = ft4.c1
+     AND ft1.c1 = ft5.c1 FOR UPDATE;
+ ERROR:  outer pathkeys do not match mergeclauses

So apparently the reason our manual tests replicated the failure is
that an auto-analyze happened in between.  Now that I realize that,
I find that the manual query doesn't fail if executed *immediately*
after the regression test run.  Wait a minute, repeat, it does fail.

OTOH, put the same ANALYZE before the test case where it is in the
script, no change.

So the contributing factors here are (1) there are a bunch of data
changes to "S 1"."T 1" further down in the script than where you
added the test case, and (2) you need an auto-analyze to have seen
those changes before the problematic plan gets picked.

It looks like Etsuro-san's proposed patch locks down the choice of
plan more tightly, which is probably a reasonable answer.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Tue, 30 Jan 2018 14:56:06 -0500

> plan more tightly, which is probably a reasonable answer.

OK, committed.  I added a comment along the lines you suggested
previously, since this no longer seems like a generic test that
happens to involve a bunch of merge joins.

---

From: Etsuro Fujita <[email-masked]>
Date: Wed, 31 Jan 2018 11:17:01 +0900

(2018/01/31 4:56), Robert Haas wrote:
> On Sat, Jan 20, 2018 at 12:20 AM, Tom Lane<[email-masked]>  wrote:

Thank you!

Best regards,
Etsuro Fujita