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
<[email protected] <mailto:[email protected]>> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
