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

On Mon, Jun 22, 2020 at 11:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

When a query on foreign table is executed from a local session using
postgres_fdw, as expected the local postgres backend opens a
connection which causes a remote session/backend to be opened on the
remote postgres server for query execution.

One observation is that, even after the query is finished, the remote
session/backend still persists on the remote postgres server. Upon
researching, I found that there is a concept of Connection Caching for
the remote connections made using postgres_fdw. Local backend/session
can cache up to 8 different connections per backend. This caching is
useful as it avoids the cost of reestablishing new connections per
foreign query.

However, at times, there may be situations where the long lasting
local sessions may execute very few foreign queries and remaining all
are local queries, in this scenario, the remote sessions opened by the
local sessions/backends may not be useful as they remain idle and eat
up the remote server connections capacity. This problem gets even
worse(though this use case is a bit imaginary) if all of
max_connections(default 100 and each backend caching 8 remote
connections) local sessions open remote sessions and they are cached
in the local backend.

I propose to have a new session level GUC called
"enable_connectioncache"(name can be changed if it doesn't correctly
mean the purpose) with the default value being true which means that
all the remote connections are cached. If set to false, the
connections are not cached and so are remote sessions closed by the local backend/session at
the end of each remote transaction.

Attached the initial patch(based on commit
9550ea3027aa4f290c998afd8836a927df40b09d), test setup.

Few comments:

 #backend_flush_after = 0               # measured in pages, 0 disables
-
+#enable_connectioncache = on
This guc could be  placed in CONNECTIONS AND AUTHENTICATION section.

+
+       /* see if the cache was for postgres_fdw connections and
+          user chose to disable connection caching*/
+       if ((strcmp(hashp->tabname,"postgres_fdw connections") == 0) &&
+               !enable_connectioncache)

Should be changed to postgres style commenting like:
/*
* See if the cache was for postgres_fdw connections and
* user chose to disable connection caching.
*/

+       /* if true, fdw connections in a session are cached, else
+          discarded at the end of every remote transaction.
+       */
+       bool        enableconncache;
Should be changed to postgres style commenting.

+/* parameter for enabling fdw connection hashing */
+bool   enable_connectioncache = true;
+

Should this be connection caching?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Mon, Jun 22, 2020 at 11:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Attached the initial patch(based on commit
9550ea3027aa4f290c998afd8836a927df40b09d), test setup.

make check is failing
sysviews.out     2020-06-27 07:22:32.162146320 +0530
@@ -73,6 +73,7 @@
               name              | setting
 --------------------------------+---------
  enable_bitmapscan              | on
+ enable_connectioncache         | on

one of the test expect files needs to be updated.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Sun, Jun 21, 2020 at 10:56 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

When a query on foreign table is executed from a local session using
postgres_fdw, as expected the local postgres backend opens a
connection which causes a remote session/backend to be opened on the
remote postgres server for query execution.

[...]

I propose to have a new session level GUC called
"enable_connectioncache"(name can be changed if it doesn't correctly
mean the purpose) with the default value being true which means that
all the remote connections are cached. If set to false, the
connections are not cached and so are remote sessions closed by the local
backend/session at
the end of each remote transaction.

[...]

Thoughts?

Test Case:
without patch:
1. Run the query on foreign table
2. Look for the backend/session opened on the remote postgres server, it
exists till the local session remains active.

with patch:
1. SET enable_connectioncache TO false;
2. Run the query on the foreign table
3. Look for the backend/session opened on the remote postgres server, it
should not exist.

If this is just going to apply to postgres_fdw why not just have that
module provide a function "disconnect_open_sessions()" or the like that
does this upon user command? I suppose there would be some potential value
to having this be set per-user but that wouldn't preclude the usefulness of
a function. And by having a function the usefulness of the GUC seems
reduced. On a related note is there any entanglement here with the
supplied dblink and/or dblink_fdw [1]The only place I see "dblink_fdw" in the documentation is in the dblink module's dblink_connect page. I would probably modify that page to say: "It is recommended to use the foreign-data wrapper dblink_fdw (installed by this module) when defining the foreign server." (adding the parenthetical). modules as they do provide connect
and disconnect functions and also leverages postgres_fdw (or dblink_fdw if
specified, which brings us back to the question of whether this option
should be respected by that FDW).

Otherwise, I would imagine that having multiple queries execute before
wanting to drop the connection would be desirable so at minimum a test case
that does something like:

SELECT count(*) FROM remote.tbl1;
-- connection still open
SET enable_connectioncache TO false;
SELECT count(*) FROM remote.tbl2;
-- now it was closed

Or maybe even better, have the close action happen on a transaction
boundary.

And if it doesn't just apply to postgres_fdw (or at least doesn't have to)
then the description text should be less specific.

David J.

[1]: The only place I see "dblink_fdw" in the documentation is in the dblink module's dblink_connect page. I would probably modify that page to say: "It is recommended to use the foreign-data wrapper dblink_fdw (installed by this module) when defining the foreign server." (adding the parenthetical).
module's dblink_connect page. I would probably modify that page to say:
"It is recommended to use the foreign-data wrapper dblink_fdw (installed by
this module) when defining the foreign server." (adding the parenthetical).

Thanks for the responses.

If this is just going to apply to postgres_fdw why not just have that module provide a function "disconnect_open_sessions()" or the like that does this upon user command? I suppose there would be some potential value to having this be set per-user but that wouldn't preclude the usefulness of a function. And by having a function the usefulness of the GUC seems reduced.

The idea of having module-specific functions to remove cached entries
seems like a good idea. Users have to frequently call these functions
to clean up the cached entries in a long lasting single session. This
may not
be always possible if these sessions are from an application not from
a psql-like client which is a more frequent scenario in the customer
use cases. In this case users might have to change their application
code that is
issuing queries to postgres server to include these functions.

Assuming the fact that the server/session configuration happens much
before the user application starts to submit actual database queries,
having a GUC just helps to avoid making such function calls in between
the session, by having to set the GUC either to true if required to
cache connections or to off if not to cache connections.

On a related note is there any entanglement here with the supplied dblink and/or dblink_fdw [1] modules as they do provide connect and disconnect functions and also leverages postgres_fdw (or dblink_fdw if specified, which brings us back to the question of whether this option should be respected by that FDW).

I found that dblink also has the connection caching concept and it
does provide a user a function to disconnect/remove cached connections
using a function, dblink_disconnect() using connection name as it's
input.
IMO, this solution seems a bit problematic as explained in my first
response in this mail.
The postgres_fdw connection caching and dblink connection caching has
no link at all. Please point me if I'm missing anything here.
But probably, this GUC can be extended from a bool to an enum of type
config_enum_entry and use it for dblink as well. This is extensible as
well. Please let me know if this is okay, so that I can code for it.

Otherwise, I would imagine that having multiple queries execute before wanting to drop the connection would be desirable so at minimum a test case that does something like:

SELECT count(*) FROM remote.tbl1;
-- connection still open
SET enable_connectioncache TO false;
SELECT count(*) FROM remote.tbl2;
-- now it was closed

Or maybe even better, have the close action happen on a transaction boundary.

This is a valid scenario, as the same connection can be used in the
same transaction multiple times. With my attached initial patch above
the point is already covered. The decision to cache or not cache the
connection happens at the main transaction end i.e. in
pgfdw_xact_callback().

And if it doesn't just apply to postgres_fdw (or at least doesn't have to) then the description text should be less specific.

If we are agreed on a generic GUC for postgres_fdw, dblink and so on.
I will change the description and documentation accordingly.

Thoughts?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, 22 Jun 2020 at 14:56, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

When a query on foreign table is executed from a local session using
postgres_fdw, as expected the local postgres backend opens a
connection which causes a remote session/backend to be opened on the
remote postgres server for query execution.

One observation is that, even after the query is finished, the remote
session/backend still persists on the remote postgres server. Upon
researching, I found that there is a concept of Connection Caching for
the remote connections made using postgres_fdw. Local backend/session
can cache up to 8 different connections per backend. This caching is
useful as it avoids the cost of reestablishing new connections per
foreign query.

However, at times, there may be situations where the long lasting
local sessions may execute very few foreign queries and remaining all
are local queries, in this scenario, the remote sessions opened by the
local sessions/backends may not be useful as they remain idle and eat
up the remote server connections capacity. This problem gets even
worse(though this use case is a bit imaginary) if all of
max_connections(default 100 and each backend caching 8 remote
connections) local sessions open remote sessions and they are cached
in the local backend.

I propose to have a new session level GUC called
"enable_connectioncache"(name can be changed if it doesn't correctly
mean the purpose) with the default value being true which means that
all the remote connections are cached. If set to false, the
connections are not cached and so are remote sessions closed by the local backend/session at
the end of each remote transaction.

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 30, 2020 at 12:23:28PM +0900, Masahiko Sawada wrote:

I propose to have a new session level GUC called
"enable_connectioncache"(name can be changed if it doesn't correctly
mean the purpose) with the default value being true which means that
all the remote connections are cached. If set to false, the
connections are not cached and so are remote sessions closed by the local backend/session at
the end of each remote transaction.

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

I thought we would add a core capability, idle_session_timeout, which
would disconnect idle sessions, and the postgres_fdw would use that. We
have already had requests for idle_session_timeout, but avoided it
because it seemed better to tell people to monitor pg_stat_activity and
terminate sessions that way, but now that postgres_fdw needs it too,
there might be enough of a requirement to add it.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

On Tue, Jun 30, 2020 at 8:54 AM Masahiko Sawada <
masahiko.sawada@2ndquadrant.com> wrote:

On Mon, 22 Jun 2020 at 14:56, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

When a query on foreign table is executed from a local session using
postgres_fdw, as expected the local postgres backend opens a
connection which causes a remote session/backend to be opened on the
remote postgres server for query execution.

One observation is that, even after the query is finished, the remote
session/backend still persists on the remote postgres server. Upon
researching, I found that there is a concept of Connection Caching for
the remote connections made using postgres_fdw. Local backend/session
can cache up to 8 different connections per backend. This caching is
useful as it avoids the cost of reestablishing new connections per
foreign query.

However, at times, there may be situations where the long lasting
local sessions may execute very few foreign queries and remaining all
are local queries, in this scenario, the remote sessions opened by the
local sessions/backends may not be useful as they remain idle and eat
up the remote server connections capacity. This problem gets even
worse(though this use case is a bit imaginary) if all of
max_connections(default 100 and each backend caching 8 remote
connections) local sessions open remote sessions and they are cached
in the local backend.

I propose to have a new session level GUC called
"enable_connectioncache"(name can be changed if it doesn't correctly
mean the purpose) with the default value being true which means that
all the remote connections are cached. If set to false, the
connections are not cached and so are remote sessions closed by the

local backend/session at

the end of each remote transaction.

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

+1

I have not looked at the implementation, but I agree that here problem
is with postgres_fdw so we should try to solve that by keeping it limited
to postgres_fdw. I liked the idea of passing it as an option to the FDW
connection.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Rushabh Lathia

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.
One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Overall, though we define the server object in a single session, it
will be used in multiple sessions, having an
option at the per-server level would not be a good idea.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 1, 2020 at 2:45 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.

In my opinion, in such cases, one needs to create two server object one with
keep-connection ON and one with keep-connection off. And need to decide
to use appropriate for the particular session.

One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Overall, though we define the server object in a single session, it
will be used in multiple sessions, having an
option at the per-server level would not be a good idea.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

--
Rushabh Lathia

I thought we would add a core capability, idle_session_timeout, which
would disconnect idle sessions, and the postgres_fdw would use that. We
have already had requests for idle_session_timeout, but avoided it
because it seemed better to tell people to monitor pg_stat_activity and
terminate sessions that way, but now that postgres_fdw needs it too,
there might be enough of a requirement to add it.

If we were to use idle_session_timeout (from patch [1]/messages/by-id/763A0689-F189-459E-946F-F0EC4458980B@hotmail.com) for the remote
session to go off without
having to delete the corresponding entry from local connection cache and
after that if we submit foreign query from local session, then below
error would occur,
which may not be an expected behaviour. (I took the patch from [1]/messages/by-id/763A0689-F189-459E-946F-F0EC4458980B@hotmail.com and
intentionally set the
idle_session_timeout to a low value on remote server, issued a
foreign_tbl query which
caused remote session to open and after idle_session_timeout , the
remote session
closes and now issue the foreign_tbl query from local session)

postgres=# SELECT * FROM foreign_tbl;
ERROR: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
CONTEXT: remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ
postgres=#

Another way is that if we are thinking to use idle_session_timeout
infra on the local postgres server to remove cached entries
from the local connection cache, then the question arises:

do we intend to use the same configuration parameter value set for
idle_session_timeout for connection cache as well?
Probably not, as we might use different values for different purposes
of the same idle_session_timeout parameter,
let's say 2000sec for idle_session_timeout and 1000sec for connection
cache cleanup.

[1]: /messages/by-id/763A0689-F189-459E-946F-F0EC4458980B@hotmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 1, 2020 at 3:54 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

If we were to use idle_session_timeout (from patch [1]) for the remote
session to go off without
having to delete the corresponding entry from local connection cache and
after that if we submit foreign query from local session, then below
error would occur,
which may not be an expected behaviour. (I took the patch from [1] and
intentionally set the
idle_session_timeout to a low value on remote server, issued a
foreign_tbl query which
caused remote session to open and after idle_session_timeout , the
remote session
closes and now issue the foreign_tbl query from local session)

postgres=# SELECT * FROM foreign_tbl;
ERROR: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
CONTEXT: remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ
postgres=#

This is actually strange. AFAIR the code, without looking at the
current code, when a query picks a foreign connection it checks its
state. It's possible that the connection has not been marked bad by
the time you fire new query. If the problem exists probably we should
fix it anyway since the backend at the other end of the connection has
higher chances of being killed while the connection was sitting idle
in the cache.

--
Best Wishes,
Ashutosh Bapat

On Wed, 1 Jul 2020 at 18:14, Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

I've not looked at your patch deeply but if this problem is talking
only about postgres_fdw I think we should improve postgres_fdw, not
adding a GUC to the core. It’s not that all FDW plugins use connection
cache and postgres_fdw’s connection cache is implemented within
postgres_fdw, I think we should focus on improving postgres_fdw. I
also think it’s not a good design that the core manages connections to
remote servers connected via FDW. I wonder if we can add a
postgres_fdw option for this purpose, say keep_connection [on|off].
That way, we can set it per server so that remote connections to the
particular server don’t remain idle.

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.
One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Is there use-case in practice where different backends need to have
different connection cache setting even if all of them connect the
same server? I thought since the problem that this feature is trying
to resolve is not to eat up the remote server connections capacity by
disabling connection cache, we’d like to disable connection cache to
the particular server, for example, which sets low max_connections.

Regards,

--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

If we were to use idle_session_timeout (from patch [1]) for the remote
session to go off without
having to delete the corresponding entry from local connection cache and
after that if we submit foreign query from local session, then below
error would occur,
which may not be an expected behaviour. (I took the patch from [1] and
intentionally set the
idle_session_timeout to a low value on remote server, issued a
foreign_tbl query which
caused remote session to open and after idle_session_timeout , the
remote session
closes and now issue the foreign_tbl query from local session)

postgres=# SELECT * FROM foreign_tbl;
ERROR: server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
CONTEXT: remote SQL command: START TRANSACTION ISOLATION LEVEL

REPEATABLE READ

postgres=#

This is actually strange. AFAIR the code, without looking at the
current code, when a query picks a foreign connection it checks its
state. It's possible that the connection has not been marked bad by
the time you fire new query. If the problem exists probably we should
fix it anyway since the backend at the other end of the connection has
higher chances of being killed while the connection was sitting idle
in the cache.

Thanks Ashutosh for the suggestion. One way, we could solve the above
problem is that, upon firing the new foreign query from local backend using
cached
connection, (assuming the remote backend/session that was cached in the
local backed got
killed by some means), instead of failing the query in the local
backend/session, upon
detecting error from remote backend, we could just delete the cached old
entry and try getting another
connection to remote backend/session, cache it and proceed to submit the
query. This has to happen only at
the beginning of remote xact.

This way, instead of failing(as mentioned above " server closed the
connection unexpectedly"),
the query succeeds if the local session is able to get a new remote backend
connection.

I worked on a POC patch to prove the above point. Attaching the patch.
Please note that, the patch doesn't contain comments and has some issues
like having some new
variable in PGconn structure and the things like.

If the approach makes some sense, then I can rework properly on the patch
and probably
can open another thread for the review and other stuff.

The way I tested the patch:

1. select * from foreign_tbl;
/*from local session - this results in a
remote connection being cached in
the connection cache and
a remote backend/session is opened.
*/
2. kill the remote backend/session
3. select * from foreign_tbl;
/*from local session - without patch
this throws error "ERROR: server closed the connection unexpectedly"
with path - try to use
the cached connection at the beginning of remote xact, upon receiving
error from remote postgres
server, instead of aborting the query, delete the cached entry, try to
get a new connection, if it
gets, cache it and use that for executing the query, query succeeds.
*/

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 1, 2020 at 7:13 PM Masahiko Sawada <
masahiko.sawada@2ndquadrant.com> wrote:

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.

In my opinion, in such cases, one needs to create two server object one with
keep-connection ON and one with keep-connection off. And need to decide
to use appropriate for the particular session.

Yes, having two variants of foreign servers: one with keep-connections
on (this can be default behavior,
even if user doesn't mention this option, internally it can be treated
as keep-connections on) ,
and if users need no connection hashing, another foreign server with
all other options same but keep-connections
off.

This looks okay to me, if we want to avoid a core session level GUC.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.
One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Is there use-case in practice where different backends need to have
different connection cache setting even if all of them connect the
same server?

Currently, connection cache exists at each backend/session level and
gets destroyed
on backend/session exit. I think the same cached connection can be
used until it gets invalidated
due to user mapping or server definition changes.

One way is to have a shared memory based connection cache instead of
backend level cache,
but it has its own problems, like maintenance, invalidation, dealing
with concurrent usages etc.

I thought since the problem that this feature is trying
to resolve is not to eat up the remote server connections capacity by
disabling connection cache, we’d like to disable connection cache to
the particular server, for example, which sets low max_connections.

Currently, the user mapping oid acts as the key for the cache's hash
table, so the cache entries
are not made directly using foreign server ids though each entry would
have some information related
to foreign server.

Just to reiterate, the main idea if this feature is to give the user
a way to choose, whether to use connection caching or not,
if he decides that his session uses remote queries very rarely, then
he can disable, or if the remote queries are more frequent in
a particular session, he can choose to use connection caching.

In a way, this feature addresses the point that local sessions not
eating up remote connections/sessions by
letting users decide(as users know better when to cache or when not
to) to cache or not cache the remote connections
and thus releasing them immediately if there is not much usage from
local session.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jul 2, 2020 at 4:29 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

This is actually strange. AFAIR the code, without looking at the
current code, when a query picks a foreign connection it checks its
state. It's possible that the connection has not been marked bad by
the time you fire new query. If the problem exists probably we should
fix it anyway since the backend at the other end of the connection has
higher chances of being killed while the connection was sitting idle
in the cache.

Thanks Ashutosh for the suggestion. One way, we could solve the above
problem is that, upon firing the new foreign query from local backend using cached
connection, (assuming the remote backend/session that was cached in the local backed got
killed by some means), instead of failing the query in the local backend/session, upon
detecting error from remote backend, we could just delete the cached old entry and try getting another
connection to remote backend/session, cache it and proceed to submit the query. This has to happen only at
the beginning of remote xact.

Yes, I believe that would be good.

This way, instead of failing(as mentioned above " server closed the connection unexpectedly"),
the query succeeds if the local session is able to get a new remote backend connection.

In GetConnection() there's a comment
/*
* We don't check the health of cached connection here, because it would
* require some overhead. Broken connection will be detected when the
* connection is actually used.
*/
Possibly this is where you want to check the health of connection when
it's being used the first time in a transaction.

I worked on a POC patch to prove the above point. Attaching the patch.
Please note that, the patch doesn't contain comments and has some issues like having some new
variable in PGconn structure and the things like.

I don't think changing the PGConn structure for this is going to help.
It's a libpq construct and used by many other applications/tools other
than postgres_fdw. Instead you could use ConnCacheEntry for the same.
See how we track invalidated connection and reconnect upon
invalidation.

If the approach makes some sense, then I can rework properly on the patch and probably
can open another thread for the review and other stuff.

The way I tested the patch:

1. select * from foreign_tbl;
/*from local session - this results in a
remote connection being cached in
the connection cache and
a remote backend/session is opened.
*/
2. kill the remote backend/session
3. select * from foreign_tbl;
/*from local session - without patch
this throws error "ERROR: server closed the connection unexpectedly"
with path - try to use
the cached connection at the beginning of remote xact, upon receiving
error from remote postgres
server, instead of aborting the query, delete the cached entry, try to
get a new connection, if it
gets, cache it and use that for executing the query, query succeeds.
*/

This will work. Be cognizant of the fact that the same connection may
be used by multiple plan nodes.

--
Best Wishes,
Ashutosh Bapat

On Wed, Jul 1, 2020 at 5:15 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.
One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Overall, though we define the server object in a single session, it
will be used in multiple sessions, having an
option at the per-server level would not be a good idea.

You present this here as if it should be a Boolean (on or off) but I
don't see why that should be the case. You can imagine trying to close
connections if they have been idle for a certain length of time, or if
there are more than a certain number of them, rather than (or in
addition to) always/never. Which one is best, and why?

I tend to think this is better as an FDW property rather than a core
facility, but I'm not 100% sure of that and I think it likely depends
somewhat on the answers we choose to the questions in the preceding
paragraph.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

If I understand it correctly, your suggestion is to add
keep_connection option and use that while defining the server object.
IMO having keep_connection option at the server object level may not
serve the purpose being discussed here.
For instance, let's say I create a foreign server in session 1 with
keep_connection on, and I want to use that
server object in session 2 with keep_connection off and session 3 with
keep_connection on and so on.
One way we can change the server's keep_connection option is to alter
the server object, but that's not a good choice,
as we have to alter it at the system level.

Overall, though we define the server object in a single session, it
will be used in multiple sessions, having an
option at the per-server level would not be a good idea.

You present this here as if it should be a Boolean (on or off) but I
don't see why that should be the case. You can imagine trying to close
connections if they have been idle for a certain length of time, or if
there are more than a certain number of them, rather than (or in
addition to) always/never. Which one is best, and why?

If the cached connection idle time property is used (I'm thinking we
can define it per server object) then the local backend might have to
close the connections which are lying unused more than idle time. To
perform this task, the local backend might have to do it before it
goes into idle state(as suggested by you in [1]/messages/by-id/CA+Tgmob_ksTOgmbXhno+k5XXPOK+-JYYLoU3MpXuutP4bH7gzA@mail.gmail.com). Please correct, if
my understanding/thinking is wrong here.

If the connection clean up is to be done by the local backend, then a
point can be - let say a local session initially issues few foreign
queries for which connections are cached, and it keeps executing all
local queries, without never going to idle mode(I think this scenario
looks too much impractical to me), then we may never clean the unused
cached connections. If this scenario is really impractical if we are
sure that there are high chances that the local backend goes to idle
mode, then the idea of having per-server-object idle time and letting
the local backend clean it up before it goes to idle mode looks great
to me.

I tend to think this is better as an FDW property rather than a core
facility, but I'm not 100% sure of that and I think it likely depends
somewhat on the answers we choose to the questions in the preceding
paragraph.

I completely agree on having it as a FDW property.

[1]: /messages/by-id/CA+Tgmob_ksTOgmbXhno+k5XXPOK+-JYYLoU3MpXuutP4bH7gzA@mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Jul 8, 2020 at 9:26 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

If the cached connection idle time property is used (I'm thinking we
can define it per server object) then the local backend might have to
close the connections which are lying unused more than idle time. To
perform this task, the local backend might have to do it before it
goes into idle state(as suggested by you in [1]). Please correct, if
my understanding/thinking is wrong here.

If the connection clean up is to be done by the local backend, then a
point can be - let say a local session initially issues few foreign
queries for which connections are cached, and it keeps executing all
local queries, without never going to idle mode(I think this scenario
looks too much impractical to me), then we may never clean the unused
cached connections. If this scenario is really impractical if we are
sure that there are high chances that the local backend goes to idle
mode, then the idea of having per-server-object idle time and letting
the local backend clean it up before it goes to idle mode looks great
to me.

If it just did it before going idle, then what about sessions that
haven't reached the timeout at the point when we go idle, but do reach
the timeout later? And how would the FDW get control at the right time
anyway?

I tend to think this is better as an FDW property rather than a core
facility, but I'm not 100% sure of that and I think it likely depends
somewhat on the answers we choose to the questions in the preceding
paragraph.

I completely agree on having it as a FDW property.

Right, but not everyone does. It looks to me like there have been
votes on both sides.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company