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

On 2020-11-18 16:39, Bharath Rupireddy wrote:
> Thanks for the interest shown!
>
> On Wed, Nov 18, 2020 at 1:07 AM Alexey Kondratov
> <a(dot)kondratov(at)postgrespro(dot)ru> wrote:
>>
>> 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.
>

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.

>>
>> 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.

> 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).

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company