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

On Fri, Sep 25, 2020 at 3:21 PM Fujii Masao <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.
Instead of "need_reset_conn", "retry_conn" would be more meaningful and
also instead of goto label name "reset;", "retry:".
I changed "closing connection %p to reestablish connection" to "closing
connection %p to reestablish a new one"
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].

Attaching v5 patch that has the above changes. Both make check and make
check-world regression tests passes. Please review.

[1] This would tell the user that we are not able to connect to the
connection.
postgres=# SELECT * FROM foreign_tbl;
DEBUG: starting remote transaction on connection 0x55ab0e416830
DEBUG: could not connect to postgres_fdw connection 0x55ab0e416830
DEBUG: closing connection 0x55ab0e416830 to reestablish a new one
DEBUG: new postgres_fdw connection 0x55ab0e416830 for server
"foreign_server" (user mapping oid 16407, userid 10)
DEBUG: starting remote transaction on connection 0x55ab0e416830
DEBUG: closing remote transaction on connection 0x55ab0e416830
a1 | b1
-----+-----
100 | 200

Without the above message, it would look like we are starting remote txn,
and closing connection without any reason.

postgres=# SELECT * FROM foreign_tbl;
DEBUG: starting remote transaction on connection 0x55ab0e4c0d50
DEBUG: closing connection 0x55ab0e4c0d50 to reestablish a new one
DEBUG: new postgres_fdw connection 0x55ab0e4c0d50 for server
"foreign_server" (user mapping oid 16389, userid 10)
DEBUG: starting remote transaction on connection 0x55ab0e4c0d50
DEBUG: closing remote transaction on connection 0x55ab0e4c0d50
a1 | b1
-----+-----
100 | 200

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com