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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
