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