Hi Rushabh and Thom,
Thanks for the review!
On 2016/02/10 22:37, Thom Brown wrote:
> On 10 February 2016 at 08:00, Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>> Fujita-san, I am attaching update version of the patch, which added
>> the documentation update.
Thanks for updating the patch!
>> Once we finalize this, I feel good with the patch and think that we
>> could mark this as ready for committer.
> I find this wording a bit confusing:
>
> "If the IsForeignRelUpdatable pointer is set to NULL, foreign tables
> are assumed to be insertable, updatable, or deletable either the FDW
> provides ExecForeignInsert,ExecForeignUpdate, or ExecForeignDelete
> respectively or if the FDW optimizes a foreign table update on a
> foreign table using PlanDMLPushdown (PlanDMLPushdown still needs to
> provide BeginDMLPushdown, IterateDMLPushdown and EndDMLPushdown to
> execute the optimized update.)."
>
> This is a very long sentence, and the word "either" doesn't work here.
Agreed.
As a result of our discussions, we reached a conclusion that the DML
pushdown APIs should be provided together with existing APIs such as
ExecForeignInsert, ExecForeignUpdate or ExecForeignDelete, IIUC. So,
how about (1) leaving the description for the existing APIs as-is and
(2) adding a new description for the DML pushdown APIs in parenthesis,
like this?:
If the <function>IsForeignRelUpdatable</> pointer is set to <literal>NULL</>, foreign tables are assumed to be
insertable,
updatable, or deletable if the FDW provides <function>ExecForeignInsert</>, <function>ExecForeignUpdate</>, or
<function>ExecForeignDelete</> respectively. (If the FDW attempts to optimize a foreign table update, it still
needs to provide PlanDMLPushdown, BeginDMLPushdown, IterateDMLPushdown and EndDMLPushdown.)
Actually, if the FDW provides the DML pushdown APIs, (pushdown-able)
foreign table updates can be done without ExecForeignInsert,
ExecForeignUpdate or ExecForeignDelete. So, the above docs are not
necessarily correct. But we don't recommend to do that without the
existing APIs, so I'm not sure it's worth complicating the docs.
> Also:
>
> "When the query doesn't has the clause, the FDW must also increment
> the row count for the ForeignScanState node in the EXPLAIN ANALYZE
> case."
>
> Should read "doesn't have"
Will fix.
Best regards,
Etsuro Fujita
