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

On 2021/01/18 12:33, Bharath Rupireddy wrote:
> On Sun, Jan 17, 2021 at 11:30 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>> This patch introduces new function postgres_fdw_disconnect() when
>> called with a foreign server name discards the associated
>> connections with the server name.
>>
>> I think the following would read better:
>>
>> This patch introduces a new function postgres_fdw_disconnect(). When
>> called with a foreign server name, it discards the associated
>> connections with the server.
>
> Thanks. I corrected the commit message.
>
>> Please note the removal of the 'name' at the end - connection is with server, not server name.
>>
>> + if (is_in_use)
>> + ereport(WARNING,
>> + (errmsg("cannot close the connection because it is still in use")));
>>
>> It would be better to include servername in the message.
>
> User would have provided the servername in
> postgres_fdw_disconnect('myserver'), I don't think we need to emit the
> warning again with the servername. The existing warning seems fine.
>
>> + ereport(WARNING,
>> + (errmsg("cannot close all connections because some of them are still in use")));
>>
>> I think showing the number of active connections would be more informative.
>> This can be achieved by changing active_conn_exists from bool to int (named active_conns, e.g.):
>>
>> + if (entry->conn && !active_conn_exists)
>> + active_conn_exists = true;
>>
>> Instead of setting the bool value, active_conns can be incremented.
>
> IMO, the number of active connections is not informative, because
> users can not do anything with them. What's actually more informative
> would be to list all the server names for which the connections are
> active, instead of the warning - "cannot close all connections because
> some of them are still in use". Having said that, I feel like it's an
> overkill for now to do that. If required, we can enhance the warnings
> in future. Thoughts?
>
> Attaching v11 patch set, with changes only in 0001. The changes are
> commit message correction and moved the warning related code to
> disconnect_cached_connections from postgres_fdw_disconnect.
>
> Please review v11 further.

Thanks for updating the patch!

The patch for postgres_fdw_get_connections() basically looks good to me.
So at first I'd like to push it. Attached is the patch that I extracted
postgres_fdw_get_connections() part from 0001 patch and tweaked.
Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION