Re: Optimization for updating foreign tables in Postgres FDW - Mailing list pgsql-hackers

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