Re: Retry Cached Remote Connections for postgres_fdw in case remote backend gets killed/goes away

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