Re: Optimization for updating foreign tables in Postgres FDW

On 2016/01/27 12:20, Etsuro Fujita wrote:
> On 2016/01/26 22:57, Rushabh Lathia wrote:
>> On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita
>> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>> wrote:
>>
>> On 2016/01/25 17:03, Rushabh Lathia wrote:

>> int
>> IsForeignRelUpdatable (Relation rel);

>> Documentation for IsForeignUpdatable() need to change as it says:
>>
>> If the IsForeignRelUpdatable pointer is set to NULL, foreign
>> tables are
>> assumed
>> to be insertable, updatable, or deletable if the FDW provides
>> ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete respectively.
>>
>> With introduce of DMLPushdown API now this is no more correct,
>> as even if
>> FDW don't provide ExecForeignInsert, ExecForeignUpdate or
>> ExecForeignDelete API
>> still foreign tables are assumed to be updatable or deletable
>> with
>> DMLPushdown
>> API's, right ?

>> That's what I'd like to discuss.
>>
>> I intentionally leave that as-is, because I think we should
>> determine the updatability of a foreign table in the current
>> manner. As you pointed out, even if the FDW doesn't provide eg,
>> ExecForeignUpdate, an UPDATE on a foreign table could be done using
>> the DML pushdown APIs if the UPDATE is *pushdown-safe*. However,
>> since all UPDATEs on the foreign table are not necessarily
>> pushdown-safe, I'm not sure it's a good idea to assume the
>> table-level updatability if the FDW provides the DML pushdown
>> callback routines. To keep the existing updatability decision, I
>> think the FDW should provide the DML pushdown callback routines
>> together with ExecForeignInsert, ExecForeignUpdate, or
>> ExecForeignDelete. What do you think about that?

>> Sorry but I am not in favour of adding compulsion that FDW should provide
>> the DML pushdown callback routines together with existing
>> ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete APIs.
>>
>> May be we should change the documentation in such way, that explains
>>
>> a) If FDW PlanDMLPushdown is NULL, then check for ExecForeignInsert,
>> ExecForeignUpdate or ExecForeignDelete APIs
>> b) If FDW PlanDMLPushdown is non-NULL and plan is not pushable
>> check for ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete APIs
>> c) If FDW PlanDMLPushdown is non-NULL and plan is pushable
>> check for DMLPushdown APIs.
>>
>> Does this sounds wired ?

> Yeah, but I think that that would be what is done during executor
> startup (see CheckValidResultRel()), while what the documentation is
> saying is about relation_is_updatable(); that is, how to decide the
> updatability of a given foreign table, not how the executor processes an
> individual INSERT/UPDATE/DELETE on a updatable foreign table. So, I'm
> not sure it's a good idea to modify the documentation in such a way.

> However, I agree that we should add a documentation note about the
> compulsion somewhere. Maybe something like this:
>
> The FDW should provide DML pushdown callback routines together with
> table-updating callback routines described above. Even if the callback
> routines are provided, the updatability of a foreign table is determined
> based on the presence of ExecForeignInsert, ExecForeignUpdate or
> ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.

On second thought, I think it might be okay to assume the presence of
PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown, and
EndDMLPushdown is also sufficient for the insertablity, updatability,
and deletability of a foreign table, if the IsForeignRelUpdatable
pointer is set to NULL. How about modifying the documentation like this:

If the IsForeignRelUpdatable pointer is set to NULL, foreign tables are
assumed to be insertable, updatable, or deletable if the FDW provides
ExecForeignInsert, ExecForeignUpdate, or ExecForeignDelete respectively,
or if the FDW provides PlanDMLPushdown, BeginDMLPushdown,
IterateDMLPushdown, and EndDMLPushdown described below.

Of course, we also need to modify relation_is_updatable() accordingly.

What's your opinion?

Best regards,
Etsuro Fujita