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.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@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