Re: partition routing layering in nodeModifyTable.c

On 13/10/2020 15:03, Amit Langote wrote:
> On Tue, Oct 13, 2020 at 7:13 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> Ok, committed. I'll continue to look at the rest of the patches in this
>> patch series now.

I've reviewed the next two patches in the series, they are pretty much 
ready for commit now. I made just a few minor changes, notably:

- I moved the responsibility to set ForeignTable->resultRelation to the 
FDWs, like you had in the original patch version. Sorry for 
flip-flopping on that.

- In postgres_fdw.c, I changed it to store the ResultRelInfo pointer in 
PgFdwDirectModifyState, instead of storing the RT index and looking it 
up in the BeginDirectModify and IterateDirectModify. I think you did it 
that way in the earlier patch versions, too.

- Some minor comment and docs kibitzing.

One little idea I had:

I think all FDWs that support direct modify will have to carry the 
resultRelaton index or the ResultRelInfo pointer from BeginDirectModify 
to IterateDirectModify in the FDW's private struct. It's not 
complicated, but should we make life easier for FDWs by storing the 
ResultRelInfo pointer in the ForeignScanState struct in the core code? 
The doc now says:

> The data that was actually inserted, updated or deleted must be
> stored in the ri_projectReturning->pi_exprContext->ecxt_scantuple of
> the target foreign table's ResultRelInfo obtained using the
> information passed to BeginDirectModify. Return NULL if no more rows
> are available.

That "ResultRelInfo obtained using the information passed to 
BeginDirectModify" part is a pretty vague. We could expand it, but if we 
stored the ResultRelInfo in the ForeignScanState, we could explain it 
succinctly.

> BTW, you mentioned the lazy ResultRelInfo optimization bit in the
> commit message, so does that mean you intend to take a look at the
> other thread [1] too?  Or should I post a rebased version of the lazy
> ResultRelInfo initialization patch here in this thread?  That patch is
> just a bunch of refactoring too.

No promises, but yeah, now that I'm knee-deep in this ResultRelInfo 
business, I'll try to take a look at that too :-).

- Heikki