Re: partition routing layering in nodeModifyTable.c

On 13/10/2020 07:32, Amit Langote wrote:
> On Tue, Oct 13, 2020 at 1:57 AM Heikki Linnakangas <hlinnaka@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