On Mon, Oct 12, 2020 at 8:12 PM Heikki Linnakangas <hlinn...@iki.fi> wrote: > On 09/10/2020 11:01, Amit Langote wrote: > > On Thu, Oct 8, 2020 at 9:35 PM Amit Langote <amitlangot...@gmail.com> wrote: > >> On Wed, Oct 7, 2020 at 9:07 PM Heikki Linnakangas <hlinn...@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=kphrdtfs...@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:
Thanks for checking. > 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(). Also, maybe call the other functions: ExecInitPlanResultRelationsArray() ExecInitPlanResultRelation() ExecClosePlanResultRelations() Thoughts? > 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? Yeah, that did bother me too a bit. I'm okay either way but it does look a bit inconsistent. 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. > 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. Thanks for the cleanup. I had noticed there was some funny capitalization in my patch: + ResultRelInfo **es_result_relations; /* Array of Per-range-table-entry s/Per-/per- Also, I think a comma may be needed in the parenthetical below: + * can index it by the RT index (minus 1 to be accurate). ...(minus 1, to be accurate) -- Amit Langote EDB: http://www.enterprisedb.com
