On Fri, Sep 25, 2020 at 3:21 PM Fujii Masao <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.
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

Attachment: v5-Retry-Cached-Remote-Connections-For-postgres_fdw.patch
Description: Binary data

Reply via email to