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
