On 13/10/2020 07:32, Amit Langote wrote:
On Tue, Oct 13, 2020 at 1:57 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
It occurred to me that if we do that (initialize the array lazily),
there's very little need for the PlannedStmt->resultRelations list
anymore. It's only used in ExecRelationIsTargetRelation(), but if we
assume that ExecRelationIsTargetRelation() is only called after InitPlan
has initialized the result relation for the relation, it can easily
check es_result_relations instead. I think that's a safe assumption.
ExecRelationIsTargetRelation() is only used in FDWs, and I believe the
FDWs initialization routine can only be called after ExecInitModifyTable
has been called on the relation.
The PlannedStmt->rootResultRelations field is even more useless.
I am very much tempted to remove those fields from PlannedStmt,
although I am concerned that the following now assumes that *all*
result relations are initialized in the executor initialization phase:
bool
ExecRelationIsTargetRelation(EState *estate, Index scanrelid)
{
if (!estate->es_result_relations)
return false;
return estate->es_result_relations[scanrelid - 1] != NULL;
}
In the other thread [1], I am proposing that we initialize result
relations lazily, but the above will be a blocker to that.
Ok, I'll leave it alone then. But I'll still merge resultRelations and
rootResultRelations into one list. I don't see any point in keeping them
separate.
I'm tempted to remove ExecRelationIsTargetRelation() altogether, but
keeping the resultRelations list isn't really a big deal, so I'll leave
that for another discussion.
Actually, maybe we don't need to be so paranoid about setting up
es_result_relations in worker.c, because none of the downstream
functionality invoked seems to rely on it, that is, no need to call
ExecInitResultRelationsArray() and ExecInitResultRelation().
ExecSimpleRelation* and downstream functionality assume a
single-relation operation and the ResultRelInfo is explicitly passed.
Hmm, yeah, I like that. Similarly in ExecuteTruncateGuts(), there isn't
actually any need to put the ResultRelInfos in the es_result_relations
array.
Putting all this together, I ended up with the attached. It doesn't
include the subsequent commits in this patch set yet, for removal of
es_result_relation_info et al.
Thanks.
+ * We put the ResultRelInfos in the es_opened_result_relations list, even
+ * though we don't have a range table and don't populate the
+ * es_result_relations array. That's a big bogus, but it's enough to make
+ * ExecGetTriggerResultRel() find them.
*/
estate = CreateExecutorState();
resultRelInfos = (ResultRelInfo *)
palloc(list_length(rels) * sizeof(ResultRelInfo));
resultRelInfo = resultRelInfos;
+ estate->es_result_relations = (ResultRelInfo **)
+ palloc(list_length(rels) * sizeof(ResultRelInfo *));
Maybe don't allocate es_result_relations here?
Fixed.
Anyway, other than my concern about ExecRelationIsTargetRelation()
mentioned above, I think the patch looks good.
Ok, committed. I'll continue to look at the rest of the patches in this
patch series now.
- Heikki
