Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On Thu, Dec 17, 2020 at 5:38 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com> wrote:
> > Discussion here is on the point - whether to show up the invalidated
> > connections in the output of the new postgres_fdw_get_connections()
> > function? If we were to show, then because of the solution we proposed for
> > the connection leak problem in [1], will the invalidated entries be shown
> > every time?
>
> IMO, we introduced the function postgres_fdw_get_connections to decide
> whether there are too many connections exists and we should disconnect them.
>
> If User decide to disconnect, we have two cases:
> 1. user decide to disconnect one of them,
> I think it’s ok for user to disconnect invalidated connection, so we'd better list the invalidated connections.
>
> 2. User decide to disconnect all of them. In this case,
> It seems postgres_fdw_disconnect will disconnect both invalidated and not connections,
> And we should let user realize what connections they are disconnecting, so we should list the invalidated connections.
>
> Based on the above two cases, Personlly, I think we can list the invalidated connections.

I will do that. So, the output will have a list of pairs like
(server_name, true/false), true/false is for valid/invalid connection.

> -----
> I took a look into the patch, and have a little issue:
>
> +bool disconnect_cached_connections(uint32 hashvalue, bool all)
> + if (all)
> + {
> + hash_destroy(ConnectionHash);
> + ConnectionHash = NULL;
> + result = true;
> + }
>
> If disconnect_cached_connections is called to disconnect all the connections,
> should we reset the 'xact_got_connection' flag ?

I think we must allow postgres_fdw_disconnect() to disconnect the
particular/all connections only when the corresponding entries have no
open txns or connections are not being used in that txn, otherwise
not. We may end up closing/disconnecting the connection that's still
being in use because entry->xact_dept can even go more than 1 for sub
txns. See use case [1].

+ if ((all || entry->server_hashvalue == hashvalue) &&
entry->xact_depth == 0 &&
+ entry->conn)
+ {
+ disconnect_pg_server(entry);
+ result = true;
+ }

Thoughts?

And to reset the 'xact_got_connection' flag: I think we should reset
it only when we close all the connections i.e. when all the
connections are at entry->xact_depth = 0, otherwise not. Same for
have_invalid_connections flag as well.

[1] -
BEGIN;
SELECT 1 FROM ft1 LIMIT 1; --> server 1 entry->xact_depth is 1
SAVEPOINT s;
SELECT 1 FROM ft1 LIMIT 1; --> entry->xact_depth becomes 2
SELECT postgres_fdw_disconnect()/postgres_fdw_disconnect('server 1');
--> I think we should not close the connection as it's txn is still
open.
COMMIT;

> > [1] -
> > https://www.postgresql.org/message-id/flat/CALj2ACVNcGH_6qLY-4_tXz8JLv
> > A%2B4yeBThRfxMz7Oxbk1aHcpQ%40mail.gmail.com
>
> The patch about connection leak looks good to me.
> And I have a same issue about the new 'have_invalid_connections' flag,
> If we disconnect all the connections, should we reset the flag ?

Yes as mentioned in the above comment.

Thanks for reviewing the connection leak patch. It will be good if the
review comments for the connection leak flag is provided separately in
that thread. I added it to commitfest -
https://commitfest.postgresql.org/31/2882/.

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