rework for scansadvice2

This seems to actually kind of work, but there are definitely some things
that are broken.

In particular, PARTITIONWISE() advice is being emitted for plans where
Append nodes are induced by setops rather than by table inheritance;
and what looks like extra PARTITIONWISE() advice is also being emitted
for non-obvious reasons in some table inheritance cases.

Also, INDEX_SCAN(), INDEX_ONLY_SCAN(), and BITMAP_HEAP_SCAN() output
needs revising, and ORDINARY_SCAN() output needs suppressing.

6 files changed, 227 insertions, 196 deletions

diff --git a/contrib/pg_plan_advice/pgpa_join.c b/contrib/pg_plan_advice/pgpa_join.c
index 9bbb5d1932..f6722f1594 100644
--- a/contrib/pg_plan_advice/pgpa_join.c
+++ b/contrib/pg_plan_advice/pgpa_join.c
@@ -55,29 +55,6 @@ static bool is_result_node_with_child(Plan *plan);
 static bool is_sorting_plan(Plan *plan);
 
 /*
- * Should a given plan node be represented by a pgpa_unrolled_join, a
- * pgpa_clumped_join, or neither?
- */
-pgpa_join_class
-pgpa_get_join_class(Plan *plan)
-{
-	/* Standard join types can be unrolled. */
-	if (IsA(plan, NestLoop) || IsA(plan, MergeJoin) || IsA(plan, HashJoin))
-		return PGPA_UNROLLED_JOIN;
-
-	/*
-	 * If a Result node, ForeignScan node, Append node, or MergeAppend node
-	 * has multiple relids, then it's a clumped join; otherwise, it's not a
-	 * join at all.
-	 */
-	if (bms_membership(pgpa_relids(plan)) == BMS_MULTIPLE)
-		return PGPA_CLUMPED_JOIN;
-
-	/* Doesn't seem to be a join. */
-	return PGPA_NON_JOIN;
-}
-
-/*
  * Create an initially-empty object for unrolling joins.
  *
  * See comments for pgpa_unrolled_join and pgpa_clumped_join in pgpa_join.h
@@ -161,7 +138,6 @@ pgpa_unroll_join(pgpa_plan_walker_context *walker, Plan *plan,
 	 * Since we've already handled nodes that require pass-through treatment,
 	 * this should be an unrollable join.
 	 */
-	Assert(pgpa_get_join_class(plan) == PGPA_UNROLLED_JOIN);
 	strategy = pgpa_decompose_join(walker, plan,
 								   &realouter, &realinner,
 								   &elidedouter, &elidedinner);
@@ -195,8 +171,7 @@ pgpa_unroll_join(pgpa_plan_walker_context *walker, Plan *plan,
 	 * into this object and must remember the outer side as the final outer
 	 * subplan.
 	 */
-	if (elidedouter == NULL &&
-		pgpa_get_join_class(realouter) == PGPA_UNROLLED_JOIN)
+	if (elidedouter == NULL && pgpa_is_join(realouter))
 		*outer_join_unroller = join_unroller;
 	else
 	{
@@ -212,8 +187,7 @@ pgpa_unroll_join(pgpa_plan_walker_context *walker, Plan *plan,
 	join_unroller->strategy[n] = strategy;
 	join_unroller->inner_subplans[n] = realinner;
 	join_unroller->inner_elided_nodes[n] = elidedinner;
-	if (elidedinner == NULL &&
-		pgpa_get_join_class(realinner) == PGPA_UNROLLED_JOIN)
+	if (elidedinner == NULL && pgpa_is_join(realinner))
 		*inner_join_unroller = pgpa_create_join_unroller();
 	else
 		*inner_join_unroller = NULL;
@@ -486,35 +460,9 @@ pgpa_decompose_join(pgpa_plan_walker_context *walker, Plan *plan,
 static void
 pgpa_fix_scan_or_clump_member(PlannedStmt *pstmt, pgpa_join_member *member)
 {
-	if (member->elided_node != NULL)
-	{
-		/*
-		 * There is an elided node, so this is either a clumped join if the
-		 * elided node mentions multiple relids or a scan if it only mentions
-		 * one.
-		 */
-		if (bms_membership(member->elided_node->relids) == BMS_MULTIPLE)
-			member->clump_join =
-				pgpa_build_scan(pstmt, member->plan, member->elided_node);
-		else
-			member->rti = bms_singleton_member(member->elided_node->relids);
-	}
-	else
-	{
-		/*
-		 * There is no elided node, so we determine whether this is a clumped
-		 * join or a scan by looking at the plan node itself.
-		 */
-		if (pgpa_get_join_class(member->plan) == PGPA_CLUMPED_JOIN)
-			member->clump_join =
-				pgpa_build_scan(pstmt, member->plan, member->elided_node);
-		else
-		{
-			member->rti = pgpa_scanrelid(member->plan);
-			if (member->rti == 0)
-				elog_node_display(ERROR, "purported scan node has no RTI", member->plan, true);
-		}
-	}
+	/* XXX remove this function */
+	member->scan =
+		pgpa_build_scan(pstmt, member->plan, member->elided_node, true);
 }
 
 /*
@@ -622,13 +570,14 @@ pgpa_unrolled_join_all_relids(pgpa_unrolled_join *join)
 static Bitmapset *
 pgpa_add_member_relids(Bitmapset *relids, pgpa_join_member *member)
 {
-	if (member->clump_join != NULL)
-		return bms_union(relids, member->clump_join->relids);
-	else if (member->unrolled_join != NULL)
+	if (member->unrolled_join != NULL)
 		return bms_union(relids,
 						 pgpa_unrolled_join_all_relids(member->unrolled_join));
 	else
-		return bms_add_member(relids, member->rti);
+	{
+		Assert(member->scan != NULL);
+		return bms_union(relids, member->scan->relids);
+	}
 }
 
 /*
diff --git a/contrib/pg_plan_advice/pgpa_join.h b/contrib/pg_plan_advice/pgpa_join.h
index 58861eebfa..7c861fb446 100644
--- a/contrib/pg_plan_advice/pgpa_join.h
+++ b/contrib/pg_plan_advice/pgpa_join.h
@@ -39,8 +39,8 @@ typedef enum
 #define NUM_PGPA_JOIN_STRATEGY		((int) JSTRAT_HASH_JOIN + 1)
 
 /*
- * Within a pgpa_unrolled_join, we may need to refer to either to scans or to
- * other joins, which may be either unrolled or clumped.
+ * In an outer-deep join tree, every member of an unrolled join will be a scan,
+ * but join trees with other shapes can contain unrolled joins.
  *
  * The plan node we store here will be the inner or outer child of the join
  * node, as appropriate, except that we look through subnodes that we regard as
@@ -49,18 +49,16 @@ typedef enum
  * not the Materialize node itself.
  *
  * If setrefs processing elided one or more nodes from the plan tree, then
- * we'll store details about the topmost of those in elided_node.
+ * we'll store details about the topmost of those in elided_node; otherwise,
+ * it will be NULL.
  *
- * If this is a scan, rti will be the RTI taken from elided_node, or if that
- * is NULL, from plan. Otherwise, this is a join, and either clump_join or
- * unrolled_join will be set as appropriate; otherwise, they will be NULL.
+ * Exactly one of scan and unrolled_join will be non-NULL.
  */
 typedef struct
 {
 	Plan	   *plan;
 	ElidedNode *elided_node;
-	Index		rti;
-	struct pgpa_scan *clump_join;
+	struct pgpa_scan *scan;
 	pgpa_unrolled_join *unrolled_join;
 } pgpa_join_member;
 
@@ -97,7 +95,14 @@ typedef enum
 	PGPA_UNROLLED_JOIN
 } pgpa_join_class;
 
-extern pgpa_join_class pgpa_get_join_class(Plan *plan);
+/*
+ * Does this plan node inherit from Join?
+ */
+static inline bool
+pgpa_is_join(Plan *plan)
+{
+	return IsA(plan, NestLoop) || IsA(plan, MergeJoin) || IsA(plan, HashJoin);
+}
 
 extern pgpa_join_unroller *pgpa_create_join_unroller(void);
 extern void pgpa_unroll_join(struct pgpa_plan_walker_context *walker,
diff --git a/contrib/pg_plan_advice/pgpa_output.c b/contrib/pg_plan_advice/pgpa_output.c
index bec5b04345..1aca7fd679 100644
--- a/contrib/pg_plan_advice/pgpa_output.c
+++ b/contrib/pg_plan_advice/pgpa_output.c
@@ -140,13 +140,9 @@ pgpa_output_advice(StringInfo buf, pgpa_plan_walker_context *walker,
 	 * Emit just one piece of advice for each scan strategy that is used at
 	 * least once in the query.
 	 *
-	 * XXX. It's not entirely clear that emitting DEGENERATE() advice is
-	 * useful at all, since there is no real decision to be made: you can't
-	 * decide whether or not something is provably empty. On the other hand,
-	 * it might be useful to human beings even if the computer doesn't need
-	 * it. FOREIGN() and PARTITIONWISE() are clearly useful, but they are scan
-	 * types as well as join types, and the question of how to handle that
-	 * deserves more thought.
+	 * XXX. We shouldn't emit anything for ordinary scans, and we should
+	 * emit more detailed information for index, index-only, and
+	 * bitmap-heap scans.
 	 */
 	for (int c = 0; c < NUM_PGPA_SCAN_STRATEGY; ++c)
 	{
@@ -172,9 +168,14 @@ pgpa_output_advice(StringInfo buf, pgpa_plan_walker_context *walker,
 				appendStringInfoChar(context.buf, ' ');
 			}
 
-			appendStringInfoChar(buf, '(');
-			pgpa_output_relations(&context, buf, relids);
-			appendStringInfoChar(buf, ')');
+			if (bms_membership(relids) == BMS_SINGLETON)
+				pgpa_output_relations(&context, buf, relids);
+			else
+			{
+				appendStringInfoChar(buf, '(');
+				pgpa_output_relations(&context, buf, relids);
+				appendStringInfoChar(buf, ')');
+			}
 		}
 		appendStringInfoChar(buf, ')');
 	}
@@ -266,12 +267,13 @@ pgpa_output_unrolled_join(pgpa_output_context *context,
 		appendStringInfoChar(context->buf, ' ');
 		pgpa_output_join_member(context, member);
 
-		if (member->clump_join != NULL)
-			relids = member->clump_join->relids;
-		else if (member->unrolled_join != NULL)
+		if (member->unrolled_join != NULL)
 			relids = pgpa_unrolled_join_all_relids(member->unrolled_join);
 		else
-			relids = bms_make_singleton(member->rti);
+		{
+			Assert(member->scan != NULL);
+			relids = member->scan->relids;
+		}
 		context->unrolled_joins[join->strategy[k]] =
 			lappend(context->unrolled_joins[join->strategy[k]], relids);
 	}
@@ -287,17 +289,7 @@ static void
 pgpa_output_join_member(pgpa_output_context *context,
 						pgpa_join_member *member)
 {
-	if (member->clump_join != NULL)
-	{
-		pgpa_scan  *scan = member->clump_join;
-
-		appendStringInfoChar(context->buf, '{');
-		pgpa_output_relations(context, context->buf, scan->relids);
-		appendStringInfoChar(context->buf, '}');
-		context->scans[scan->strategy] =
-			lappend(context->scans[scan->strategy], scan->relids);
-	}
-	else if (member->unrolled_join != NULL)
+	if (member->unrolled_join != NULL)
 	{
 		appendStringInfoChar(context->buf, '(');
 		pgpa_output_unrolled_join(context, member->unrolled_join);
@@ -305,10 +297,19 @@ pgpa_output_join_member(pgpa_output_context *context,
 	}
 	else
 	{
-		if (context->rt_identifiers[member->rti - 1] == NULL)
-			elog(ERROR, "no identifier for RTI %d", member->rti);
-		appendStringInfoString(context->buf,
-							   context->rt_identifiers[member->rti - 1]);
+		pgpa_scan  *scan = member->scan;
+
+		Assert(scan != NULL);
+		if (bms_membership(scan->relids) == BMS_SINGLETON)
+			pgpa_output_relations(context, context->buf, scan->relids);
+		else
+		{
+			appendStringInfoChar(context->buf, '{');
+			pgpa_output_relations(context, context->buf, scan->relids);
+			appendStringInfoChar(context->buf, '}');
+		}
+		context->scans[scan->strategy] =
+			lappend(context->scans[scan->strategy], scan->relids);
 	}
 }
 
@@ -378,11 +379,21 @@ pgpa_cstring_scan_strategy(pgpa_scan_strategy strategy)
 {
 	switch (strategy)
 	{
-		case JSTRAT_CLUMP_DEGENERATE:
-			return "DEGENERATE";
-		case JSTRAT_CLUMP_FOREIGN:
-			return "FOREIGN";
-		case JSTRAT_CLUMP_PARTITIONWISE:
+		case PGPA_SCAN_ORDINARY:
+			return "ORDINARY_SCAN";
+		case PGPA_SCAN_SEQ:
+			return "SEQ_SCAN";
+		case PGPA_SCAN_BITMAP_HEAP:
+			return "BITMAP_HEAP_SCAN";
+		case PGPA_SCAN_FOREIGN:
+			return "FOREIGN_SCAN";
+		case PGPA_SCAN_CUSTOM:
+			return "CUSTOM_SCAN";
+		case PGPA_SCAN_INDEX:
+			return "INDEX_SCAN";
+		case PGPA_SCAN_INDEX_ONLY:
+			return "INDEX_ONLY_SCAN";
+		case PGPA_SCAN_PARTITIONWISE:
 			return "PARTITIONWISE";
 	}
 
diff --git a/contrib/pg_plan_advice/pgpa_scan.c b/contrib/pg_plan_advice/pgpa_scan.c
index 8865773b68..61455c1874 100644
--- a/contrib/pg_plan_advice/pgpa_scan.c
+++ b/contrib/pg_plan_advice/pgpa_scan.c
@@ -22,72 +22,141 @@
  *
  * If there is at least one ElidedNode for this plan node, pass the uppermost
  * one as elided_node, else pass NULL.
+ *
+ * Set the 'force' flag if we're inside of a join problem and not otherwise;
+ * see comments below for why this matters.
+ *
+ * The return value can be NULL only if force is false.
  */
 pgpa_scan *
-pgpa_build_scan(PlannedStmt *pstmt, Plan *plan, ElidedNode *elided_node)
+pgpa_build_scan(PlannedStmt *pstmt, Plan *plan, ElidedNode *elided_node,
+				bool force)
 {
-	pgpa_scan  *scan = palloc(sizeof(pgpa_scan));
+	pgpa_scan  *scan;
+	pgpa_scan_strategy	strategy = PGPA_SCAN_ORDINARY;
 	Bitmapset  *relids = NULL;
 	int			rti = -1;
-
-	scan->plan = plan;
+	bool		filter_join_rtis = false;
 
 	if (elided_node != NULL)
 	{
 		NodeTag		elided_type = elided_node->elided_type;
 
 		/*
-		 * The only case we expect to encounter here is a partitionwise join
-		 * whose Append or MergeAppend node was elided due to having only one
-		 * surviving child. It's also possiblee for a trivial SubqueryScan to
-		 * be elided, but in that case we expected only a single RTI, in which
-		 * case it's not a join.
+		 * If setrefs processing elided an Append or MergeAppend node that
+		 * had only one surviving child, then this is a partitionwise
+		 * "scan" -- which may really be a partitionwise join, but there's
+		 * no need to distinguish.
+		 *
+		 * If it's a trivial SubqueryScan that was elided, then this is an
+		 * "ordinary" scan i.e. one for which we need to generate advice
+		 * because the planner has not made any meaningful choice.
 		 */
-		Assert(bms_membership(elided_node->relids) == BMS_MULTIPLE);
 		relids = elided_node->relids;
 		if (elided_type == T_Append || elided_type == T_MergeAppend)
-			scan->strategy = JSTRAT_CLUMP_PARTITIONWISE;
+			strategy = PGPA_SCAN_PARTITIONWISE;
 		else
-			elog(ERROR, "unexpected elided node type");
+			strategy = PGPA_SCAN_ORDINARY;
+		filter_join_rtis = true;
 	}
-	else
+	else if ((rti = pgpa_scanrelid(plan)) != 0)
 	{
-		Assert(pgpa_get_join_class(plan) == PGPA_CLUMPED_JOIN);
+		relids = bms_make_singleton(rti);
 
-		relids = pgpa_relids(plan);
-
-		if (IsA(plan, Result))
-			scan->strategy = JSTRAT_CLUMP_DEGENERATE;
-		else if (IsA(plan, ForeignScan))
-			scan->strategy = JSTRAT_CLUMP_FOREIGN;
-		else if (IsA(plan, Append))
-			scan->strategy = JSTRAT_CLUMP_PARTITIONWISE;
-		else if (IsA(plan, MergeAppend))
-			scan->strategy = JSTRAT_CLUMP_PARTITIONWISE;
-		else
-			elog(ERROR, "unexpected plan type");
+		switch (nodeTag(plan))
+		{
+			case T_SeqScan:
+				strategy = PGPA_SCAN_SEQ;
+				break;
+			case T_BitmapHeapScan:
+				strategy = PGPA_SCAN_BITMAP_HEAP;
+				break;
+			case T_ForeignScan:
+				strategy = PGPA_SCAN_FOREIGN;
+				break;
+			case T_CustomScan:
+				strategy = PGPA_SCAN_CUSTOM;
+				break;
+			case T_IndexScan:
+				strategy = PGPA_SCAN_INDEX;
+				break;
+			case T_IndexOnlyScan:
+				strategy = PGPA_SCAN_INDEX_ONLY;
+				break;
+			default:
+				strategy = PGPA_SCAN_ORDINARY;
+				break;
+		}
+	}
+	else if ((relids = pgpa_relids(plan)) != NULL)
+	{
+		switch (nodeTag(plan))
+		{
+			case T_ForeignScan:
+				strategy = PGPA_SCAN_FOREIGN;
+				break;
+			case T_Append:
+			case T_MergeAppend:
+				strategy = PGPA_SCAN_PARTITIONWISE;
+				break;
+			default:
+				strategy = PGPA_SCAN_ORDINARY;
+				break;
+		}
+		filter_join_rtis = true;
 	}
 
 	/*
-	 * Filter out any RTIs of type RTE_JOIN, since we have no use for them,
-	 * and don't want them creating confusion later. (We always refer to
-	 * groups of relations in terms of the relation RTIs, not the join RTIs.)
+	 * If this plan node has no associated RTIs, it's not a scan. When the
+	 * 'force' flag is set, that's unexpected, so throw an error, else return
+	 * quietly.
 	 */
-	scan->relids = NULL;
-	while ((rti = bms_next_member(relids, rti)) >= 0)
+	if (relids == NULL)
 	{
-		RangeTblEntry *rte = rt_fetch(rti, pstmt->rtable);
-
-		if (rte->rtekind != RTE_JOIN)
-			scan->relids = bms_add_member(scan->relids, rti);
+		if (force)
+			elog(ERROR, "plan node has no RTIs: %d", (int) nodeTag(plan));
+		return NULL;
 	}
 
 	/*
-	 * We concluded that this was a join based on the fact that there were
-	 * multiple RTIs -- and even after removing the join RTIs, that should
-	 * still be the case.
+	 * Ordinary scans appearing outside of a join problem need not be tracked,
+	 * since nothing interesting can be said either about the choice of join
+	 * method or about the join order.
 	 */
-	Assert(bms_membership(scan->relids) == BMS_MULTIPLE);
+	if (!force && strategy == PGPA_SCAN_ORDINARY)
+		return NULL;
+
+	/* Now create the result object. */
+	scan = palloc(sizeof(pgpa_scan));
+	scan->plan = plan;
+	scan->strategy = strategy;
+
+	/*
+	 * In some cases, it's possible for RTIs of type RTE_JOIN to be included,
+	 * and when that is the case, we want to filter them out.  We always refer
+	 * to groups of relations in terms of the relation RTIs, not the join RTIs,
+	 * so the join RTIs are just noise.
+	 *
+	 * When the code earlier in this function knows that no join RTIs can have
+	 * infiltrated the relids set, it can set filter_join_rtis = false to save
+	 * a few CPU cycles here.
+	 *
+	 * One important case where join RTIs can definitely occur is when we
+	 * encounter a Result node where a whole join result has been proven empty.
+	 */
+	if (!filter_join_rtis)
+		scan->relids = relids;
+	else
+	{
+		scan->relids = NULL;
+		while ((rti = bms_next_member(relids, rti)) >= 0)
+		{
+			RangeTblEntry *rte = rt_fetch(rti, pstmt->rtable);
+
+			if (rte->rtekind != RTE_JOIN)
+				scan->relids = bms_add_member(scan->relids, rti);
+		}
+	}
 
 	return scan;
 }
diff --git a/contrib/pg_plan_advice/pgpa_scan.h b/contrib/pg_plan_advice/pgpa_scan.h
index 5724af1c92..c2a7c645b7 100644
--- a/contrib/pg_plan_advice/pgpa_scan.h
+++ b/contrib/pg_plan_advice/pgpa_scan.h
@@ -21,32 +21,44 @@
 #include "nodes/plannodes.h"
 
 /*
- * Certain types of plan nodes can join any number of input relations in
- * a single step; we call these "clumped joins".
+ * Scan strategies.
  *
- * For our purposes, the important thing about a clumped join is that we
- * can't meaningfully speak about the order in which tables are joined
- * within a single clump. For example, if the optimizer chooses a
- * partitionwise join on tables A and B, we can't say whether A was joined
- * to B or whether B was joined to A; instead, each pair of child tables
- * has its own join order. Likewise, if a foreign data wrapper pushes a
- * join to the remote side, we don't know the join order.
+ * PGPA_SCAN_ORDINARY is any scan strategy that isn't interesting to us
+ * because there is no meaningful planner decision involved. For example,
+ * the only way to scan a subquery is a SubqueryScan, and the only way to
+ * scan a VALUES construct is a ValuesScan. We need not care exactly which
+ * type of planner node was used in such cases, because the same thing will
+ * happen when replanning.
  *
- * JSTRAT_CLUMP_DEGENERATE refers to the case where several relations are
- * all proven empty and replaced with a single Result node. Here again, while
- * the Result node may be joined to other things and we can speak about its
- * place within the larger join order, we can't speak about a join ordering
- * within the Result node itself.
+ * PGPA_SCAN_ORDINARY also includes Result nodes that correspond to scans
+ * or even joins that are proved empty. We don't know whether or not the scan
+ * or join will still be provably empty at replanning time, but if it is,
+ * then no scan-type advice is needed, and if it's not, we can't recommend
+ * a scan type based on the current plan.
+ *
+ * PGPA_SCAN_PARTITIONWISE also lumps together scans and joins: this can
+ * be either a partitionwise scan of a partitioned table or a partitionwise
+ * join between several partitioned tables. Note that all decisions about
+ * whether or not to use partitionwise join are meaningful: no matter what
+ * we decided this time, we could do more or fewer things partitionwise the
+ * next time.
+ *
+ * Other scan strategies map one-to-one to plan nodes.
  */
 typedef enum
 {
-	JSTRAT_CLUMP_DEGENERATE = 0,
-	JSTRAT_CLUMP_FOREIGN,
-	JSTRAT_CLUMP_PARTITIONWISE
-	/* update NUM_PGPA_CLUMP_JOIN_STRATEGY if you add anything here */
+	PGPA_SCAN_ORDINARY = 0,
+	PGPA_SCAN_SEQ,
+	PGPA_SCAN_BITMAP_HEAP,
+	PGPA_SCAN_FOREIGN,
+	PGPA_SCAN_CUSTOM,
+	PGPA_SCAN_INDEX,
+	PGPA_SCAN_INDEX_ONLY,
+	PGPA_SCAN_PARTITIONWISE
+	/* update NUM_PGPA_SCAN_STRATEGY if you add anything here */
 } pgpa_scan_strategy;
 
-#define NUM_PGPA_SCAN_STRATEGY	((int) JSTRAT_CLUMP_PARTITIONWISE + 1)
+#define NUM_PGPA_SCAN_STRATEGY	((int) PGPA_SCAN_PARTITIONWISE + 1)
 
 /*
  * All of the details we need regarding a scan.
@@ -59,6 +71,6 @@ typedef struct pgpa_scan
 } pgpa_scan;
 
 extern pgpa_scan *pgpa_build_scan(PlannedStmt *pstmt, Plan *plan,
-								  ElidedNode *elided_node);
+								  ElidedNode *elided_node, bool force);
 
 #endif
diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c
index 597bb3098c..9eb739220a 100644
--- a/contrib/pg_plan_advice/pgpa_walker.c
+++ b/contrib/pg_plan_advice/pgpa_walker.c
@@ -29,7 +29,6 @@ pgpa_plan_walker(pgpa_plan_walker_context *walker, Plan *plan,
 	pgpa_join_unroller *inner_join_unroller = NULL;
 	bool		join_unroller_toplevel = false;
 	bool		pushdown_query_features = false;
-	pgpa_join_class class;
 	ListCell   *lc;
 	List	   *extraplans = NIL;
 	List	   *elided_nodes = NIL;
@@ -103,19 +102,19 @@ pgpa_plan_walker(pgpa_plan_walker_context *walker, Plan *plan,
 
 		/*
 		 * Every element of elided_nodes is an ElidedNode for which any
-		 * necessary pgpa_clumped_join has not yet been created. Do that here,
-		 * and attach the resulting objects directly to the walker object,
-		 * since we have nowhere else to put a reference to it.
+		 * necessary pgpa_scan has not yet been created. Do that here, and
+		 * attach the resulting objects directly to the walker object, since
+		 * we have nowhere else to put a reference to it.
+		 *
+		 * XXX. Isn't this a bald-faced lie?
 		 */
 		foreach_node(ElidedNode, n, elided_nodes)
 		{
-			if (bms_membership(n->relids) == BMS_MULTIPLE)
-			{
-				pgpa_scan  *scan;
+			pgpa_scan  *scan;
 
-				scan = pgpa_build_scan(walker->pstmt, plan, n);
+			scan = pgpa_build_scan(walker->pstmt, plan, n, false);
+			if (scan != NULL)
 				walker->scans = lappend(walker->scans, scan);
-			}
 		}
 
 		/*
@@ -133,14 +132,11 @@ pgpa_plan_walker(pgpa_plan_walker_context *walker, Plan *plan,
 		join_unroller = NULL;
 	}
 
-	/* Check whether the Plan node is a join, and if so, which kind. */
-	class = pgpa_get_join_class(plan);
-
 	/*
 	 * If this join needs to unrolled but there's no join unroller already
-	 * available, tcreate one.
+	 * available, create one.
 	 */
-	if (class == PGPA_UNROLLED_JOIN && join_unroller == NULL)
+	if (join_unroller == NULL && pgpa_is_join(plan))
 	{
 		join_unroller = pgpa_create_join_unroller();
 		join_unroller_toplevel = true;
@@ -148,16 +144,17 @@ pgpa_plan_walker(pgpa_plan_walker_context *walker, Plan *plan,
 	}
 
 	/*
-	 * If we're underneath a join unroller, it will create a pgpa_clumped_join
-	 * and attach it to the unrolled join. If we're not, we must create the
-	 * pgpa_clumped_join here.
+	 * We only need to handle scans here if we're not underneath a join
+	 * unroller. When we are, the join unroller will create the pgpa_scan and
+	 * and attach it to the unrolled join. Otherwise, do it here.
 	 */
-	if (class == PGPA_CLUMPED_JOIN && join_unroll_level == 0)
+	if (join_unroll_level == 0)
 	{
 		pgpa_scan  *scan;
 
-		scan = pgpa_build_scan(walker->pstmt, plan, NULL);
-		walker->scans = lappend(walker->scans, scan);
+		scan = pgpa_build_scan(walker->pstmt, plan, NULL, false);
+		if (scan != NULL)
+			walker->scans = lappend(walker->scans, scan);
 	}
 
 	/*
@@ -349,19 +346,7 @@ pgpa_scanrelid(Plan *plan)
 		case T_IndexOnlyScan:
 			return ((Scan *) plan)->scanrelid;
 		default:
-			{
-				Bitmapset  *relids = pgpa_relids(plan);
-
-				/*
-				 * If the node type is capable of carrying multiple relids but
-				 * the relids set actually present has exactly one member, we
-				 * regard it as a scan.
-				 */
-				if (bms_membership(relids) == BMS_SINGLETON)
-					return bms_singleton_member(relids);
-
-				return 0;
-			}
+			return 0;
 	}
 }