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

On 2020-12-17 18:02, Bharath Rupireddy wrote:
> On Thu, Dec 17, 2020 at 5:38 PM Hou, Zhijie <houzj.fnst@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