On Wed, Jan 20, 2021 at 11:53 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> So, furthermore, we can use hash_search() to find the target cached
> connection, instead of using hash_seq_search(), when the server name
> is given. This would simplify the code a bit more? Of course,
> hash_seq_search() is necessary when closing all the connections, though.
Note that the cache entry key is user mapping oid and to use
hash_search() we need the user mapping oid. But in
postgres_fdw_disconnect we can get server oid and we can also get user
mapping id using GetUserMapping, but it requires
GetUserId()/CurrentUserId as an input, I doubt we will have problems
if CurrentUserId is changed somehow with the change of current user in
the session. And user mapping may be dropped but still the connection
can exist if it's in use, in that case GetUserMapping fails in cache
lookup.
And yes, disconnecting all connections requires hash_seq_search().
Keeping above in mind, I feel we can do hash_seq_search(), as we do
currently, even when the server name is given as input. This way, we
don't need to bother much on the above points.
Thoughts?
> + * 2) If no input argument is provided, then it tries to disconnect all the
> + * connections.
>
> I'm concerned that users can easily forget to specify the argument and
> accidentally discard all the connections. So, IMO, to alleviate this situation,
> what about changing the function name (only when closing all the connections)
> to something postgres_fdw_disconnect_all(), like we have
> pg_advisory_unlock_all() against pg_advisory_unlock()?
+1. We will have two functions postgres_fdw_disconnect(server name),
postgres_fdw_disconnect_all.
> + if (result)
> + {
> + /* We closed at least one connection, others are in use. */
> + ereport(WARNING,
> + (errmsg("cannot close all connections because some of them are still
inuse")));
> + }
>
> Sorry if this was already discussed upthread. Isn't it more helpful to
> emit a warning for every connections that fail to be closed? For example,
>
> WARNING: cannot close connection for server "loopback1" because it is still in use
> WARNING: cannot close connection for server "loopback2" because it is still in use
> WARNING: cannot close connection for server "loopback3" because it is still in use
> ...
>
> This enables us to identify easily which server connections cannot be
> closed for now.
+1. Looks like pg_advisory_unlock is doing that. Given the fact that
still in use connections are possible only in explicit txns, we might
not have many still in use connections in the real world use case, so
I'm okay to change that way.
I will address all these comments and post an updated patch set soon.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
