On 2016/02/22 20:13, Rushabh Lathia wrote:
I did another round of review for the latest patch and well as performed the sanity test, and haven't found any functional issues. Found couple of issue, see in-line comments for the same.
Thanks!
On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote: On 2016/02/12 21:19, Etsuro Fujita wrote:
+ /* Check point 1 */ + if (operation == CMD_INSERT) + return false; + + /* Check point 2 */ + if (nodeTag(subplan) != T_ForeignScan) + return false; + + /* Check point 3 */ + if (subplan->qual != NIL) + return false; + + /* Check point 4 */ + if (operation == CMD_UPDATE) These comments are referring to something in the function header further up, but you could instead just delete the stuff from the header and mention the actual conditions here.
Will fix.
Done. The patch doesn't allow the postgres_fdw to push down an UPDATE/DELETE on a foreign join, so I added one more condition here not to handle such cases. (I'm planning to propose a patch to handle such cases, in the next CF.)
I think we should place the checking foreign join condition before the target columns, as foreign join condition is less costly then the target columns.
Agreed.
Other changes:
* I keep Rushabh's code change that we call PlanDMLPushdown only when all the required APIs are available with FDW, but for CheckValidResultRel, I left the code as-is (no changes to that function), to match the docs saying that the FDW needs to provide the DML pushdown callback functions together with existing table-updating functions such as ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete.
I think we should also update the CheckValidResultRel(), because even though ExecForeignInsert, ExecForeignUpdate and ExecForeignDelete not present, FDW still can perform UPDATE/DELETE/INSERT using DML Pushdown APIs. Lets take committer's view on this.
OK
PFA update patch, which includes changes into postgresPlanDMLPushdown() to check for join condition before target columns and also fixed couple of whitespace issues.
Thanks again for updating the patch and fixing the issues! 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