commit 82ea2fee2d71cd9f32fd9bc591d64780f2d7d6f4 Author: Tom Lane Date: Mon Jan 23 14:45:35 2023 -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 473fa45bd4..8a847deb13 100644 --- a/contrib/postgres_fdw/deparse.c +++ b/contrib/postgres_fdw/deparse.c @@ -4026,7 +4026,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 50d23f922c..29b2be602c 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 5212a64b1e..1b0e1bc26a 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 abe8432853..cf1871b0f5 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 cd68942af0..4c99b28d0a 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 8ea257f492..fb28dead4d 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1633,6 +1633,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 c1234fcf36..4781a9c632 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;