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. idea[1]) automatic clean up of cache entries, but letting users decide on caching also seems to be good. Please note that documentation is still pending. 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. [1] - https://www.postgresql.org/message-id/CA%2BTgmob_ksTOgmbXhno%2Bk5XXPOK%2B-JYYLoU3MpXuutP4bH7gzA%40mail.gmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com ![]() ![]() |
On Mon, Jun 22, 2020 at 11:26 AM Bharath Rupireddy
<[hidden email]> 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 |
In reply to this post by Bharath Rupireddy
On Mon, Jun 22, 2020 at 11:26 AM Bharath Rupireddy
<[hidden email]> 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 |
In reply to this post by Bharath Rupireddy
On Sun, Jun 21, 2020 at 10:56 PM Bharath Rupireddy <[hidden email]> wrote:
[...]
[...]
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] 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). |
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 |
In reply to this post by Bharath Rupireddy
On Mon, 22 Jun 2020 at 14:56, Bharath Rupireddy
<[hidden email]> 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 <[hidden email]> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee |
In reply to this post by Masahiko Sawada-2
On Tue, Jun 30, 2020 at 8:54 AM Masahiko Sawada <[hidden email]> wrote: On Mon, 22 Jun 2020 at 14:56, Bharath Rupireddy +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, Rushabh Lathia |
In reply to this post by Masahiko Sawada-2
>
> 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 <[hidden email]> wrote: > 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 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]) 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=# 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] - https://www.postgresql.org/message-id/763A0689-F189-459E-946F-F0EC4458980B%40hotmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com On Wed, Jul 1, 2020 at 3:33 PM Rushabh Lathia <[hidden email]> wrote: > > > > On Wed, Jul 1, 2020 at 2:45 PM Bharath Rupireddy <[hidden email]> 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 |
On Wed, Jul 1, 2020 at 3:54 PM Bharath Rupireddy
<[hidden email]> 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 |
In reply to this post by Bharath Rupireddy
On Wed, 1 Jul 2020 at 18:14, Bharath Rupireddy
<[hidden email]> 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. */ On Wed, Jul 1, 2020 at 7:13 PM Masahiko Sawada <[hidden email]> wrote: On Wed, 1 Jul 2020 at 18:14, Bharath Rupireddy |
In reply to this post by Rushabh Lathia
>>
>> 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 |
In reply to this post by Masahiko Sawada-2
> >
> > 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 |
In reply to this post by Bharath Rupireddy
On Thu, Jul 2, 2020 at 4:29 PM Bharath Rupireddy
<[hidden email]> 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 |
In reply to this post by Bharath Rupireddy
On Wed, Jul 1, 2020 at 5:15 AM Bharath Rupireddy
<[hidden email]> 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
If the cached connection idle time property is used (I'm thinking we
> > 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? > 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. > > 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] - https://www.postgresql.org/message-id/CA%2BTgmob_ksTOgmbXhno%2Bk5XXPOK%2B-JYYLoU3MpXuutP4bH7gzA%40mail.gmail.com With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com |
On Wed, Jul 8, 2020 at 9:26 AM Bharath Rupireddy
<[hidden email]> 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 |
Free forum by Nabble | Edit this page |