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