On Mon, Jan 18, 2021 at 9:11 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> > Attaching v12 patch set. 0001 is for postgres_fdw_disconnect()
> > function, 0002 is for keep_connections GUC and 0003 is for
> > keep_connection server level option.
>
> Thanks!
>
> >
> > Please review it further.
>
> + server = GetForeignServerByName(servername, true);
> +
> + if (!server)
> + ereport(ERROR,
> +
> (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
> + errmsg("foreign server \"%s\" does
> not exist", servername)));
>
> ISTM we can simplify this code as follows.
>
> server = GetForeignServerByName(servername, false);
Done.
> + hash_seq_init(&scan, ConnectionHash);
> + while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
>
> When the server name is specified, even if its connection is successfully
> closed, postgres_fdw_disconnect() scans through all the entries to check
> whether there are active connections. But if "result" is true and
> active_conn_exists is true, we can get out of this loop to avoid unnecessary
> scans.
My initial thought was that it's possible to have two entries with the
same foreign server name but with different user mappings, looks like
it's not possible. I tried associating a foreign server with two
different user mappings [1], then the cache entry is getting
associated initially with the user mapping that comes first in the
pg_user_mappings, if this user mapping is dropped then the cache entry
gets invalidated, so next time the second user mapping is used.
Since there's no way we can have two cache entries with the same
foreign server name, we can get out of the loop when we find the cache
entry match with the given server. I made the changes.
[1]
postgres=# select * from pg_user_mappings ;
umid | srvid | srvname | umuser | usename | umoptions
-------+-------+-----------+--------+---------+-----------
16395 | 16394 | loopback1 | 10 | bharath | -----> cache entry
is initially made with this user mapping.
16399 | 16394 | loopback1 | 0 | public | -----> if the
above user mapping is dropped, then the cache entry is made with this
user mapping.
> + /*
> + * Destroy the cache if we discarded all active connections i.e. if
> there
> + * is no single active connection, which we can know while scanning
> the
> + * cached entries in the above loop. Destroying the cache is better
> than to
> + * keep it in the memory with all inactive entries in it to save some
> + * memory. Cache can get initialized on the subsequent queries to
> foreign
> + * server.
>
> How much memory is assumed to be saved by destroying the cache in
> many cases? I'm not sure if it's really worth destroying the cache to save
> the memory.
I removed the cache destroying code, if somebody complains in
future(after the feature commit), we can really revisit then.
> + a warning is issued and <literal>false</literal> is returned.
> <literal>false</literal>
> + is returned when there are no open connections. When there are some
> open
> + connections, but there is no connection for the given foreign server,
> + then <literal>false</literal> is returned. When no foreign server
> exists
> + with the given name, an error is emitted. Example usage of the
> function:
>
> When a non-existent server name is specified, postgres_fdw_disconnect()
> emits an error if there is at least one open connection, but just returns
> false otherwise. At least for me, this behavior looks inconsitent and strange.
> In that case, IMO the function always should emit an error.
Done.
Attaching v13 patch set, please review it further.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 0731c6244ac228818916d62cc51ea1434178c5be Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Tue, 19 Jan 2021 08:23:55 +0530
Subject: [PATCH v13] postgres_fdw function to discard cached connections
This patch introduces a new function postgres_fdw_disconnect().
When called with a foreign server name, it discards the associated
connection with the server. When called without any argument, it
discards all the existing cached connections.
---
contrib/postgres_fdw/connection.c | 147 ++++++++++++++++++
.../postgres_fdw/expected/postgres_fdw.out | 93 +++++++++++
.../postgres_fdw/postgres_fdw--1.0--1.1.sql | 10 ++
contrib/postgres_fdw/sql/postgres_fdw.sql | 35 +++++
doc/src/sgml/postgres-fdw.sgml | 59 +++++++
5 files changed, 344 insertions(+)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index a1404cb6bb..287a047c80 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -80,6 +80,7 @@ static bool xact_got_connection = false;
* SQL functions
*/
PG_FUNCTION_INFO_V1(postgres_fdw_get_connections);
+PG_FUNCTION_INFO_V1(postgres_fdw_disconnect);
/* prototypes of private functions */
static void make_new_connection(ConnCacheEntry *entry, UserMapping *user);
@@ -102,6 +103,7 @@ static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
static bool pgfdw_get_cleanup_result(PGconn *conn, TimestampTz endtime,
PGresult **result);
static bool UserMappingPasswordRequired(UserMapping *user);
+static bool disconnect_cached_connections(uint32 hashvalue, bool all);
/*
* Get a PGconn which can be used to execute queries on the remote PostgreSQL
@@ -1470,3 +1472,148 @@ postgres_fdw_get_connections(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+
+/*
+ * Disconnect cached connections.
+ *
+ * If server name is provided as input, this function disconnects the
+ * associated cached connection. Otherwise all the cached connections are
+ * disconnected.
+ *
+ * This function returns false if the cache doesn't exist.
+ * When the cache exists:
+ * 1) If the server name is provided, it first checks whether the foreign
+ * server exists, if not, an error is emitted. Otherwise it disconnects the
+ * associated connection when it's not being used in current transaction
+ * and returns true. If it's in use, then issues a warning and returns
+ * false.
+ * 2) If no input argument is provided, then it tries to disconnect all the
+ * connections. If all the connections are not being used, then it
+ * disconnects them and returns true. If all the connections are being
+ * used, then it issues a warning and returns false. If at least one
+ * connection is closed and others are in use, then issues a warning and
+ * returns true.
+ */
+Datum
+postgres_fdw_disconnect(PG_FUNCTION_ARGS)
+{
+ bool result = false;
+
+ if (PG_NARGS() == 1)
+ {
+ ForeignServer *server = NULL;
+ char *servername = NULL;
+ uint32 hashvalue;
+
+ servername = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ server = GetForeignServerByName(servername, false);
+
+ if (!ConnectionHash)
+ PG_RETURN_BOOL(result);
+
+ hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID,
+ ObjectIdGetDatum(server->serverid));
+ result = disconnect_cached_connections(hashvalue, false);
+ }
+ else if (PG_NARGS() == 0 && ConnectionHash)
+ result = disconnect_cached_connections(0, true);
+
+ PG_RETURN_BOOL(result);
+}
+
+/*
+ * Workhorse to disconnect the cached connections.
+ *
+ * This function tries to disconnect the connections only when they are not in
+ * use in the current transaction.
+ *
+ * If all is true, all the cached connections that are not being used in the
+ * current transaction are disconnected. Otherwise, the unused entries with the
+ * given hashvalue are disconnected.
+ *
+ * This function returns true if at least one connection is disconnected.
+ * Otherwise it returns false.
+ */
+static bool
+disconnect_cached_connections(uint32 hashvalue, bool all)
+{
+ HASH_SEQ_STATUS scan;
+ ConnCacheEntry *entry;
+ bool result = false;
+ bool is_in_use = false;
+
+ /* We are here only when ConnectionHash exists. */
+ Assert(ConnectionHash);
+
+ hash_seq_init(&scan, ConnectionHash);
+ while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
+ {
+ /*
+ * Either disconnect given or all the active and not in use cached
+ * connections.
+ */
+ if ((all || entry->server_hashvalue == hashvalue) &&
+ entry->conn)
+ {
+ if (entry->xact_depth > 0)
+ is_in_use = true;
+ else
+ {
+ elog(DEBUG3, "discarding connection %p", entry->conn);
+ disconnect_pg_server(entry);
+ result = true;
+ }
+
+ /*
+ * For the given server, if we closed connection or it is still in
+ * use, then no need of scanning the cache further.
+ */
+ if (entry->server_hashvalue == hashvalue && (is_in_use ||
+ result))
+ {
+ hash_seq_term(&scan);
+ break;
+ }
+ }
+ }
+
+ /*
+ * is_in_use flag would be set to true when there exists at least one
+ * connection that's being used in the current transaction.
+ */
+ if (all)
+ {
+ /*
+ * Check if some or all of the connections are in use i.e.
+ * entry->xact_depth > 0. Since we can not close them, so inform user.
+ */
+ if (is_in_use)
+ {
+ if (result)
+ {
+ /* We closed at least one connection, others are in use. */
+ ereport(WARNING,
+ (errmsg("cannot close all connections because some of them are still in use")));
+ }
+ else
+ {
+ /* We did not close any connection, all are in use. */
+ ereport(WARNING,
+ (errmsg("cannot close any connection because they are still in use")));
+ }
+ }
+ }
+ else
+ {
+ /*
+ * Check if the connection associated with the given foreign server is
+ * in use i.e. entry->xact_depth > 0. Since we can not close it, so
+ * error out.
+ */
+ if (is_in_use)
+ ereport(WARNING,
+ (errmsg("cannot close the connection because it is still in use")));
+ }
+
+ return result;
+}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1cad311436..b478a8382d 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9112,3 +9112,96 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
loopback2 | t
(1 row)
+-- ===================================================================
+-- test postgres_fdw_disconnect function
+-- ===================================================================
+-- Returns true as it closes loopback2 connection.
+SELECT * FROM postgres_fdw_disconnect('loopback2');
+ postgres_fdw_disconnect
+-------------------------
+ t
+(1 row)
+
+-- Returns false as there is no cached connection.
+SELECT * FROM postgres_fdw_disconnect();
+ postgres_fdw_disconnect
+-------------------------
+ f
+(1 row)
+
+-- Ensure to cache loopback connection.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+BEGIN;
+-- Ensure to cache loopback2 connection.
+SELECT 1 FROM ft6 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- List all the existing cached connections. loopback and loopback2 should be
+-- output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+ loopback | t
+ loopback2 | t
+(2 rows)
+
+-- Issues a warning, returns false as loopback2 connection is still in use and
+-- can not be closed.
+SELECT * FROM postgres_fdw_disconnect('loopback2');
+WARNING: cannot close the connection because it is still in use
+ postgres_fdw_disconnect
+-------------------------
+ f
+(1 row)
+
+-- Closes loopback connection, returns true and issues a warning as loopback2
+-- connection is still in use and can not be closed.
+SELECT * FROM postgres_fdw_disconnect();
+WARNING: cannot close all connections because some of them are still in use
+ postgres_fdw_disconnect
+-------------------------
+ t
+(1 row)
+
+-- Issues a warning and returns false as it can not close any connection in the
+-- cache.
+SELECT * FROM postgres_fdw_disconnect();
+WARNING: cannot close any connection because they are still in use
+ postgres_fdw_disconnect
+-------------------------
+ f
+(1 row)
+
+COMMIT;
+-- List all the existing cached connections. loopback2 should be output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+ loopback2 | t
+(1 row)
+
+-- Returns an error as there is no foreign server with given name.
+SELECT * FROM postgres_fdw_disconnect('unknownserver');
+ERROR: server "unknownserver" does not exist
+-- Closes loopback2 connection and returns true.
+SELECT * FROM postgres_fdw_disconnect();
+ postgres_fdw_disconnect
+-------------------------
+ t
+(1 row)
+
+-- List all the existing cached connections. No connection exists, so NULL
+-- should be output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+(0 rows)
+
diff --git a/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql
index 7f85784466..63dff0f7a8 100644
--- a/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql
+++ b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql
@@ -8,3 +8,13 @@ CREATE FUNCTION postgres_fdw_get_connections (OUT server_name text,
RETURNS SETOF record
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT PARALLEL RESTRICTED;
+
+CREATE FUNCTION postgres_fdw_disconnect ()
+RETURNS bool
+AS 'MODULE_PATHNAME','postgres_fdw_disconnect'
+LANGUAGE C STRICT PARALLEL RESTRICTED;
+
+CREATE FUNCTION postgres_fdw_disconnect (text)
+RETURNS bool
+AS 'MODULE_PATHNAME','postgres_fdw_disconnect'
+LANGUAGE C STRICT PARALLEL RESTRICTED;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index ebf6eb10a6..e9c3fd2d41 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2738,3 +2738,38 @@ COMMIT;
-- should not be output because they should be closed at the end of
-- the above transaction.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+
+-- ===================================================================
+-- test postgres_fdw_disconnect function
+-- ===================================================================
+-- Returns true as it closes loopback2 connection.
+SELECT * FROM postgres_fdw_disconnect('loopback2');
+-- Returns false as there is no cached connection.
+SELECT * FROM postgres_fdw_disconnect();
+-- Ensure to cache loopback connection.
+SELECT 1 FROM ft1 LIMIT 1;
+BEGIN;
+-- Ensure to cache loopback2 connection.
+SELECT 1 FROM ft6 LIMIT 1;
+-- List all the existing cached connections. loopback and loopback2 should be
+-- output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- Issues a warning, returns false as loopback2 connection is still in use and
+-- can not be closed.
+SELECT * FROM postgres_fdw_disconnect('loopback2');
+-- Closes loopback connection, returns true and issues a warning as loopback2
+-- connection is still in use and can not be closed.
+SELECT * FROM postgres_fdw_disconnect();
+-- Issues a warning and returns false as it can not close any connection in the
+-- cache.
+SELECT * FROM postgres_fdw_disconnect();
+COMMIT;
+-- List all the existing cached connections. loopback2 should be output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- Returns an error as there is no foreign server with given name.
+SELECT * FROM postgres_fdw_disconnect('unknownserver');
+-- Closes loopback2 connection and returns true.
+SELECT * FROM postgres_fdw_disconnect();
+-- List all the existing cached connections. No connection exists, so NULL
+-- should be output.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 6a91926da8..c05d29a996 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -507,6 +507,51 @@ postgres=# SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term><function>postgres_fdw_disconnect(IN servername text) returns boolean</function></term>
+ <listitem>
+ <para>
+ When called in local session with foreign server name as input, it
+ discards the unused open connection previously made to the foreign server
+ and returns <literal>true</literal>. If the open connection is still
+ being used in current transaction, it is not discarded, instead a warning
+ is issued and <literal>false</literal> is returned. When no foreign
+ server exists with the given name, an error is emitted. <literal>false</literal>
+ is returned when there are no open connections at all. When there are
+ some open connections, but there is no open connection for the given
+ foreign server, then <literal>false</literal> is returned. Example usage
+ of the function:
+ <screen>
+postgres=# SELECT * FROM postgres_fdw_disconnect('loopback1');
+ postgres_fdw_disconnect
+-------------------------
+ t
+</screen>
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><function>postgres_fdw_disconnect() returns boolean</function></term>
+ <listitem>
+ <para>
+ When called in the local session with no input argument, it discards all
+ the unused open connections that are previously made to the foreign
+ servers and returns <literal>true</literal>. If there is any open
+ connection that is still being used in the current transaction, then a
+ warning is issued. <literal>false</literal> is returned when no open
+ connection is discarded or there are no open connections at all. Example
+ usage of the function:
+ <screen>
+postgres=# SELECT * FROM postgres_fdw_disconnect();
+ postgres_fdw_disconnect
+-------------------------
+ t
+</screen>
+ </para>
+ </listitem>
+ </varlistentry>
</variablelist>
</sect2>
@@ -522,6 +567,20 @@ postgres=# SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
multiple user identities (user mappings) are used to access the foreign
server, a connection is established for each user mapping.
</para>
+
+ <para>
+ Since the <filename>postgres_fdw</filename> keeps the connections to remote
+ servers in the local session, the corresponding sessions that are opened on
+ the remote servers are kept idle until they are re-used by the local session.
+ This may waste resources if those connections are not frequently used by the
+ local session. To address this, the <filename>postgres_fdw</filename>
+ provides following way to remove the connections to the remote servers and
+ so the remote sessions:
+
+ <function>postgres_fdw_disconnect()</function> to discard all the
+ connections or <function>postgres_fdw_disconnect(text)</function>
+ to discard the connection associated with the given foreign server.
+ </para>
</sect2>
<sect2>
--
2.25.1
From 899be02dbf203ac31b7dff97dba65c3273c40bd0 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 18 Jan 2021 18:13:04 +0530
Subject: [PATCH v13] postgres_fdw add keep_connections GUC to not cache
connections
This patch adds a new GUC postgres_fdw.keep_connections, default
being on, when set to off no remote connections are cached by the
local session.
---
contrib/postgres_fdw/connection.c | 12 +++--
.../postgres_fdw/expected/postgres_fdw.out | 43 ++++++++++++++++++
contrib/postgres_fdw/postgres_fdw.c | 16 +++++++
contrib/postgres_fdw/postgres_fdw.h | 3 ++
contrib/postgres_fdw/sql/postgres_fdw.sql | 20 +++++++++
doc/src/sgml/postgres-fdw.sgml | 44 ++++++++++++++++++-
6 files changed, 134 insertions(+), 4 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 1e93a84aaa..88943361c6 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -814,6 +814,7 @@ pgfdw_xact_callback(XactEvent event, void *arg)
while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
{
PGresult *res;
+ bool used_in_current_xact = false;
/* Ignore cache entry if no open connection right now */
if (entry->conn == NULL)
@@ -824,6 +825,8 @@ pgfdw_xact_callback(XactEvent event, void *arg)
{
bool abort_cleanup_failure = false;
+ used_in_current_xact = true;
+
elog(DEBUG3, "closing remote transaction on connection %p",
entry->conn);
@@ -958,16 +961,19 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/*
* If the connection isn't in a good idle state or it is marked as
- * invalid, then discard it to recover. Next GetConnection will open a
- * new connection.
+ * invalid or keep_connections GUC is false and this connection is used
+ * in current xact, then discard it to recover. Next GetConnection will
+ * open a new connection.
*/
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
entry->changing_xact_state ||
- entry->invalidated)
+ entry->invalidated ||
+ (used_in_current_xact && !keep_connections))
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);
+ used_in_current_xact = false;
}
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 33c61d8a14..e5e5018769 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9205,3 +9205,46 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
-------------+-------
(0 rows)
+-- ===================================================================
+-- Test postgres_fdw.keep_connections GUC
+-- ===================================================================
+-- By default, keep_connections GUC is on i.e. local session caches all the
+-- foreign server connections.
+SHOW postgres_fdw.keep_connections;
+ postgres_fdw.keep_connections
+-------------------------------
+ on
+(1 row)
+
+-- Set it off i.e. the cached connections which are used after this setting are
+-- disconnected at the end of respective xacts.
+SET postgres_fdw.keep_connections TO off;
+-- Make connection using loopback server, connection should not be cached as
+-- the GUC is off.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- Should output nothing.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+(0 rows)
+
+-- Set it on i.e. connections are cached.
+SET postgres_fdw.keep_connections TO on;
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- Should output loopback.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+ loopback | t
+(1 row)
+
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 2f2d4d171c..0c2ced501e 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -302,6 +302,8 @@ typedef struct
List *already_used; /* expressions already dealt with */
} ec_member_foreign_arg;
+bool keep_connections = true;
+
/*
* SQL functions
*/
@@ -506,6 +508,20 @@ static void merge_fdw_options(PgFdwRelationInfo *fpinfo,
const PgFdwRelationInfo *fpinfo_o,
const PgFdwRelationInfo *fpinfo_i);
+void
+_PG_init(void)
+{
+ DefineCustomBoolVariable("postgres_fdw.keep_connections",
+ "Enables postgres_fdw connection caching.",
+ "When off postgres_fdw will close connections at the end of transaction.",
+ &keep_connections,
+ true,
+ PGC_USERSET,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+}
/*
* Foreign-data wrapper handler function: return a struct with pointers
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index 19ea27a1bc..4d8dd4cd4a 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -124,9 +124,12 @@ typedef struct PgFdwRelationInfo
int relation_index;
} PgFdwRelationInfo;
+extern bool keep_connections;
+
/* in postgres_fdw.c */
extern int set_transmission_modes(void);
extern void reset_transmission_modes(int nestlevel);
+extern void _PG_init(void);
/* in connection.c */
extern PGconn *GetConnection(UserMapping *user, bool will_prep_stmt);
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index e9c3fd2d41..8f054690c4 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2773,3 +2773,23 @@ SELECT * FROM postgres_fdw_disconnect();
-- List all the existing cached connections. No connection exists, so NULL
-- should be output.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+
+-- ===================================================================
+-- Test postgres_fdw.keep_connections GUC
+-- ===================================================================
+-- By default, keep_connections GUC is on i.e. local session caches all the
+-- foreign server connections.
+SHOW postgres_fdw.keep_connections;
+-- Set it off i.e. the cached connections which are used after this setting are
+-- disconnected at the end of respective xacts.
+SET postgres_fdw.keep_connections TO off;
+-- Make connection using loopback server, connection should not be cached as
+-- the GUC is off.
+SELECT 1 FROM ft1 LIMIT 1;
+-- Should output nothing.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- Set it on i.e. connections are cached.
+SET postgres_fdw.keep_connections TO on;
+SELECT 1 FROM ft1 LIMIT 1;
+-- Should output loopback.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 4c5264aaa3..0ee1f6bd87 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -555,6 +555,42 @@ postgres=# SELECT * FROM postgres_fdw_disconnect();
</sect2>
+<sect2>
+ <title>Configuration Parameters</title>
+
+ <variablelist>
+ <varlistentry>
+
+ <term>
+ <varname>postgres_fdw.keep_connections</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>postgres_fdw.keep_connections</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+
+ <listitem>
+
+ <para>
+ Allows <filename>postgres_fdw</filename> to keep or discard the
+ connection made to the foreign server by the local session. Default is
+ <literal>on</literal>. When set to <literal>off</literal> the local
+ session doesn't keep the connections made to the foreign servers. Each
+ connection is discarded at the end of transaction in which it is used.
+ </para>
+
+ <para>
+ Note that setting <varname>postgres_fdw.keep_connections</varname> to
+ <literal>off</literal> does not discard any previously made and still open
+ connections immediately. They will be closed only at the end of future
+ transactions, which operated on them. To close all connections
+ immediately use <function>postgres_fdw_disconnect</function> function.
+ </para>
+
+ </listitem>
+ </varlistentry>
+ </variablelist>
+ </sect2>
+
<sect2>
<title>Connection Management</title>
@@ -573,12 +609,18 @@ postgres=# SELECT * FROM postgres_fdw_disconnect();
the remote servers are kept idle until they are re-used by the local session.
This may waste resources if those connections are not frequently used by the
local session. To address this, the <filename>postgres_fdw</filename>
- provides following way to remove the connections to the remote servers and
+ provides following ways to remove the connections to the remote servers and
so the remote sessions:
<function>postgres_fdw_disconnect()</function> to discard all the
connections or <function>postgres_fdw_disconnect(text)</function>
to discard the connection associated with the given foreign server.
+
+ A configuration parameter, <varname>postgres_fdw.keep_connections</varname>,
+ default being <literal>on</literal>, when set to <literal>off</literal>, the
+ local session doesn't keep remote connections that are made to the foreign
+ servers. Each connection is discarded at the end of transaction in which it
+ is used.
</para>
</sect2>
--
2.25.1
From f89d2b280593e2dc16bcfbf7dd64db0f47eb4ea8 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Mon, 18 Jan 2021 18:26:36 +0530
Subject: [PATCH v13] postgres_fdw server level option, keep_connection to not
cache connection
This patch adds a new server level option, keep_connection,
default being on, when set to off, the local session doesn't cache
the connections associated with the foreign server.
---
contrib/postgres_fdw/connection.c | 25 +++++++++--
.../postgres_fdw/expected/postgres_fdw.out | 44 ++++++++++++++++++-
contrib/postgres_fdw/option.c | 9 +++-
contrib/postgres_fdw/sql/postgres_fdw.sql | 19 ++++++++
doc/src/sgml/postgres-fdw.sgml | 43 ++++++++++++++++++
5 files changed, 134 insertions(+), 6 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 88943361c6..35bafd1287 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -62,6 +62,8 @@ typedef struct ConnCacheEntry
Oid serverid; /* foreign server OID used to get server name */
uint32 server_hashvalue; /* hash value of foreign server OID */
uint32 mapping_hashvalue; /* hash value of user mapping OID */
+ /* Keep or discard this connection at the end of xact */
+ bool keep_connection;
} ConnCacheEntry;
/*
@@ -124,6 +126,8 @@ GetConnection(UserMapping *user, bool will_prep_stmt)
ConnCacheEntry *entry;
ConnCacheKey key;
MemoryContext ccxt = CurrentMemoryContext;
+ ListCell *lc;
+ ForeignServer *server;
/* First time through, initialize connection cache hashtable */
if (ConnectionHash == NULL)
@@ -266,6 +270,15 @@ GetConnection(UserMapping *user, bool will_prep_stmt)
begin_remote_xact(entry);
}
+ server = GetForeignServer(user->serverid);
+ foreach(lc, server->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "keep_connection") == 0)
+ entry->keep_connection = defGetBoolean(def);
+ }
+
/* Remember if caller will prepare statements */
entry->have_prep_stmt |= will_prep_stmt;
@@ -290,6 +303,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user)
entry->changing_xact_state = false;
entry->invalidated = false;
entry->serverid = server->serverid;
+ entry->keep_connection = true;
entry->server_hashvalue =
GetSysCacheHashValue1(FOREIGNSERVEROID,
ObjectIdGetDatum(server->serverid));
@@ -961,15 +975,18 @@ pgfdw_xact_callback(XactEvent event, void *arg)
/*
* If the connection isn't in a good idle state or it is marked as
- * invalid or keep_connections GUC is false and this connection is used
- * in current xact, then discard it to recover. Next GetConnection will
- * open a new connection.
+ * invalid or this connection is used in current xact and
+ * keep_connections GUC is false or GUC is true but the server level
+ * option is false, then discard it to recover. Next GetConnection will
+ * open a new connection. Note that keep_connections GUC overrides the
+ * server level keep_connection option.
*/
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
entry->changing_xact_state ||
entry->invalidated ||
- (used_in_current_xact && !keep_connections))
+ (used_in_current_xact &&
+ (!keep_connections || !entry->keep_connection)))
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index e5e5018769..40329f9b74 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8923,7 +8923,7 @@ DO $d$
END;
$d$;
ERROR: invalid option "password"
-HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size
+HINT: Valid options in this context are: service, passfile, channel_binding, connect_timeout, dbname, host, hostaddr, port, options, application_name, keepalives, keepalives_idle, keepalives_interval, keepalives_count, tcp_user_timeout, sslmode, sslcompression, sslcert, sslkey, sslrootcert, sslcrl, requirepeer, ssl_min_protocol_version, ssl_max_protocol_version, gssencmode, krbsrvname, gsslib, target_session_attrs, use_remote_estimate, fdw_startup_cost, fdw_tuple_cost, extensions, updatable, fetch_size, keep_connection
CONTEXT: SQL statement "ALTER SERVER loopback_nopw OPTIONS (ADD password 'dummypw')"
PL/pgSQL function inline_code_block line 3 at EXECUTE
-- If we add a password for our user mapping instead, we should get a different
@@ -9248,3 +9248,45 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
loopback | t
(1 row)
+-- ===================================================================
+-- Test foreign server level option keep_connection
+-- ===================================================================
+-- By default, the connections associated with foreign server are cached i.e.
+-- keep_connection option is on. Set it to off.
+ALTER SERVER loopback OPTIONS (keep_connection 'off');
+-- loopback server connection is closed by the local session at the end of xact
+-- as the keep_connection was set to off.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- Should output nothing.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+(0 rows)
+
+ALTER SERVER loopback OPTIONS (SET keep_connection 'on');
+-- loopback server connection should get cached.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- Should output loopback.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid
+-------------+-------
+ loopback | t
+(1 row)
+
+-- Closes loopback connection and returns true.
+SELECT * FROM postgres_fdw_disconnect();
+ postgres_fdw_disconnect
+-------------------------
+ t
+(1 row)
+
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 1fec3c3eea..e8a144c06c 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -107,7 +107,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
* Validate option value, when we can do so without any context.
*/
if (strcmp(def->defname, "use_remote_estimate") == 0 ||
- strcmp(def->defname, "updatable") == 0)
+ strcmp(def->defname, "updatable") == 0 ||
+ strcmp(def->defname, "keep_connection") == 0)
{
/* these accept only boolean values */
(void) defGetBoolean(def);
@@ -213,6 +214,12 @@ InitPgFdwOptions(void)
{"sslcert", UserMappingRelationId, true},
{"sslkey", UserMappingRelationId, true},
+ /*
+ * If true, cache the connection associated with this server, otherwise
+ * remove it at the end of the xact. Default is true.
+ */
+ {"keep_connection", ForeignServerRelationId, false},
+
{NULL, InvalidOid, false}
};
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 8f054690c4..ded8805763 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2793,3 +2793,22 @@ SET postgres_fdw.keep_connections TO on;
SELECT 1 FROM ft1 LIMIT 1;
-- Should output loopback.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+
+-- ===================================================================
+-- Test foreign server level option keep_connection
+-- ===================================================================
+-- By default, the connections associated with foreign server are cached i.e.
+-- keep_connection option is on. Set it to off.
+ALTER SERVER loopback OPTIONS (keep_connection 'off');
+-- loopback server connection is closed by the local session at the end of xact
+-- as the keep_connection was set to off.
+SELECT 1 FROM ft1 LIMIT 1;
+-- Should output nothing.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ALTER SERVER loopback OPTIONS (SET keep_connection 'on');
+-- loopback server connection should get cached.
+SELECT 1 FROM ft1 LIMIT 1;
+-- Should output loopback.
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- Closes loopback connection and returns true.
+SELECT * FROM postgres_fdw_disconnect();
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index 0ee1f6bd87..fca19eb653 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -477,6 +477,35 @@ OPTIONS (ADD password_required 'false');
</para>
</sect3>
+
+ <sect3>
+ <title>Connection Management Options</title>
+
+ <para>
+ By default the foreign server connections made with
+ <filename>postgres_fdw</filename> are kept in local session for re-use.
+ This may be overridden using the following
+ <xref linkend="sql-createserver"/> option:
+ </para>
+
+ <variablelist>
+
+ <varlistentry>
+ <term><literal>keep_connection</literal></term>
+ <listitem>
+ <para>
+ This option controls whether <filename>postgres_fdw</filename> keeps the
+ server connection that is made with a specific foreign server. It can be
+ specified for a foreign server. Default is <literal>on</literal>. When
+ set to <literal>off</literal>, the associated foreign server connection
+ is discarded at the end of the transaction.
+ </para>
+
+ </listitem>
+ </varlistentry>
+
+ </variablelist>
+ </sect3>
</sect2>
<sect2>
@@ -578,6 +607,14 @@ postgres=# SELECT * FROM postgres_fdw_disconnect();
connection is discarded at the end of transaction in which it is used.
</para>
+ <para>
+ <varname>postgres_fdw.keep_connections</varname> configuration parameter
+ overrides the server level <literal>keep_connection</literal> option.
+ Which means that when the configuration parameter is set to
+ <literal>on</literal>, irrespective of the server option
+ <literal>keep_connection</literal> setting, the connections are discarded.
+ </para>
+
<para>
Note that setting <varname>postgres_fdw.keep_connections</varname> to
<literal>off</literal> does not discard any previously made and still open
@@ -621,6 +658,12 @@ postgres=# SELECT * FROM postgres_fdw_disconnect();
local session doesn't keep remote connections that are made to the foreign
servers. Each connection is discarded at the end of transaction in which it
is used.
+
+ A server level option, <literal>keep_connection</literal> that is used with
+ <xref linkend="sql-createserver"/>. Default being <literal>on</literal>,
+ when set to <literal>off</literal> the local session doesn't keep remote
+ connection associated with the foreign server. The connection is discarded
+ at the end of the transaction.
</para>
</sect2>
--
2.25.1
