commit e0f35326c479ab14809bd4e0da9ef7e53f840d3a Author: Tom Lane Date: Thu Aug 18 14:32:53 2022 -0400 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 immediate detection. postgres_fdw also has one place where it needs to ignore varnullingrels while matching Vars, similarly to the unfinished work in setrefs.c. (That requirement will only affect join-planning FDWs, too, since Vars seen at a base relation scan should never have any varnullingrels.) diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c index a9766f9734..a187cd08fa 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -3952,7 +3952,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); + + /* + * As in setrefs.c, we match 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 16320170ce..85712cdce6 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -1513,13 +1513,13 @@ postgresBeginForeignScan(ForeignScanState *node, int eflags) /* * Identify which user to do the remote access as. This should match what * ExecCheckRTEPerms() does. In case of a join or aggregate, use the - * lowest-numbered member RTE as a representative; we would get the same - * result from any. + * lowest-numbered member base RTE as a representative; we would get the + * same result from any. */ 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); userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); @@ -2407,7 +2407,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; } @@ -2833,8 +2833,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; @@ -2851,7 +2851,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(); @@ -2866,7 +2866,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 d0b5951019..329affa30b 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 e078456b19..36dcfc1fab 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 cd8a3ef7cb..880cbfeb5e 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -29,6 +29,7 @@ #include "optimizer/cost.h" #include "optimizer/optimizer.h" #include "optimizer/paramassign.h" +#include "optimizer/pathnode.h" #include "optimizer/paths.h" #include "optimizer/placeholder.h" #include "optimizer/plancat.h" @@ -4105,6 +4106,8 @@ create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path, Index scan_relid = rel->relid; Oid rel_oid = InvalidOid; Plan *outer_plan = NULL; + Relids fs_base_relids; + int rtindex; Assert(rel->fdwroutine != NULL); @@ -4153,14 +4156,28 @@ 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. + */ + fs_base_relids = NULL; + rtindex = -1; + while ((rtindex = bms_next_member(scan_plan->fs_relids, rtindex)) >= 0) + { + if (find_base_rel_ignore_join(root, rtindex) != NULL) + fs_base_relids = bms_add_member(fs_base_relids, rtindex); + } + scan_plan->fs_base_relids = fs_base_relids; + /* * 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 @@ -5800,8 +5817,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 a1827d113d..1a047ddda2 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1533,6 +1533,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 dca2a21e7a..7e98d0b7a3 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -688,6 +688,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 @@ -707,7 +708,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;