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

On Wed, Nov 18, 2020 at 10:32 PM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)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