Re: Optimization for updating foreign tables in Postgres FDW

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