From 32deb2248ea65f83ca5cdc81459648486d80eac4 Mon Sep 17 00:00:00 2001 From: Nikita Malakhov Date: Tue, 21 Apr 2026 14:10:39 +0300 Subject: [PATCH] [POC] There is very old and well-known problem in FDW wrapper - while deleteing or updating rows of partitioned tables via FDW the FDW engine does not consider partitions which rows belong to, and when user tries to delete or update just 1 row - actually all rows with the same TID are deleted/updated across all partitions. There was previous approach to this problem [1] but it had some unsolved problems resulted in segfaults. Current solution appears to be more complex and requires core modification allowing copying tuples with different amount of attributes. This core patch adds new field in exec params to keep remote table OID and use it along with the TID for delete and update operations. [1] https://www.postgresql.org/message-id/flat/CAPmGK15CQK-oYFMAyq%2BrR0rQapUHtvAGuGgY5ahERHzZ4tmC8g%40mail.gmail.com#4321975bbd1af71c78d45d9a441e8458 --- src/backend/executor/execMain.c | 4 ++ src/backend/executor/nodeForeignscan.c | 2 + src/backend/optimizer/path/allpaths.c | 19 ++++++++ src/backend/optimizer/plan/createplan.c | 19 ++++++++ src/backend/optimizer/plan/initsplan.c | 43 ++++++++++++++++++ src/backend/optimizer/plan/planner.c | 14 +++--- src/backend/optimizer/plan/setrefs.c | 59 ++++++++++++++++++++++++- src/backend/optimizer/plan/subselect.c | 7 ++- src/backend/optimizer/util/appendinfo.c | 27 +++++++++++ src/backend/optimizer/util/relnode.c | 21 +++++++++ src/backend/utils/adt/ruleutils.c | 2 +- src/include/nodes/pathnodes.h | 7 +++ src/include/nodes/primnodes.h | 1 + 13 files changed, 217 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4b30f768680..b7b3b73941e 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2621,6 +2621,10 @@ ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist) resname); if (!AttributeNumberIsValid(aerm->ctidAttNo)) elog(ERROR, "could not find junk %s column", resname); + + snprintf(resname, sizeof(resname), "tableoid%u", erm->rowmarkId); + aerm->toidAttNo = ExecFindJunkAttributeInTlist(targetlist, + resname); } else { diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c index 6f0daddce07..21b3e9a961b 100644 --- a/src/backend/executor/nodeForeignscan.c +++ b/src/backend/executor/nodeForeignscan.c @@ -65,6 +65,8 @@ ForeignNext(ForeignScanState *node) * Insert valid value into tableoid, the only actually-useful system * column. */ + + slot->tts_remoteOid = slot->tts_tableOid; if (plan->fsSystemCol && !TupIsNull(slot)) slot->tts_tableOid = RelationGetRelid(node->ss.ss_currentRelation); diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 61093f222a1..006ab5f89d6 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -1161,6 +1161,25 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, (Node *) rel->reltarget->exprs, 1, &appinfo); + /* Do it if fdw is partition */ + if (planner_rt_fetch(childRTindex, root)->relkind == RELKIND_FOREIGN_TABLE && + !bms_is_empty(root->glob->foreignParamIDs)) + { + foreach(lc, root->processed_tlist) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + Param *param = (Param *) tle->expr; + + if (tle->resjunk && IsA(param, Param) && + IS_FOREIGN_PARAM(root, param) && + param->target_rte == childRTindex) // TODO same for another case + { + childrel->reltarget->exprs = + lappend(childrel->reltarget->exprs, param); + } + } + } + /* * We have to make child entries in the EquivalenceClass data * structures as well. This is needed either if the parent diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index de6a183da79..00ad2dcb07e 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -980,6 +980,25 @@ use_physical_tlist(PlannerInfo *root, Path *path, int flags) } } + /* + * Also, can't do it to a ForeignPath if the path is requested to emit + * Params generated by the FDW. + */ + if (IsA(path, ForeignPath) && + path->parent->relid == root->parse->resultRelation && + !bms_is_empty(root->glob->foreignParamIDs)) + { + foreach(lc, path->pathtarget->exprs) + { + Param *param = (Param *) lfirst(lc); + if (param && IsA(param, Param)) + { + Assert(IS_FOREIGN_PARAM(root, param)); + return false; + } + } + } + return true; } diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index 96ee312ebdf..64be8a8b9d1 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -32,6 +32,7 @@ #include "optimizer/planner.h" #include "optimizer/restrictinfo.h" #include "parser/analyze.h" +#include "parser/parsetree.h" #include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -231,6 +232,38 @@ add_other_rels_to_query(PlannerInfo *root) * *****************************************************************************/ +/* + * add_params_to_result_rel + * If the query's final tlist contains Params the FDW generated, add + * targetlist entries for each such Param to the result relation. + */ +static void +add_params_to_result_rel(PlannerInfo *root, List *final_tlist) +{ + RelOptInfo *target_rel = find_base_rel(root, root->parse->resultRelation); + ListCell *lc; + + /* + * If no parameters have been generated by any FDWs, we certainly don't + * need to do anything here. + */ + if (bms_is_empty(root->glob->foreignParamIDs)) + return; + + foreach(lc, final_tlist) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + Param *param = (Param *) tle->expr; + + if (tle->resjunk && IsA(param, Param) && + IS_FOREIGN_PARAM(root, param) && + param->target_rte == target_rel->relid) + { + target_rel->reltarget->exprs = lappend(target_rel->reltarget->exprs, param); + } + } +} + /* * build_base_rel_tlists * Add targetlist entries for each var needed in the query's final tlist @@ -270,6 +303,16 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist) list_free(having_vars); } } + + if (root->parse->commandType == CMD_UPDATE || + root->parse->commandType == CMD_DELETE) + { + int result_relation = root->parse->resultRelation; + + if (planner_rt_fetch(result_relation, root)->relkind == RELKIND_FOREIGN_TABLE) + add_params_to_result_rel(root, final_tlist); + + } } /* diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 4ec76ce31a9..d168368c2fe 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -375,6 +375,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions, glob->dependsOnRole = false; glob->partition_directory = NULL; glob->rel_notnullatts_hash = NULL; + glob->foreignParamIDs = NULL; /* * Assess whether it's feasible to use parallel mode for this query. We @@ -603,12 +604,15 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions, } /* - * If any Params were generated, run through the plan tree and compute - * each plan node's extParam/allParam sets. Ideally we'd merge this into - * set_plan_references' tree traversal, but for now it has to be separate - * because we need to visit subplans before not after main plan. + * If any Params were generated by the planner not by FDWs, run through + * the plan tree and compute each plan node's extParam/allParam sets. + * (Params added by FDWs are irrelevant for parameter change signaling.) + * Ideally we'd merge this into set_plan_references' tree traversal, but + * for now it has to be separate because we need to visit subplans before + * not after main plan. */ - if (glob->paramExecTypes != NIL) + if (glob->paramExecTypes != NIL && + bms_num_members(glob->foreignParamIDs) < list_length(glob->paramExecTypes)) { Assert(list_length(glob->subplans) == list_length(glob->subroots)); forboth(lp, glob->subplans, lr, glob->subroots) diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index ff0e875f2a2..87e97360eae 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -1901,7 +1901,8 @@ set_append_references(PlannerInfo *root, { Plan *p = (Plan *) linitial(aplan->appendplans); - if (p->parallel_aware == aplan->plan.parallel_aware) + if ((p->parallel_aware == aplan->plan.parallel_aware) && + (list_length(((Plan *) aplan)->targetlist) == list_length(p->targetlist))) { Plan *result; @@ -3310,7 +3311,42 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context) } /* Special cases (apply only AFTER failing to match to lower tlist) */ if (IsA(node, Param)) + { + Param *param = (Param *) node; + + /* + * If the Param is a PARAM_EXEC Param generated by an FDW, it should + * have bubbled up from a lower plan node; convert it into a simple + * Var referencing the output of the subplan. + * + * Note: set_join_references() would have kept has_non_vars=true for + * the subplan emitting the Param since it effectively belong to the + * result relation and that relation can never be the nullable side of + * an outer join. + */ + if (IS_FOREIGN_PARAM(context->root, param)) + { + if (context->outer_itlist && context->outer_itlist->has_non_vars) + { + newvar = search_indexed_tlist_for_non_var((Expr *) node, + context->outer_itlist, + OUTER_VAR); + if (newvar) + return (Node *) newvar; + } + if (context->inner_itlist && context->inner_itlist->has_non_vars) + { + newvar = search_indexed_tlist_for_non_var((Expr *) node, + context->inner_itlist, + INNER_VAR); + if (newvar) + return (Node *) newvar; + } + // XXX Is it an error to be here? + } + /* If not, do fix_param_node() */ return fix_param_node(context->root, (Param *) node); + } if (IsA(node, AlternativeSubPlan)) return fix_join_expr_mutator(fix_alternative_subplan(context->root, (AlternativeSubPlan *) node, @@ -3421,7 +3457,28 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context) } /* Special cases (apply only AFTER failing to match to lower tlist) */ if (IsA(node, Param)) + { + Param *param = (Param *) node; + /* + * If the Param is a PARAM_EXEC Param generated by an FDW, it should + * have bubbled up from a lower plan node; convert it into a simple + * Var referencing the output of the subplan. + */ + if (IS_FOREIGN_PARAM(context->root, param)) + { + if (context->subplan_itlist->has_non_vars) + { + newvar = search_indexed_tlist_for_non_var((Expr *) node, + context->subplan_itlist, + context->newvarno); + if (newvar) + return (Node *) newvar; + } + // XXX Is it an error to be here? + } + /* If not, do fix_param_node() */ return fix_param_node(context->root, (Param *) node); + } if (IsA(node, Aggref)) { Aggref *aggref = (Aggref *) node; diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index ccec1eaa7fe..375fdf27e53 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -3192,7 +3192,12 @@ finalize_primnode(Node *node, finalize_primnode_context *context) { int paramid = ((Param *) node)->paramid; - context->paramids = bms_add_member(context->paramids, paramid); + /* + * Params added by FDWs are irrelevant for parameter change + * signaling. + */ + if (!bms_is_member(paramid, context->root->glob->foreignParamIDs)) + context->paramids = bms_add_member(context->paramids, paramid); } return false; /* no more to do here */ } diff --git a/src/backend/optimizer/util/appendinfo.c b/src/backend/optimizer/util/appendinfo.c index 778e4662f6e..7089a267125 100644 --- a/src/backend/optimizer/util/appendinfo.c +++ b/src/backend/optimizer/util/appendinfo.c @@ -948,6 +948,29 @@ add_row_identity_var(PlannerInfo *root, Var *orig_var, root->processed_tlist = lappend(root->processed_tlist, tle); } +static void +fix_foreign_params(PlannerInfo *root, List *tlist) +{ + ListCell *lc; + + foreach(lc, tlist) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + Param *param = (Param *) tle->expr; + + if (tle->resjunk && IsA(param, Param) && + param->paramkind == PARAM_EXEC && + param->paramid == -1) + { + param->paramid = list_length(root->glob->paramExecTypes); + root->glob->paramExecTypes = + lappend_oid(root->glob->paramExecTypes, param->paramtype); + root->glob->foreignParamIDs = + bms_add_member(root->glob->foreignParamIDs, param->paramid); + } + } +} + /* * add_row_identity_columns * @@ -992,8 +1015,12 @@ add_row_identity_columns(PlannerInfo *root, Index rtindex, fdwroutine = GetFdwRoutineForRelation(target_relation, false); if (fdwroutine->AddForeignUpdateTargets != NULL) + { + fdwroutine->AddForeignUpdateTargets(root, rtindex, target_rte, target_relation); + fix_foreign_params(root, root->processed_tlist); + } /* * For UPDATE, we need to make the FDW fetch unchanged columns by diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index 3fc2c2f71d0..655b2b00bd7 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -1320,6 +1320,27 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel, } continue; } + /* + * We allow FDWs to have PARAM_EXEC Params here. + */ + else if (IsA(var, Param)) + { + Param *param = (Param *) var; + + Assert(IS_FOREIGN_PARAM(root, param)); + + joinrel->reltarget->exprs = + lappend(joinrel->reltarget->exprs, param); + + /* + * Estimate using the type info (Note: keep this in sync with + * set_rel_width()) + */ + joinrel->reltarget->width += + get_typavgwidth(param->paramtype, param->paramtypmod); + + continue; + } /* * Otherwise, anything in a baserel or joinrel targetlist ought to be diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 090e8cc28c1..d2357018d0a 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -9374,7 +9374,7 @@ get_parameter(Param *param, deparse_context *context) * It's a bug if we get here for anything except PARAM_EXTERN Params, but * in production builds printing $N seems more useful than failing. */ - Assert(param->paramkind == PARAM_EXTERN); + Assert(param->paramkind == PARAM_EXTERN || param->paramkind == PARAM_EXEC); appendStringInfo(context->buf, "$%d", param->paramid); } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 693b879f76d..3ad051d0af4 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -265,6 +265,9 @@ typedef struct PlannerGlobal /* partition descriptors */ PartitionDirectory partition_directory pg_node_attr(read_write_ignore); + /* PARAM_EXEC Params generated by FDWs */ + Bitmapset *foreignParamIDs; + /* hash table for NOT NULL attnums of relations */ struct HTAB *rel_notnullatts_hash pg_node_attr(read_write_ignore); @@ -277,6 +280,10 @@ typedef struct PlannerGlobal #define planner_subplan_get_plan(root, subplan) \ ((Plan *) list_nth((root)->glob->subplans, (subplan)->plan_id - 1)) +/* macro for checking if a Param is a PARAM_EXEC Param generated by an FDW */ +#define IS_FOREIGN_PARAM(root, param) \ + ((param)->paramkind == PARAM_EXEC && \ + bms_is_member((param)->paramid, (root)->glob->foreignParamIDs)) /*---------- * PlannerInfo diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 6dfc946c20b..b85c54e8ec2 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -402,6 +402,7 @@ typedef struct Param Oid paramcollid; /* token location, or -1 if unknown */ ParseLoc location; + Index target_rte; } Param; /* -- 2.43.0