From: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Retry Cached Remote Connections for postgres_fdw in case remote backend gets killed/goes away |
Date: | 2020-09-29 14:00:50 |
Message-ID: | 41d5880b-0c5d-8029-3c50-4cdf709d86b6@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Alvaro Herrera | 2020-09-29 14:09:44 | Re: [PATCH] We install pg_regress and isolationtester but not pg_isolation_regress |
Previous Message | Alvaro Herrera | 2020-09-29 13:30:33 | Re: Optimize memory allocation code |