On 13/07/2020 08:47, Amit Langote wrote:
It's been over 11 months since there was any significant commentary on
the contents of the patches themselves, so perhaps I should reiterate
what the patches are about and why it might still be a good idea to
consider them.
The thread started with some very valid criticism of the way
executor's partition tuple routing logic looks randomly sprinkled over
in nodeModifyTable.c, execPartition.c. In the process of making it
look less random, we decided to get rid of the global variable
es_result_relation_info to avoid complex maneuvers of
setting/resetting it correctly when performing partition tuple
routing, causing some other churn beside the partitioning code. Same
with another global variable TransitionCaptureState.tcs_map. So, the
patches neither add any new capabilities, nor improve performance, but
they do make the code in this area a bit easier to follow.
Actually, there is a problem that some of the changes here conflict
with patches being discussed on other threads ([1], [2]), so much so
that I decided to absorb some changes here into another "refactoring"
patch that I have posted at [2].
Thanks for the summary. It's been a bit hard to follow what depends on
what across these threads, and how they work together. It seems that
this patch set is the best place to start.
Attached rebased patches.
0001 contains preparatory FDW API changes to stop relying on
es_result_relation_info being set correctly.
Makes sense. The only thing I don't like about this is the way the
ForeignScan->resultRelIndex field is set. make_foreignscan() initializes
it to -1, and the FDW's PlanDirectModify() function is expected to set
it, like you did in postgres_fdw:
@@ -2319,6 +2322,11 @@ postgresPlanDirectModify(PlannerInfo *root,
rebuild_fdw_scan_tlist(fscan, returningList);
}
+ /*
+ * Set the index of the subplan result rel.
+ */
+ fscan->resultRelIndex = subplan_index;
+
table_close(rel, NoLock);
return true;
}
It has to be set to that value (subplan_index is an argument to
PlanDirectModify()), the FDW doesn't have any choice there, so this is
just additional boilerplate code that has to be copied to every FDW that
implements direct modify. Furthermore, if the FDW doesn't set it
correctly, you could have some very interesting results, like updating
wrong table. It would be better to set it in make_modifytable(), just
after calling PlanDirectModify().
I'm also a bit unhappy with the way it's updated in set_plan_refs():
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -904,6 +904,13 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
rc->rti += rtoffset;
rc->prti += rtoffset;
}
+ /*
+ * Caution: Do not change the relative ordering
of this loop
+ * and the statement below that adds the result
relations to
+ * root->glob->resultRelations, because we need
to use the
+ * current value of
list_length(root->glob->resultRelations)
+ * in some plans.
+ */
foreach(l, splan->plans)
{
lfirst(l) = set_plan_refs(root,
@@ -1243,6 +1250,14 @@ set_foreignscan_references(PlannerInfo *root,
}
fscan->fs_relids = offset_relid_set(fscan->fs_relids, rtoffset);
+
+ /*
+ * Adjust resultRelIndex if it's valid (note that we are called before
+ * adding the RT indexes of ModifyTable result relations to the global
+ * list)
+ */
+ if (fscan->resultRelIndex >= 0)
+ fscan->resultRelIndex +=
list_length(root->glob->resultRelations);
}
/*
That "Caution" comment is well deserved, but could we make this more
robust to begin with? The most straightforward solution would be to pass
down the "current resultRelIndex" as an extra parameter to
set_plan_refs(), similar to rtoffset. If we did that, we wouldn't
actually need to set it before setrefs.c processing at all.
I'm a bit wary of adding another argument to set_plan_refs() because
that's a lot of code churn, but it does seem like the most natural
solution to me. Maybe create a new context struct to hold the
PlannerInfo, rtoffset, and the new "currentResultRelIndex" value,
similar to fix_scan_expr_context, to avoid passing through so many
arguments.
Another idea is to merge "resultRelIndex" and a "range table index" into
one value. Range table entries that are updated would have a
ResultRelInfo, others would not. I'm not sure if that would end up being
cleaner or messier than what we have now, but might be worth trying.
0002 removes es_result_relation_info in favor passing the active
result relation around as a parameter in the various functions that
need it
Looks good.
0003 Moves UPDATE tuple-routing logic into a new function
0004 removes the global variable TransitionCaptureState.tcs_map which
needed to be set/reset whenever the active result relation relation
changes in favor of a new field in ResultRelInfo to store the same map
I didn't look closely, but these make sense at a quick glance.
- Heikki
