Re: partition routing layering in nodeModifyTable.c

On Mon, Oct 12, 2020 at 8:12 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 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:

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