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

Reply via email to