On 2020/09/25 21:19, Bharath Rupireddy wrote:
On Fri, Sep 25, 2020 at 3:21 PM Fujii Masao <masao.fu...@oss.nttdata.com <mailto:masao.fu...@oss.nttdata.com>> wrote: > > I think that we can simplify the code by merging the connection-retry > code into them, like the attached very WIP patch (based on yours) does. > +1. > > + else > + ereport(ERROR, > + (errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION), > + errmsg("could not connect to server \"%s\"", > + server->servername), > + errdetail_internal("%s", pchomp(PQerrorMessage(entry->conn))))); > > The above is not necessary? If this error occurs, connect_pg_server() > reports an error, before the above code is reached. Right? > Removed. Thanks for the comments. I think we need to have a volatile qualifier for need_reset_conn, because of the sigsetjmp.
Yes.
Instead of "need_reset_conn", "retry_conn" would be more meaningful and also instead of goto label name "reset;", "retry:".
Sounds good.
I changed "closing connection %p to reestablish connection" to "closing connection %p to reestablish a new one"
OK.
I also adjusted the comments to be under the 80char limit. I feel, when we are about to close an existing connection and reestablish a new connection, it will be good to have a debug3 message saying that we "could not connect to postgres_fdw connection %p"[1].
+1 to add debug3 message there. But this message doesn't seem to match with what the error actually happened. What about something like "could not start remote transaction on connection %p", instead? Also maybe it's better to append PQerrorMessage(entry->conn)?
Attaching v5 patch that has the above changes. Both make check and make check-world regression tests passes. Please review.
Thanks for updating the patch! +-- Generate a connection to remote. Local backend will cache it. +SELECT * FROM ft1 LIMIT 1; The result of this query would not be stable. Probably you need to, for example, add ORDER BY or replace * with 1, etc. +-- Retrieve pid of remote backend with application name fdw_retry_check +-- and kill it intentionally here. Note that, local backend still has +-- the remote connection/backend info in it's cache. +SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE +backend_type = 'client backend' AND application_name = 'fdw_retry_check'; Isn't this test fragile because there is no gurantee that the target backend has exited just after calling pg_terminate_backend()? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
