Re: partition routing layering in nodeModifyTable.c

On 12/10/2020 16:47, Amit Langote wrote:
> On Mon, Oct 12, 2020 at 8:12 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>> 1. We have many different cleanup/close routines now:
>> ExecCloseResultRelations, ExecCloseRangeTableRelations, and
>> ExecCleanUpTriggerState. Do we need them all? It seems to me that we
>> could merge ExecCloseRangeTableRelations() and
>> ExecCleanUpTriggerState(), they seem to do roughly the same thing: close
>> relations that were opened for ResultRelInfos. They are always called
>> together, except in afterTriggerInvokeEvents(). And in
>> afterTriggerInvokeEvents() too, there would be no harm in doing both,
>> even though we know there aren't any entries in the es_result_relations
>> array at that point.
>
> Hmm, I find trigger result relations to behave differently enough to
> deserve a separate function. For example, unlike plan-specified
> result relations, they don't point to range table relations and don't
> open indices. Maybe the name could be revisited, say,
> ExecCloseTriggerResultRelations().

Matter of perception I guess. I still prefer to club them together into
one Close call. It's true that they're slightly different, but they're
also pretty similar. And IMHO they're more similar than different.

> Also, maybe call the other functions:
>
> ExecInitPlanResultRelationsArray()
> ExecInitPlanResultRelation()
> ExecClosePlanResultRelations()
>
> Thoughts?

Hmm. How about initializing the array lazily, on the first
ExecInitPlanResultRelation() call? It's not performance critical, and
that way there's one fewer initialization function that you need to
remember to call.

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.

> 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.

- Heikki