Re: partition routing layering in nodeModifyTable.c

On 09/10/2020 11:01, Amit Langote wrote:
> On Thu, Oct 8, 2020 at 9:35 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>> On 07/10/2020 12:50, Amit Langote wrote:
>>>> I have thought about something like this before. An idea I had is to
>>>> make es_result_relations array indexable by plain RT indexes, then we
>>>> don't need to maintain separate indexes that we do today for result
>>>> relations.
>>>
>>> That sounds like a good idea. es_result_relations is currently an array
>>> of ResultRelInfos, so that would leave a lot of unfilled structs in the
>>> array. But in on of your other threads, you proposed turning
>>> es_result_relations into an array of pointers anyway
>>> (https://www.postgresql.org/message-id/CA+HiwqE4k1Q2TLmCAvekw+8_NXepbnfUOamOeX=KpHRDTfSKxA@mail.gmail.com).
>>
>> Okay, I am reorganizing the patches around that idea and will post an
>> update soon.
>
> Attached updated patches.
>
> 0001 makes es_result_relations an RTI-indexable array, which allows to
> get rid of all "result relation index" fields across the code.

Thanks! A couple small things I wanted to check with you before committing:

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.

2. The way this is handled in worker.c is a bit funny. In
create_estate_for_relation(), you create a ResultRelInfo, but you
*don't* put it in the es_opened_result_relations list. That's
surprising, but I'm also surprised there are no
ExecCloseResultRelations() calls before the FreeExecutorState() calls in
worker.c. It's not needed because the
apply_handle_insert/update/delete_internal() functions call
ExecCloseIndices() directly, so they don't rely on the
ExecCloseResultRelations() function for cleanup. That works too, but
it's a bit surprising because it's different from how it's done in
copy.c and nodeModifyTable.c. It would feel natural to rely on
ExecCloseResultRelations() in worker.c as well, but on the other hand,
it also calls ExecOpenIndices() in a more lazy fashion, and it makes
sense to call ExecCloseIndices() in the same functions that
ExecOpenIndices() is called. So I'm not sure if changing that would be
an improvement overall. What do you think? Did you consider doing that?

Attached is your original patch v13, and a patch on top of it that
merges ExecCloseResultRelations() and ExecCleanUpTriggerState(), and
makes some minor comment changes. I didn't do anything about the
worker.c business, aside from adding a comment about it.

- Heikki