commit b187b829578334b13285385f924a896f3a1ee50a Author: Tom Lane Date: Fri Dec 23 10:33:13 2022 -0500 Teach FDWs about base-plus-outer-join relids. Conversion of the planner to include OJ relids in join relids affects FDWs that want to plan foreign joins. They *must* follow suit when labeling foreign joins in order to match with the core planner, but for many purposes (if postgres_fdw is any guide) they'd prefer to consider only base relations within the join. To support both requirements, redefine ForeignScan.fs_relids as base+OJ relids, and add a new field fs_base_relids that's set up by the core planner. Another way we could do this is to keep fs_relids as just base relids and make the new field be the one with OJ relids added. While that would be more backwards-compatible in some sense, it would be inconsistent with the naming used in the core planner, and I think that it might allow some types of bugs to escape quick detection. postgres_fdw also has one place where it needs to ignore varnullingrels while matching Vars. It's not clear whether it's worth trying to improve that. (This too is probably only an issue for FDWs that do join planning, since Vars seen in a base relation scan should never have any varnullingrels.) As of this step, this patch series again passes check-world. diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index 9524765650..94dd7b2c96 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -3950,7 +3950,17 @@ get_relation_column_alias_ids(Var *node, RelOptInfo *foreignrel, i = 1; foreach(lc, foreignrel->reltarget->exprs) { - if (equal(lfirst(lc), (Node *) node)) + Var *tlvar = (Var *) lfirst(lc); + + /* + * Match reltarget entries only on varno/varattno. Ideally there + * would be some cross-check on varnullingrels, but it's unclear what + * to do exactly; we don't have enough context to know what that value + * should be. + */ + if (IsA(tlvar, Var) && + tlvar->varno == node->varno && + tlvar->varattno == node->varattno) { *colno = i; return; diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index b9268e32dd..4028c5692e 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -1517,7 +1517,7 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags) if (fsplan->scan.scanrelid > 0) rtindex = fsplan->scan.scanrelid; else - rtindex = bms_next_member(fsplan->fs_relids, -1); + rtindex = bms_next_member(fsplan->fs_base_relids, -1); rte = exec_rt_fetch(rtindex, estate); /* Get info about foreign table. */ @@ -2414,7 +2414,7 @@ find_modifytable_subplan(PlannerInfo *root, { ForeignScan *fscan = (ForeignScan *) subplan; - if (bms_is_member(rtindex, fscan->fs_relids)) + if (bms_is_member(rtindex, fscan->fs_base_relids)) return fscan; } @@ -2838,8 +2838,8 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es) * that setrefs.c won't update the string when flattening the * rangetable. To find out what rtoffset was applied, identify the * minimum RT index appearing in the string and compare it to the - * minimum member of plan->fs_relids. (We expect all the relids in - * the join will have been offset by the same amount; the Asserts + * minimum member of plan->fs_base_relids. (We expect all the relids + * in the join will have been offset by the same amount; the Asserts * below should catch it if that ever changes.) */ minrti = INT_MAX; @@ -2856,7 +2856,7 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es) else ptr++; } - rtoffset = bms_next_member(plan->fs_relids, -1) - minrti; + rtoffset = bms_next_member(plan->fs_base_relids, -1) - minrti; /* Now we can translate the string */ relations = makeStringInfo(); @@ -2871,7 +2871,7 @@ postgresExplainForeignScan(ForeignScanState *node, ExplainState *es) char *refname; rti += rtoffset; - Assert(bms_is_member(rti, plan->fs_relids)); + Assert(bms_is_member(rti, plan->fs_base_relids)); rte = rt_fetch(rti, es->rtable); Assert(rte->rtekind == RTE_RELATION); /* This logic should agree with explain.c's ExplainTargetRel */ diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index 94263c628f..ac1717bc3c 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -351,6 +351,17 @@ GetForeignJoinPaths(PlannerInfo *root, it will supply at run time in the tuples it returns. + + + Beginning with PostgreSQL 16, + fs_relids includes the rangetable indexes + of outer joins, if any were involved in this join. The new field + fs_base_relids includes only base + relation indexes, and thus + mimics fs_relids's old semantics. + + + See for additional information. diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index f86983c660..ed9a118416 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -1114,7 +1114,7 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used) break; case T_ForeignScan: *rels_used = bms_add_members(*rels_used, - ((ForeignScan *) plan)->fs_relids); + ((ForeignScan *) plan)->fs_base_relids); break; case T_CustomScan: *rels_used = bms_add_members(*rels_used, diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c index 043bb83f55..2b37266b6a 100644 --- a/src/backend/executor/execScan.c +++ b/src/backend/executor/execScan.c @@ -325,7 +325,7 @@ ExecScanReScan(ScanState *node) * all of them. */ if (IsA(node->ps.plan, ForeignScan)) - relids = ((ForeignScan *) node->ps.plan)->fs_relids; + relids = ((ForeignScan *) node->ps.plan)->fs_base_relids; else if (IsA(node->ps.plan, CustomScan)) relids = ((CustomScan *) node->ps.plan)->custom_relids; else diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 66139928e8..78dc153882 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -4158,14 +4158,22 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path, /* * Likewise, copy the relids that are represented by this foreign scan. An - * upper rel doesn't have relids set, but it covers all the base relations - * participating in the underlying scan, so use root's all_baserels. + * upper rel doesn't have relids set, but it covers all the relations + * participating in the underlying scan/join, so use root->all_query_rels. */ if (rel->reloptkind == RELOPT_UPPER_REL) - scan_plan->fs_relids = root->all_baserels; + scan_plan->fs_relids = root->all_query_rels; else scan_plan->fs_relids = best_path->path.parent->relids; + /* + * Join relid sets include relevant outer joins, but FDWs may need to know + * which are the included base rels. That's a bit tedious to get without + * access to the plan-time data structures, so compute it here. + */ + scan_plan->fs_base_relids = bms_difference(scan_plan->fs_relids, + root->outer_join_rels); + /* * If this is a foreign join, and to make it valid to push down we had to * assume that the current user is the same as some user explicitly named @@ -5806,8 +5814,9 @@ make_foreignscan(List *qptlist, node->fdw_private = fdw_private; node->fdw_scan_tlist = fdw_scan_tlist; node->fdw_recheck_quals = fdw_recheck_quals; - /* fs_relids will be filled in by create_foreignscan_plan */ + /* fs_relids, fs_base_relids will be filled by create_foreignscan_plan */ node->fs_relids = NULL; + node->fs_base_relids = NULL; /* fsSystemCol will be filled in by create_foreignscan_plan */ node->fsSystemCol = false; diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 846ca39269..eadfbb7218 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1628,6 +1628,7 @@ set_foreignscan_references(PlannerInfo *root, } fscan->fs_relids = offset_relid_set(fscan->fs_relids, rtoffset); + fscan->fs_base_relids = offset_relid_set(fscan->fs_base_relids, rtoffset); /* Adjust resultRelation if it's valid */ if (fscan->resultRelation > 0) diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index bddfe86191..4d0ffb6039 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -695,6 +695,7 @@ typedef struct WorkTableScan * When the plan node represents a foreign join, scan.scanrelid is zero and * fs_relids must be consulted to identify the join relation. (fs_relids * is valid for simple scans as well, but will always match scan.scanrelid.) + * fs_relids includes outer joins; fs_base_relids does not. * * If the FDW's PlanDirectModify() callback decides to repurpose a ForeignScan * node to perform the UPDATE or DELETE operation directly in the remote @@ -716,7 +717,8 @@ typedef struct ForeignScan List *fdw_private; /* private data for FDW */ List *fdw_scan_tlist; /* optional tlist describing scan tuple */ List *fdw_recheck_quals; /* original quals not in scan.plan.qual */ - Bitmapset *fs_relids; /* RTIs generated by this scan */ + Bitmapset *fs_relids; /* base+OJ RTIs generated by this scan */ + Bitmapset *fs_base_relids; /* base RTIs generated by this scan */ bool fsSystemCol; /* true if any "system column" is needed */ } ForeignScan;