On Sun, Jan 17, 2021 at 11:30 PM Zhihong Yu <zyu@yugabyte.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.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com