From 04447372a3e63035cc2501c338f0f89cce866957 Mon Sep 17 00:00:00 2001 From: Nikita Malakhov Date: Thu, 14 May 2026 17:41:02 +0300 Subject: [PATCH] DELETE/UPDATE more than one row in partitioned foreign table (2/3) There is known bug in delete and update rows in partitioned foreign tables via FDW [1] Core patch - introduction of remote table OID returned from remote query and passing it through planner and executor to use in foreign queries. [1] Discussion: https://www.postgresql.org/message-id/flat/20250718175314.4513c00a%40karst --- src/backend/executor/execMain.c | 4 ++ src/backend/executor/execTuples.c | 4 +- src/backend/executor/nodeForeignscan.c | 2 + src/backend/optimizer/path/allpaths.c | 20 +++++++++ src/backend/optimizer/plan/createplan.c | 19 +++++++++ src/backend/optimizer/plan/initsplan.c | 45 ++++++++++++++++++++ src/backend/optimizer/plan/planner.c | 14 ++++--- src/backend/optimizer/plan/setrefs.c | 56 +++++++++++++++++++++++++ src/backend/optimizer/plan/subselect.c | 7 +++- src/backend/optimizer/util/appendinfo.c | 27 ++++++++++++ src/backend/optimizer/util/inherit.c | 1 + src/backend/optimizer/util/relnode.c | 21 ++++++++++ src/backend/utils/adt/ruleutils.c | 2 +- src/include/executor/tuptable.h | 1 + src/include/nodes/pathnodes.h | 7 ++++ src/include/nodes/primnodes.h | 1 + 16 files changed, 222 insertions(+), 9 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 4b30f768680..d18ee581c6f 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); +//fdwproblem + snprintf(resname, sizeof(resname), "tableoid%u", erm->rowmarkId); + aerm->toidAttNo = ExecFindJunkAttributeInTlist(targetlist, + resname); } else { diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 6322a8a64b3..f8c66bdaef6 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -1911,8 +1911,7 @@ ExecStoreAllNullTuple(TupleTableSlot *slot) * Until the slot is materialized, the contents of the slot depend on the * datum. */ -void -ExecStoreHeapTupleDatum(Datum data, TupleTableSlot *slot) +void ExecStoreHeapTupleDatum(Datum data, TupleTableSlot *slot) { HeapTupleData tuple = {0}; HeapTupleHeader td; @@ -1922,6 +1921,7 @@ ExecStoreHeapTupleDatum(Datum data, TupleTableSlot *slot) tuple.t_len = HeapTupleHeaderGetDatumLength(td); tuple.t_self = td->t_ctid; tuple.t_data = td; + tuple.t_tableOid = InvalidOid; ExecClearTuple(slot); 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..040763caf35 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -1161,6 +1161,26 @@ 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 + { + /* XXX is copyObject necessary here? */ + childrel->reltarget->exprs = + lappend(childrel->reltarget->exprs, copyObject(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 b38422c47a4..cc09065e561 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" @@ -243,6 +244,40 @@ 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) + { + /* XXX is copyObject necessary here? */ + target_rel->reltarget->exprs = lappend(target_rel->reltarget->exprs, + copyObject(param)); + } + } +} + /* * build_base_rel_tlists * Add targetlist entries for each var needed in the query's final tlist @@ -282,6 +317,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 f4689e7c9f8..a560ecfbd8a 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -393,6 +393,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 @@ -621,12 +622,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..4fd162920db 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -3310,7 +3310,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 +3456,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/inherit.c b/src/backend/optimizer/util/inherit.c index 6a7b9edff3f..d8f08485e39 100644 --- a/src/backend/optimizer/util/inherit.c +++ b/src/backend/optimizer/util/inherit.c @@ -294,6 +294,7 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, InvalidOid, 0); snprintf(resname, sizeof(resname), "tableoid%u", oldrc->rowmarkId); + tle = makeTargetEntry((Expr *) var, list_length(root->processed_tlist) + 1, pstrdup(resname), 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 88de5c0481c..004f22084f8 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/executor/tuptable.h b/src/include/executor/tuptable.h index 43018ed4dfa..02ddc4fb79d 100644 --- a/src/include/executor/tuptable.h +++ b/src/include/executor/tuptable.h @@ -141,6 +141,7 @@ typedef struct TupleTableSlot MemoryContext tts_mcxt; /* slot itself is in this context */ ItemPointerData tts_tid; /* stored tuple's tid */ Oid tts_tableOid; /* table oid of tuple */ + Oid tts_remoteOid; } TupleTableSlot; /* routines for a TupleTableSlot implementation */ diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 27a2c6815b7..abe4904cc24 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -271,12 +271,19 @@ typedef struct PlannerGlobal /* extension state */ void **extension_state pg_node_attr(read_write_ignore); int extension_state_allocated; + + /* PARAM_EXEC Params generated by FDWs */ + Bitmapset *foreignParamIDs; } PlannerGlobal; /* macro for fetching the Plan associated with a SubPlan node */ #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 7977ee24783..cd70f673236 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