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

Thanks for the interest shown!

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