Re: partition routing layering in nodeModifyTable.c

On Thu, Jul 18, 2019 at 4:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Jul 18, 2019 at 2:53 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2019-07-18 14:24:29 +0900, Amit Langote wrote:
> > > On Thu, Jul 18, 2019 at 10:09 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > 1) How come partition routing is done outside of ExecInsert()?
> > > >
> > > > case CMD_INSERT:
> > > > /* Prepare for tuple routing if needed. */
> > > > if (proute)
> > > > slot = ExecPrepareTupleRouting(node, estate, proute,
> > > > resultRelInfo, slot);
> > > > slot = ExecInsert(node, slot, planSlot,
> > > > estate, node->canSetTag);
> > > > /* Revert ExecPrepareTupleRouting's state change. */
> > > > if (proute)
> > > > estate->es_result_relation_info = resultRelInfo;
> > > > break;
> > > >
> > > > That already seems like a layering violation,
> > >
> > > The decision to move partition routing out of ExecInsert() came about
> > > when we encountered a bug [1] whereby ExecInsert() would fail to reset
> > > estate->es_result_relation_info back to the root table if it had to
> > > take an abnormal path out (early return), of which there are quite a
> > > few instances. The first solution I came up with was to add a goto
> > > label for the code to reset estate->es_result_relation_info and jump
> > > to it from the various places that do an early return, which was
> > > complained about as reducing readability. So, the solution we
> > > eventually settled on in 6666ee49f was to perform ResultRelInfos
> > > switching at a higher level.
> >
> > I think that was the wrong path, given that the code now lives in
> > multiple places. Without even a comment explaining that if one has to be
> > changed, the other has to be changed too.

I thought this would be OK because we have the
ExecPrepareTupleRouting() call in just two places in a single source
file, at least currently.

> > Or perhaps the actually correct fix is to remove es_result_relation_info
> > alltogether, and just pass it down the places that need it - we've a lot
> > more code setting it than using the value. And it'd not be hard to
> > actually pass it to the places that read it. Given all the
> > setting/resetting of it it's pretty obvious that a query-global resource
> > isn't the right place for it.
>
> I tend to agree that managing state through es_result_relation_info
> across various operations on a result relation has turned a bit messy
> at this point. That said, while most of the places that access the
> currently active result relation from es_result_relation_info can be
> easily modified to receive it directly, the FDW API BeginDirectModify
> poses bit of a challenge. BeginDirectlyModify() is called via
> ExecInitForeignScan() that in turn can't be changed to add a result
> relation (Index or ResultRelInfo *) argument, so the only way left for
> BeginDirectlyModify() is to access it via es_result_relation_info.

That's right. I'm not sure that's a good idea, because I think other
extensions also might look at es_result_relation_info, and if so,
removing es_result_relation_info altogether would require the
extension authors to update their extensions without any benefit,
which I think isn't a good thing.

Best regards,
Etsuro Fujita