Re: Optimization for updating foreign tables in Postgres FDW

On Fri, Feb 12, 2016 at 5:40 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> Hi Rushabh and Thom,
>
> Thanks for the review!
>
> On 2016/02/10 22:37, Thom Brown wrote:
>
>> On 10 February 2016 at 08:00, Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
>> wrote:
>>
>>> Fujita-san, I am attaching update version of the patch, which added
>>> the documentation update.
>>>
>>
> Thanks for updating the patch!
>
> Once we finalize this, I feel good with the patch and think that we
>>> could mark this as ready for committer.
>>>
>>
> I find this wording a bit confusing:
>>
>> "If the IsForeignRelUpdatable pointer is set to NULL, foreign tables
>> are assumed to be insertable, updatable, or deletable either the FDW
>> provides ExecForeignInsert,ExecForeignUpdate, or ExecForeignDelete
>> respectively or if the FDW optimizes a foreign table update on a
>> foreign table using PlanDMLPushdown (PlanDMLPushdown still needs to
>> provide BeginDMLPushdown, IterateDMLPushdown and EndDMLPushdown to
>> execute the optimized update.)."
>>
>> This is a very long sentence, and the word "either" doesn't work here.
>>
>
> Agreed.
>
> As a result of our discussions, we reached a conclusion that the DML
> pushdown APIs should be provided together with existing APIs such as
> ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete, IIUC. So, how
> about (1) leaving the description for the existing APIs as-is and (2)
> adding a new description for the DML pushdown APIs in parenthesis, like
> this?:
>
> If the <function>IsForeignRelUpdatable</> pointer is set to
> <literal>NULL</>, foreign tables are assumed to be insertable,
> updatable,
> or deletable if the FDW provides <function>ExecForeignInsert</>,
> <function>ExecForeignUpdate</>, or <function>ExecForeignDelete</>
> respectively.
> (If the FDW attempts to optimize a foreign table update, it still
> needs to provide PlanDMLPushdown, BeginDMLPushdown,
> IterateDMLPushdown and EndDMLPushdown.)
>
> Actually, if the FDW provides the DML pushdown APIs, (pushdown-able)
> foreign table updates can be done without ExecForeignInsert,
> ExecForeignUpdate or ExecForeignDelete. So, the above docs are not
> necessarily correct. But we don't recommend to do that without the
> existing APIs, so I'm not sure it's worth complicating the docs.

Adding a new description for DML pushdown API seems good idea. I would
suggest to add that as separate paragraph rather then into brackets.

>

>
> Also:
>>
>> "When the query doesn't has the clause, the FDW must also increment
>> the row count for the ForeignScanState node in the EXPLAIN ANALYZE
>> case."
>>
>> Should read "doesn't have"
>>
>
> Will fix.
>
> Best regards,
> Etsuro Fujita
>
>
>

--
Rushabh Lathia