Re: Optimization for updating foreign tables in Postgres FDW

On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
wrote:

> On 2016/01/25 17:03, Rushabh Lathia wrote:
>
>> Here are couple of comments:
>>
>
> 1)
>>
>> 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 ?

> 2)
>>
>> + /* SQL statement to execute remotely (as a String node) */
>> + FdwDmlPushdownPrivateUpdateSql,
>>
>> FdwDmlPushdownPrivateUpdateSql holds the UPDATE/DELETE query, so name
>> should
>> be something like FdwDmlPushdownPrivateQuery os FdwDmlPushdownPrivateSql ?
>>
>
> Later I realized that for FdwModifyPrivateIndex too the index name is
>> FdwModifyPrivateUpdateSql even though its holding any DML query. Not sure
>> whether we should consider to change this or not ?
>>
>
> To tell the truth, I imitated FdwModifyPrivateIndex when adding
> FdwDmlPushdownPrivateIndex, because I think "UpdateSql" means INSERT,
> UPDATE, or DELETE, not just UPDATE. (IsForeignRelUpdatable discussed above
> reports not only the updatability but the insertability and deletability of
> a foreign table!). So, +1 for leaving that as-is.
>
>
Make sense for now.

> Apart from this perform sanity testing on the new patch and things working
>> as expected.
>>
>
> Thanks for the review!
>
> Best regards,
> Etsuro Fujita
>
>
>

--
Rushabh Lathia