On Thu, Dec 17, 2020 at 10:32 PM Alexey Kondratov <a.kondra...@postgrespro.ru> wrote: > On 2020-12-17 18:02, Bharath Rupireddy wrote: > > On Thu, Dec 17, 2020 at 5:38 PM Hou, Zhijie <houzj.f...@cn.fujitsu.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