Re: partition routing layering in nodeModifyTable.c

Hi,

On Thu, Aug 8, 2019 at 10:10 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Aug 7, 2019 at 6:00 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Wed, Aug 7, 2019 at 4:28 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > I just noticed obsolete references to es_result_relation_info that
> > > 0002 failed to remove.  One of them is in fdwhandler.sgml:
> > >
> > > <programlisting>
> > > TupleTableSlot *
> > > IterateDirectModify(ForeignScanState *node);
> > > </programlisting>
> > >
> > >     ... The data that was actually inserted, updated
> > >      or deleted must be stored in the
> > >      <literal>es_result_relation_info->ri_projectReturning->pi_exprContext->ecxt_scantuple</literal>
> > >      of the node's <structname>EState</structname>.
> > >
> > > We will need to rewrite this without mentioning
> > > es_result_relation_info.  How about as follows:
> > >
> > > -     <literal>es_result_relation_info->ri_projectReturning->pi_exprContext->ecxt_scantuple</literal>
> > > -     of the node's <structname>EState</structname>.
> > > +     <literal>ri_projectReturning->pi_exprContext->ecxt_scantuple</literal>
> > > +     of the result relation's<structname>ResultRelInfo</structname> that has
> > > +     been made available via node.
> > >
> > > I've updated 0001 with the above change.

> > This would be nitpicking, but:
> >
> > * IIUC, we don't use the term "result relation" in fdwhandler.sgml.
> > For consistency with your change to the doc for BeginDirectModify, how
> > about using the term "target foreign table" instead of "result
> > relation"?
>
> Agreed, done.
>
> > * ISTM that "<structname>ResultRelInfo</structname> that has been made
> > available via node" would be a bit fuzzy to FDW authors.  To be more
> > specific, how about changing it to
> > "<structname>ResultRelInfo</structname> passed to
> > <function>BeginDirectModify</function>" or something like that?
>
> That works for me, although an FDW author reading this still has got
> to make the connection.
>
> Attached updated patches; only 0001 changed in this version.

Thanks for the updated version, Amit-san!  I updated the 0001 patch a
bit further:

* Tweaked comments in plannodes.h, createplan.c, and nodeForeignscan.c.
* Made cosmetic changes to postgres_fdw.c.
* Adjusted doc changes a bit, mainly not to produce unnecessary diff.
* Modified the commit message.

Attached is an updated version of the 0001 patch.  Does that make sense?

Best regards,
Etsuro Fujita