Re: Optimization for updating foreign tables in Postgres FDW

On Mon, Apr 11, 2016 at 11:30 AM, Etsuro Fujita
<fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I agree with Rushabh. Thanks for updating the patch!

Yes, not using a PG_TRY/CATCH block is better. We are not doing
anything that need to clean up PGresult in case of an unplanned
failure.

> Another thing I'd like to propose to revise the patch is to call
> pgfdw_report_error in the newly added hunk, with the clear argument set to
> *false*. The PGresult argument is NULL there, so no need to release the
> PGresult.

Sure, this saves a couple of cycles. PQclear is anyway smart enough to
handle NULL results correctly, but this way is better.

> Attached is an updated patch.

+ if (wc & WL_SOCKET_READABLE)
+ {
+ if (!PQconsumeInput(dmstate->conn))
+ pgfdw_report_error(ERROR, NULL, dmstate->conn, false,
+ dmstate->query);
+ }
OK, so here we would fail with ERRCODE_CONNECTION_FAILURE in the case
where the socket is readable but no data can be consumed. I guess it
makes sense.

+ if ((cancel = PQgetCancel(entry->conn)))
+ {
+ PQcancel(cancel, errbuf, sizeof(errbuf));
+ PQfreeCancel(cancel);
+ }
Wouldn't it be better to issue a WARNING here if PQcancel does not succeed?
--
Michael