On Wed, Nov 18, 2020 at 10:32 PM Alexey Kondratov <a.kondra...@postgrespro.ru> wrote: > > > 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. > > This approach looks solid enough from my perspective to give it a try. I > would only make it as three separate patches for an ease of further > review. >
Thanks! I will make separate patches and post them soon. > > >> 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. > > Yes, and this looks like a valid state for postgres_fdw and it can get > into the same state even without my patch. Next time GetConnection() > will find this cache entry, figure out that entry->conn is NULL and > establish a fresh connection. It is not clear for me right now, what > benefits we will get from clearing also this cache entry, except just > doing this for sanity. > By clearing the cache entry we will have 2 advantages: 1) we could save a(small) bit of memory 2) we could allow new connections to be cached, currently ConnectionHash can have only 8 entries. IMHO, along with disconnecting, we can also clear off the cache entry. Thoughts? > > > 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. > > > > Yes, they will. This could be solved with that additional disconnect > function as you proposed in c). > Right. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
