Re: partition routing layering in nodeModifyTable.c

On Thu, Jul 18, 2019 at 4:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Jul 18, 2019 at 2:53 PM Andres Freund <andres@anarazel.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@anarazel.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