From e5f5f8a10b163093abf96dda4d64ddbe9691afbf Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy Date: Mon, 22 Feb 2021 11:21:10 +0530 Subject: [PATCH v21] postgres_fdw server level option, keep_connections to not cache connection This patch adds a new server level option, keep_connections, 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 | 58 +++++++++++++++++-- .../postgres_fdw/expected/postgres_fdw.out | 24 +++++++- contrib/postgres_fdw/option.c | 9 ++- contrib/postgres_fdw/sql/postgres_fdw.sql | 13 +++++ doc/src/sgml/postgres-fdw.sgml | 47 +++++++++++++++ 5 files changed, 145 insertions(+), 6 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 3325ce00f2..0c5408dfe2 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -48,6 +48,17 @@ */ typedef Oid ConnCacheKey; +/* + * Represents whether or not the keep_connections server level option is + * provided by users with the foreign server. + */ +typedef enum KeepConnSrvOpt +{ + KEEP_CONN_NOT_SPECFIEID = 0, /* option not provided */ + KEEP_CONN_SPECIFIED_TRUE = 1, /* option provided with value true */ + KEEP_CONN_SPECIFIED_FALSE = 2 /* option provided with value false */ +} KeepConnSrvOpt; + typedef struct ConnCacheEntry { ConnCacheKey key; /* hash key (must be first) */ @@ -62,6 +73,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 */ + /* Holds the server level keep_connections option info. */ + KeepConnSrvOpt keep_conn_srv_opt; } ConnCacheEntry; /* @@ -124,6 +137,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) @@ -261,6 +276,24 @@ GetConnection(UserMapping *user, bool will_prep_stmt) begin_remote_xact(entry); } + server = GetForeignServer(user->serverid); + entry->keep_conn_srv_opt = KEEP_CONN_NOT_SPECFIEID; + + foreach(lc, server->options) + { + DefElem *def = (DefElem *) lfirst(lc); + + if (strcmp(def->defname, "keep_connections") == 0) + { + bool opt_val = defGetBoolean(def); + + if (opt_val) + entry->keep_conn_srv_opt = KEEP_CONN_SPECIFIED_TRUE; + else + entry->keep_conn_srv_opt = KEEP_CONN_SPECIFIED_FALSE; + } + } + /* Remember if caller will prepare statements */ entry->have_prep_stmt |= will_prep_stmt; @@ -285,6 +318,7 @@ make_new_connection(ConnCacheEntry *entry, UserMapping *user) entry->changing_xact_state = false; entry->invalidated = false; entry->serverid = server->serverid; + entry->keep_conn_srv_opt = KEEP_CONN_NOT_SPECFIEID; entry->server_hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID, ObjectIdGetDatum(server->serverid)); @@ -810,6 +844,7 @@ pgfdw_xact_callback(XactEvent event, void *arg) { PGresult *res; bool used_in_current_xact = false; + bool discard_conn = false; /* Ignore cache entry if no open connection right now */ if (entry->conn == NULL) @@ -954,21 +989,36 @@ pgfdw_xact_callback(XactEvent event, void *arg) /* Reset state to show we're out of a transaction */ entry->xact_depth = 0; + /* + * Check if user wants to discard open connection used in this xact. + * See keep_connections server level option if specified, otherwise use + * keep_connections GUC. Note that the server level option overrides + * the GUC. + */ + if (entry->conn && used_in_current_xact) + { + if ((entry->keep_conn_srv_opt == KEEP_CONN_NOT_SPECFIEID && + !keep_connections) || + entry->keep_conn_srv_opt == KEEP_CONN_SPECIFIED_FALSE) + discard_conn = true; + + used_in_current_xact = false; + } + /* * 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 + * invalid or user wants to discard open connection used in this 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 || - (used_in_current_xact && !keep_connections)) + discard_conn) { 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 fd0c3109f1..995f474db4 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8946,7 +8946,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 +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, 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 @@ -9309,6 +9309,28 @@ SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1; ------------- (0 rows) +RESET postgres_fdw.keep_connections; +-- =================================================================== +-- 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'); +-- loopback server connection is closed by the local session at the end of xact +-- as the 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 -- =================================================================== diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 64698c4da3..ecd12b47c1 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_connections") == 0) { /* these accept only boolean values */ (void) defGetBoolean(def); @@ -227,6 +228,12 @@ InitPgFdwOptions(void) {"sslcert", UserMappingRelationId, true}, {"sslkey", UserMappingRelationId, true}, + /* + * If true, cache connections associated with this server, otherwise + * remove them at the end of the xact. Default is true. + */ + {"keep_connections", ForeignServerRelationId, false}, + {NULL, InvalidOid, false} }; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 310f89288a..6a998f895c 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2834,6 +2834,19 @@ 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; +RESET postgres_fdw.keep_connections; +-- =================================================================== +-- 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'); +-- loopback server connection is closed by the local session at the end of xact +-- as the 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 838b9a0084..8543565450 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -490,6 +490,38 @@ OPTIONS (ADD password_required 'false'); + + + Connection Management Options + + + By default the foreign server connections made with + postgres_fdw are kept in local session for re-use. + This may be overridden using the following + option: + + + + + + keep_connections + + + This option controls whether postgres_fdw keeps the + foreign server connections that are made within a local session. It can + be specified for each foreign server. If specified as off, + the associated foreign server connections are discarded at the end of + the transaction, otherwise the connections are kept by the local session. + If it's not specified at all, then the postgres_fdw.keep_connections + configuration parameter is used to decide whether to keep or discard + the foreign server connections. + + + + + + + @@ -594,6 +626,14 @@ postgres=# SELECT postgres_fdw_disconnect_all(); connection is discarded at the end of transaction in which it is used. + + The server level keep_connections option overrides the + postgres_fdw.keep_connections configuration parameter + that is, the postgres_fdw.keep_connections + configuration parameter will be checked only if the server level option + is not specified by the user for a foreign server. + + Note that setting postgres_fdw.keep_connections to off does not discard any previously made and still open @@ -647,6 +687,13 @@ postgres=# SELECT postgres_fdw_disconnect_all(); set to off, each connection is discarded at the end of local transaction in which it is used. + + + Use level option keep_connections + so that the local session doesn't keep the remote connection that is made to + the foreign server. Default being on, when set to off, + the connection is discarded at the end of local transaction in which it is used. + -- 2.25.1