Re: Optimization for updating foreign tables in Postgres FDW

On 2016/04/19 12:45, Etsuro Fujita wrote:
> On 2016/04/19 12:26, Michael Paquier wrote:
>> Note for Robert: pgfdw_get_result copycats PQexec by discarding all
>> PQresult received except the last one. I think that's fine for the
>> purposes of postgres_fdw, but perhaps you have a different opinion on
>> the matter.

> That seemed reasonable to me, but sorry, on second thought, I'm not sure
> that's still a good idea.  One reason is (1) I think it's better for the
> in-postgres_fdw.c functions using pgfdw_get_result to verify that there
> are no more results, in itself.  I think that would improve the
> robustness of those functions.  Another reason is I don't think
> pgfdw_report_error, which is used in pgfdw_get_result, works well for
> cases where the query contains multiple SQL commands.  So, +1 for the
> idea of simply encapsulating the while (PQisBusy(...)) loop into a new
> function pgfdw_get_result().

Here is a proposed patch for that.

Other changes:

       * We don't use a PG_TRY block here, so be careful not to throw error
       * without releasing the PGresult.
       */
-    res = PQexecPrepared(fmstate->conn,
-                         fmstate->p_name,
-                         fmstate->p_nums,
-                         p_values,
-                         NULL,
-                         NULL,
-                         0);
+    if (!PQsendQueryPrepared(fmstate->conn,
+                             fmstate->p_name,
+                             fmstate->p_nums,
+                             p_values,
+                             NULL,
+                             NULL,
+                             0))
+        pgfdw_report_error(ERROR, NULL, fmstate->conn, true, fmstate->query);

The comment "We don't use a PG_TRY block here ..." seems to be wrongly
placed, so I moved that comment.  Also, I think it'd be better to call
pgfdw_report_error() with the clear argument set to false, not true,
since we don't need to clear the PGresult.  Same for
postgresExecForeignUpdate, postgresExecForeignUpdate, and
prepare_foreign_modify.

What do you think about that?

Best regards,
Etsuro Fujita