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.kondra...@postgrespro.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
