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@gmail.com> wrote:
>> On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinnaka@iki.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