On 2021/02/22 14:55, Bharath Rupireddy wrote:
On Thu, Feb 4, 2021 at 9:36 AM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
On Wed, Feb 3, 2021 at 4:22 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
Maybe my explanation in the previous email was unclear. What I think is; If the
server-level option is explicitly specified, its setting is used whatever GUC
is. On the other hand, if the server-level option is NOT specified, GUC setting
is used. For example, if we define the server as follows, GUC setting is used
because the server-level option is NOT specified.
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres;
If we define the server as follows, the server-level setting is used.
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres OPTIONS
(keep_connections 'on');
Attaching v20 patch set. Now, server level option if provided
overrides the GUC.The GUC will be used only if server level option is
not provided. And also, both server level option and GUC are named the
same - "keep_connections".
Please have a look.
Attaching v21 patch set, rebased onto the latest master.
I agree to add the server-level option. But I'm still not sure if it's good
idea to also expose that option as GUC. Isn't the server-level option enough
for most cases?
Also it's strange to expose only this option as GUC while there are other many
postgres_fdw options?
With v21-002 patch, even when keep_connections GUC is disabled, the existing
open connections are not close immediately. Only connections used in the
transaction are closed at the end of that transaction. That is, the existing
connections that no transactions use will never be closed. I'm not sure if this
behavior is intuitive for users.
Therefore for now I'm thinking to support the server-level option at first...
Then if we find it's not enough for most cases in practice, I'd like to
consider to expose postgres_fdw options including keep_connections as GUC.
Thought?
BTW these patches fail to be applied to the master because of commit
27e1f14563. I updated and simplified the 003 patch. Patch attached.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 54ab8edfab..6a61d83862 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -59,6 +59,8 @@ typedef struct ConnCacheEntry
bool have_error; /* have any subxacts aborted in
this xact? */
bool changing_xact_state; /* xact state change in process
*/
bool invalidated; /* true if reconnect is pending */
+ bool keep_connections; /* setting value of
keep_connections
+ *
server option */
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 */
@@ -286,6 +288,7 @@ static void
make_new_connection(ConnCacheEntry *entry, UserMapping *user)
{
ForeignServer *server = GetForeignServer(user->serverid);
+ ListCell *lc;
Assert(entry->conn == NULL);
@@ -304,6 +307,26 @@ make_new_connection(ConnCacheEntry *entry, UserMapping
*user)
ObjectIdGetDatum(user->umid));
memset(&entry->state, 0, sizeof(entry->state));
+ /*
+ * Determine whether to keep the connection that we're about to make
here
+ * open even after the transaction using it ends, so that the subsequent
+ * transactions can re-use it.
+ *
+ * It's enough to determine this only when making new connection because
+ * all the connections to the foreign server whose keep_connections
option
+ * is changed will be closed and re-made later.
+ *
+ * By default, all the connections to any foreign servers are kept open.
+ */
+ entry->keep_connections = true;
+ foreach(lc, server->options)
+ {
+ DefElem *def = (DefElem *) lfirst(lc);
+
+ if (strcmp(def->defname, "keep_connections") == 0)
+ entry->keep_connections = defGetBoolean(def);
+ }
+
/* Now try to make the connection */
entry->conn = connect_pg_server(server, user);
@@ -970,14 +993,16 @@ pgfdw_xact_callback(XactEvent event, void *arg)
entry->xact_depth = 0;
/*
- * 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.
+ * If the connection isn't in a good idle state, it is marked as
+ * invalid or keep_connections option of its server is
disabled, 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 ||
+ !entry->keep_connections)
{
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 eff7b04f11..5da68415ed 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8908,7 +8908,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, sslcrldir, 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, batch_size, async_capable
+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, sslcrldir, 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, batch_size, async_capable, keep_connections
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
@@ -9244,6 +9244,27 @@ DROP USER MAPPING FOR regress_multi_conn_user2 SERVER
loopback;
DROP ROLE regress_multi_conn_user1;
DROP ROLE regress_multi_conn_user2;
-- ===================================================================
+-- Test foreign server level option keep_connections
+-- ===================================================================
+-- By default, the connections associated with foreign server are cached i.e.
+-- keep_connections option is on. Set it to off.
+ALTER SERVER loopback OPTIONS (keep_connections 'off');
+-- connection to loopback server is closed at the end of xact
+-- as keep_connections was set to off.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- No cached connections, so no records should be output.
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name
+-------------
+(0 rows)
+
+ALTER SERVER loopback OPTIONS (SET keep_connections 'on');
+-- ===================================================================
-- batch insert
-- ===================================================================
BEGIN;
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index 530d7a66d4..f1d0c8bd41 100644
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -108,7 +108,8 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
*/
if (strcmp(def->defname, "use_remote_estimate") == 0 ||
strcmp(def->defname, "updatable") == 0 ||
- strcmp(def->defname, "async_capable") == 0)
+ strcmp(def->defname, "async_capable") == 0 ||
+ strcmp(def->defname, "keep_connections") == 0)
{
/* these accept only boolean values */
(void) defGetBoolean(def);
@@ -221,6 +222,7 @@ InitPgFdwOptions(void)
/* async_capable is available on both server and table */
{"async_capable", ForeignServerRelationId, false},
{"async_capable", ForeignTableRelationId, false},
+ {"keep_connections", ForeignServerRelationId, false},
{"password_required", UserMappingRelationId, false},
/*
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 806a5bca28..85a968f7f0 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2819,6 +2819,19 @@ DROP USER MAPPING FOR regress_multi_conn_user2 SERVER
loopback;
DROP ROLE regress_multi_conn_user1;
DROP ROLE regress_multi_conn_user2;
+-- ===================================================================
+-- Test foreign server level option keep_connections
+-- ===================================================================
+-- By default, the connections associated with foreign server are cached i.e.
+-- keep_connections option is on. Set it to off.
+ALTER SERVER loopback OPTIONS (keep_connections 'off');
+-- connection to loopback server is closed at the end of xact
+-- as keep_connections was set to off.
+SELECT 1 FROM ft1 LIMIT 1;
+-- No cached connections, so no records should be output.
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ALTER SERVER loopback OPTIONS (SET keep_connections 'on');
+
-- ===================================================================
-- batch insert
-- ===================================================================
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml
index c21e9be209..a0c6b8f0b1 100644
--- a/doc/src/sgml/postgres-fdw.sgml
+++ b/doc/src/sgml/postgres-fdw.sgml
@@ -518,6 +518,33 @@ OPTIONS (ADD password_required 'false');
</para>
</sect3>
+
+ <sect3>
+ <title>Connection Management Options</title>
+
+ <para>
+ By default all the open connections that <filename>postgres_fdw</filename>
+ established to the foreign servers are kept in local session for re-use.
+ </para>
+
+ <variablelist>
+
+ <varlistentry>
+ <term><literal>keep_connections</literal></term>
+ <listitem>
+ <para>
+ This option controls whether <filename>postgres_fdw</filename> keeps
+ the connections to the foreign server open so that the subsequent
+ queries can re-use them. It can only be specified for a foreign server.
+ The default is <literal>true</literal>. If set to
<literal>off</literal>,
+ all connections to this foreign server will be discarded at the end of
+ transaction.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ </variablelist>
+ </sect3>
</sect2>
<sect2>
@@ -605,8 +632,10 @@ postgres=# SELECT postgres_fdw_disconnect_all();
<para>
<filename>postgres_fdw</filename> establishes a connection to a
foreign server during the first query that uses a foreign table
- associated with the foreign server. This connection is kept and
- re-used for subsequent queries in the same session. However, if
+ associated with the foreign server. By default this connection
+ is kept and re-used for subsequent queries in the same session.
+ This behavior can be controlled using
+ <literal>keep_connections</literal> option for a foreign server. If
multiple user identities (user mappings) are used to access the foreign
server, a connection is established for each user mapping.
</para>
@@ -622,8 +651,10 @@ postgres=# SELECT postgres_fdw_disconnect_all();
<para>
Once a connection to a foreign server has been established,
- it's usually kept until the local or corresponding remote
+ it's by default kept until the local or corresponding remote
session exits. To disconnect a connection explicitly,
+ <literal>keep_connections</literal> option for a foreign server
+ may be disabled, or
<function>postgres_fdw_disconnect</function> and
<function>postgres_fdw_disconnect_all</function> functions
may be used. For example, these are useful to close
