On 2016/01/21 5:06, Robert Haas wrote:
> On Wed, Jan 20, 2016 at 4:50 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> My concern about that is that would make the code in deparseTargetList()
>> complicated.
>>
>> Essentially, I think your propossal needs a two-pass algorithm for
>> deparseTargetList; (1) create an integer List of the columns being retrieved
>> from the given attrs_used (getRetrievedAttrs()), and (2) print those columns
>> (printRetrievedAttrs()). How about sharing those two functions between
>> deparseTargetList and deparseReturningList?:
> I don't see why we'd need that. I adjusted the code in postgres_fdw
> along the lines I had in mind and am attaching the result. It doesn't
> look complicated to me, and it passes the regression test you wrote.
Thanks for the patch! From the patch, I correctly understand what you
proposed. Good idea!
> By the way, I'm not too sure I understand the need for the core
> changes that are part of this patch, and I think that point merits
> some discussion. Whenever you change core like this, you're changing
> the contract between the FDW and core; it's not just postgres_fdw that
> needs updating, but every FDW. So we'd better be pretty sure we need
> these changes and they are adequately justified before we think about
> putting them into the tree. Are these core changes really needed
> here, or can we fix this whole issue in postgres_fdw and leave the
> core code alone?
Well, if we think it is the FDW's responsibility to insert a valid value
for tableoid in the returned slot during ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete, we don't need those core
changes. However, I think it would be better that that's done by
ModifyTable in the same way as ForeignScan does in ForeignNext, IMO.
That eliminates the need for postgres_fdw or any other FDW to do that
business in the callback routines.
Best regards,
Etsuro Fujita
