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(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.

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