On Fri, Feb 12, 2016 at 5:40 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
> 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.lat...@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. Adding a new description for DML pushdown API seems good idea. I would suggest to add that as separate paragraph rather then into brackets. > > > 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 > > > -- Rushabh Lathia
