On 2020/09/25 21:19, Bharath Rupireddy wrote:
> On Fri, Sep 25, 2020 at 3:21 PM Fujii Masao <masao.fujii@oss.nttdata.com <mailto:masao.fujii@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
debug3message 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
