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

On Thu, Dec 17, 2020 at 10:32 PM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)ru> wrote:
> On 2020-12-17 18:02, Bharath Rupireddy wrote:
> > On Thu, Dec 17, 2020 at 5:38 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>
> > wrote:
> >> 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?
> >
>
> I think that you are right. Actually, I was thinking about much more
> simple solution to this problem --- just restrict
> postgres_fdw_disconnect() to run only *outside* of explicit transaction
> block. This should protect everyone from closing its underlying
> connections, but seems to be a bit more restrictive than you propose.

Agree that it's restrictive from a usability point of view. I think
having entry->xact_depth == 0 should be enough to protect from closing
any connections that are currently in use.

Say the user has called postgres_fdw_disconnect('myserver1'), if it's
currently in use in that xact, then we can return false or even go
further and issue a warning along with false. Also if
postgres_fdw_disconnect() is called for closing all connections and
any one of the connections are currently in use in the xact, then also
we can return: true and a warning if atleast one connection is closed
or false and a warning if all the connections are in use.

The warning message can be something like - for the first case -
"could not close the server connection as it is in use" and for the
second case - "could not close some of the connections as they are in
use".

Thoughts?

> Just thought, that if we start closing fdw connections in the open xact
> block:
>
> 1) Close a couple of them.
> 2) Found one with xact_depth > 0 and error out.
> 3) End up in the mixed state: some of connections were closed, but some
> them not, and it cannot be rolled back with the xact.

We don't error out, but we may issue a warning (if agreed on the above
reponse) and return false, but definitely not an error.

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