Thanks for the interest shown! On Wed, Nov 18, 2020 at 1:07 AM Alexey Kondratov <a.kondra...@postgrespro.ru> wrote: > > I had a look on the initial patch and discussed options [1] to proceed > with this issue. I agree with Bruce about idle_session_timeout, it would > be a nice to have in-core feature on its own. However, this should be a > cluster-wide option and it will start dropping all idle connection not > only foreign ones. So it may be not an option for some cases, when the > same foreign server is used for another load as well. >
With idle_session_timeout the remote idle backends may go away, part of our problem is solved. But we also need to clear that connection entry from the local backend's connection cache. > > Regarding the initial issue I prefer point #3, i.e. foreign server > option. It has a couple of benefits IMO: 1) it may be set separately on > per foreign server basis, 2) it will live only in the postgres_fdw > contrib without any need to touch core. I would only supplement this > postgres_fdw foreign server option with a GUC, e.g. > postgres_fdw.keep_connections, so one could easily define such behavior > for all foreign servers at once or override server-level option by > setting this GUC on per session basis. > Below is what I have in my mind, mostly inline with yours: a) Have a server level option (keep_connetion true/false, with the default being true), when set to false the connection that's made with this foreign server is closed and cached entry from the connection cache is deleted at the end of txn in pgfdw_xact_callback. b) Have postgres_fdw level GUC postgres_fdw.keep_connections default being true. When set to false by the user, the connections, that are used after this, are closed and removed from the cache at the end of respective txns. If we don't use a connection that was cached prior to the user setting the GUC as false, then we may not be able to clear it. We can avoid this problem by recommending users either to set the GUC to false right after the CREATE EXTENSION postgres_fdw; or else use the function specified in (c). c) Have a new function that gets defined as part of CREATE EXTENSION postgres_fdw;, say postgres_fdw_discard_connections(), similar to dblink's dblink_disconnect(), which discards all the remote connections and clears connection cache. And we can also have server name as input to postgres_fdw_discard_connections() to discard selectively. Thoughts? If okay with the approach, I will start working on the patch. > > Attached is a small POC patch, which implements this contrib-level > postgres_fdw.keep_connections GUC. What do you think? > I see two problems with your patch: 1) It just disconnects the remote connection at the end of txn if the GUC is set to false, but it doesn't remove the connection cache entry from ConnectionHash. 2) What happens if there are some cached connections, user set the GUC to false and not run any foreign queries or not use those connections thereafter, so only the new connections will not be cached? Will the existing unused connections still remain in the connection cache? See (b) above for a solution. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
