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.
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.
In other words, I have some doubts about allowing to call a
non-transactional by its nature function in the transaction block.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
