Re: Optimization for updating foreign tables in Postgres FDW

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?

> 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.

> Apart from this perform sanity testing on the new patch and things working
> as expected.

Thanks for the review!

Best regards,
Etsuro Fujita