Code review for foreign/custom join pushdown patch.

Commit e7cb7ee14555cc9c5773e2c102efd6371f6f2005 included some design
decisions that seem pretty questionable to me, and there was quite a lot
of stuff not to like about the documentation and comments.  Clean up
as follows:

* Consider foreign joins only between foreign tables on the same server,
rather than between any two foreign tables with the same underlying FDW
handler function.  In most if not all cases, the FDW would simply have had
to apply the same-server restriction itself (far more expensively, both for
lack of caching and because it would be repeated for each combination of
input sub-joins), or else risk nasty bugs.  Anyone who's really intent on
doing something outside this restriction can always use the
set_join_pathlist_hook.

* Rename fdw_ps_tlist/custom_ps_tlist to fdw_scan_tlist/custom_scan_tlist
to better reflect what they're for, and allow these custom scan tlists
to be used even for base relations.

* Change make_foreignscan() API to include passing the fdw_scan_tlist
value, since the FDW is required to set that.  Backwards compatibility
doesn't seem like an adequate reason to expect FDWs to set it in some
ad-hoc extra step, and anyway existing FDWs can just pass NIL.

* Change the API of path-generating subroutines of add_paths_to_joinrel,
and in particular that of GetForeignJoinPaths and set_join_pathlist_hook,
so that various less-used parameters are passed in a struct rather than
as separate parameter-list entries.  The objective here is to reduce the
probability that future additions to those parameter lists will result in
source-level API breaks for users of these hooks.  It's possible that this
is even a small win for the core code, since most CPU architectures can't
pass more than half a dozen parameters efficiently anyway.  I kept root,
joinrel, outerrel, innerrel, and jointype as separate parameters to reduce
code churn in joinpath.c --- in particular, putting jointype into the
struct would have been problematic because of the subroutines' habit of
changing their local copies of that variable.

* Avoid ad-hocery in ExecAssignScanProjectionInfo.  It was probably all
right for it to know about IndexOnlyScan, but if the list is to grow
we should refactor the knowledge out to the callers.

* Restore nodeForeignscan.c's previous use of the relcache to avoid
extra GetFdwRoutine lookups for base-relation scans.

* Lots of cleanup of documentation and missed comments.  Re-order some
code additions into more logical places.

1 files changed, 30 insertions, 12 deletions

diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c
index cdbd550fd43..763ee7c9bce 100644
--- a/src/backend/foreign/foreign.c
+++ b/src/backend/foreign/foreign.c
@@ -304,21 +304,16 @@ GetFdwRoutine(Oid fdwhandler)
 
 
 /*
- * GetFdwHandlerByRelId - look up the handler of the foreign-data wrapper
- * for the given foreign table
+ * GetForeignServerIdByRelId - look up the foreign server
+ * for the given foreign table, and return its OID.
  */
 Oid
-GetFdwHandlerByRelId(Oid relid)
+GetForeignServerIdByRelId(Oid relid)
 {
 	HeapTuple	tp;
-	Form_pg_foreign_data_wrapper fdwform;
-	Form_pg_foreign_server serverform;
 	Form_pg_foreign_table tableform;
 	Oid			serverid;
-	Oid			fdwid;
-	Oid			fdwhandler;
 
-	/* Get server OID for the foreign table. */
 	tp = SearchSysCache1(FOREIGNTABLEREL, ObjectIdGetDatum(relid));
 	if (!HeapTupleIsValid(tp))
 		elog(ERROR, "cache lookup failed for foreign table %u", relid);
@@ -326,6 +321,23 @@ GetFdwHandlerByRelId(Oid relid)
 	serverid = tableform->ftserver;
 	ReleaseSysCache(tp);
 
+	return serverid;
+}
+
+
+/*
+ * GetFdwRoutineByServerId - look up the handler of the foreign-data wrapper
+ * for the given foreign server, and retrieve its FdwRoutine struct.
+ */
+FdwRoutine *
+GetFdwRoutineByServerId(Oid serverid)
+{
+	HeapTuple	tp;
+	Form_pg_foreign_data_wrapper fdwform;
+	Form_pg_foreign_server serverform;
+	Oid			fdwid;
+	Oid			fdwhandler;
+
 	/* Get foreign-data wrapper OID for the server. */
 	tp = SearchSysCache1(FOREIGNSERVEROID, ObjectIdGetDatum(serverid));
 	if (!HeapTupleIsValid(tp))
@@ -350,9 +362,11 @@ GetFdwHandlerByRelId(Oid relid)
 
 	ReleaseSysCache(tp);
 
-	return fdwhandler;
+	/* And finally, call the handler function. */
+	return GetFdwRoutine(fdwhandler);
 }
 
+
 /*
  * GetFdwRoutineByRelId - look up the handler of the foreign-data wrapper
  * for the given foreign table, and retrieve its FdwRoutine struct.
@@ -360,9 +374,13 @@ GetFdwHandlerByRelId(Oid relid)
 FdwRoutine *
 GetFdwRoutineByRelId(Oid relid)
 {
-	Oid			fdwhandler = GetFdwHandlerByRelId(relid);
+	Oid			serverid;
 
-	return GetFdwRoutine(fdwhandler);
+	/* Get server OID for the foreign table. */
+	serverid = GetForeignServerIdByRelId(relid);
+
+	/* Now retrieve server's FdwRoutine struct. */
+	return GetFdwRoutineByServerId(serverid);
 }
 
 /*
@@ -656,7 +674,7 @@ get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok)
 
 
 /*
- * get_foreign_server_oid - given a FDW name, look up the OID
+ * get_foreign_server_oid - given a server name, look up the OID
  *
  * If missing_ok is false, throw an error if name not found.  If true, just
  * return InvalidOid.