On 2016/01/26 22:57, Rushabh Lathia wrote:
On Tue, Jan 26, 2016 at 4:15 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
On 2016/01/25 17:03, Rushabh Lathia wrote:
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 ?
Yeah, but I think that that would be what is done during executor
startup (see CheckValidResultRel()), while what the documentation is
saying is about relation_is_updatable(); that is, how to decide the
updatability of a given foreign table, not how the executor processes an
individual INSERT/UPDATE/DELETE on a updatable foreign table. So, I'm
not sure it's a good idea to modify the documentation in such a way.
BTW, I have added the description about that check partially. I added
to the PlanDMLPushdown documentation:
+ If this function succeeds, <function>PlanForeignModify</>
+ won't be executed, and <function>BeginDMLPushdown</>,
+ <function>IterateDMLPushdown</> and <function>EndDMLPushdown</> will
+ be called at the execution stage, instead.
And for example, I added to the BeginDMLPushdown documentation:
+ If the <function>BeginDMLPushdown</> pointer is set to
+ <literal>NULL</>, attempts to execute the table update directly on
+ the remote server will fail with an error message.
However, I agree that we should add a documentation note about the
compulsion somewhere. Maybe something like this:
The FDW should provide DML pushdown callback routines together with
table-updating callback routines described above. Even if the callback
routines are provided, the updatability of a foreign table is determined
based on the presence of ExecForeignInsert, ExecForeignUpdate or
ExecForeignDelete if the IsForeignRelUpdatable pointer is set to NULL.
What's your opinion?
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
