On Sat, Jan 16, 2021 at 10:36 AM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > > > Please consider the v9 patch set for further review. > > > > Thanks for updating the patch! I reviewed only 0001 patch.
I addressed the review comments and attached v10 patch set. Please consider it for further review. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
From 3cf7d7d4d7241460997530ed01c2a53508257786 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com> Date: Sun, 17 Jan 2021 12:30:22 +0530 Subject: [PATCH v10 1/3] postgres_fdw function to discard cached connections This patch introduces new function postgres_fdw_disconnect() when called with a foreign server name discards the associated connections with the server name. When called without any argument, discards all the existing cached connections. This patch also adds another function postgres_fdw_get_connections() to get the list of all cached connections by corresponding foreign server names. --- contrib/postgres_fdw/Makefile | 2 +- contrib/postgres_fdw/connection.c | 330 +++++++++++++- .../postgres_fdw/expected/postgres_fdw.out | 414 +++++++++++++++++- .../postgres_fdw/postgres_fdw--1.0--1.1.sql | 20 + contrib/postgres_fdw/postgres_fdw.control | 2 +- contrib/postgres_fdw/sql/postgres_fdw.sql | 160 +++++++ doc/src/sgml/postgres-fdw.sgml | 90 ++++ 7 files changed, 999 insertions(+), 19 deletions(-) create mode 100644 contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index ee8a80a392..c1b0cad453 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -14,7 +14,7 @@ PG_CPPFLAGS = -I$(libpq_srcdir) SHLIB_LINK_INTERNAL = $(libpq) EXTENSION = postgres_fdw -DATA = postgres_fdw--1.0.sql +DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql REGRESS = postgres_fdw diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index eaedfea9f2..b6da4899ea 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -16,12 +16,14 @@ #include "access/xact.h" #include "catalog/pg_user_mapping.h" #include "commands/defrem.h" +#include "funcapi.h" #include "mb/pg_wchar.h" #include "miscadmin.h" #include "pgstat.h" #include "postgres_fdw.h" #include "storage/fd.h" #include "storage/latch.h" +#include "utils/builtins.h" #include "utils/datetime.h" #include "utils/hsearch.h" #include "utils/inval.h" @@ -66,6 +68,7 @@ typedef struct ConnCacheEntry * Connection cache (initialized on first use) */ static HTAB *ConnectionHash = NULL; +static bool conn_cache_destroyed = false; /* for assigning cursor numbers and prepared statement numbers */ static unsigned int cursor_number = 0; @@ -74,6 +77,12 @@ static unsigned int prep_stmt_number = 0; /* tracks whether any work is needed in callback functions */ 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); static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user); @@ -95,6 +104,8 @@ 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, + bool *is_in_use); /* * Get a PGconn which can be used to execute queries on the remote PostgreSQL @@ -127,15 +138,20 @@ GetConnection(UserMapping *user, bool will_prep_stmt) HASH_ELEM | HASH_BLOBS); /* - * Register some callback functions that manage connection cleanup. - * This should be done just once in each backend. + * Register callback functions that manage connection cleanup. This + * should be done just once in each backend. We don't register the + * callbacks again, if the connection cache is destroyed at least once + * in the backend. */ - RegisterXactCallback(pgfdw_xact_callback, NULL); - RegisterSubXactCallback(pgfdw_subxact_callback, NULL); - CacheRegisterSyscacheCallback(FOREIGNSERVEROID, - pgfdw_inval_callback, (Datum) 0); - CacheRegisterSyscacheCallback(USERMAPPINGOID, - pgfdw_inval_callback, (Datum) 0); + if (!conn_cache_destroyed) + { + RegisterXactCallback(pgfdw_xact_callback, NULL); + RegisterSubXactCallback(pgfdw_subxact_callback, NULL); + CacheRegisterSyscacheCallback(FOREIGNSERVEROID, + pgfdw_inval_callback, (Datum) 0); + CacheRegisterSyscacheCallback(USERMAPPINGOID, + pgfdw_inval_callback, (Datum) 0); + } } /* Set flag that we did GetConnection during the current transaction */ @@ -1095,6 +1111,13 @@ pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue) Assert(cacheid == FOREIGNSERVEROID || cacheid == USERMAPPINGOID); + /* + * Quick exit if the cache has been destroyed in + * disconnect_cached_connections. + */ + if (!ConnectionHash) + return; + /* ConnectionHash must exist already, if we're registered */ hash_seq_init(&scan, ConnectionHash); while ((entry = (ConnCacheEntry *) hash_seq_search(&scan))) @@ -1335,3 +1358,294 @@ exit: ; *result = last_res; return timed_out; } + +/* + * List active foreign server connections. + * + * This function takes no input parameter and returns setof record made of + * following values: + * - server_name - server name of active connection. In case the foreign server + * is dropped but still the connection is active, then the server name will + * be NULL in output. + * - valid - true/false representing whether the connection is valid or not. + * Note that the connections can get invalidated in pgfdw_inval_callback. + * + * No records are returned when there are no cached connections at all. + */ +Datum +postgres_fdw_get_connections(PG_FUNCTION_ARGS) +{ + ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo; + TupleDesc tupdesc; + Tuplestorestate *tupstore; + MemoryContext per_query_ctx; + MemoryContext oldcontext; + HASH_SEQ_STATUS scan; + ConnCacheEntry *entry; + + /* check to see if caller supports us returning a tuplestore */ + if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("set-valued function called in context that cannot accept a set"))); + if (!(rsinfo->allowedModes & SFRM_Materialize)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("materialize mode required, but it is not allowed in this context"))); + + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); + + /* Build tuplestore to hold the result rows */ + per_query_ctx = rsinfo->econtext->ecxt_per_query_memory; + oldcontext = MemoryContextSwitchTo(per_query_ctx); + + tupstore = tuplestore_begin_heap(true, false, work_mem); + rsinfo->returnMode = SFRM_Materialize; + rsinfo->setResult = tupstore; + rsinfo->setDesc = tupdesc; + + MemoryContextSwitchTo(oldcontext); + + /* If cache doesn't exist, we return no records. */ + if (!ConnectionHash) + { + /* clean up and return the tuplestore */ + tuplestore_donestoring(tupstore); + + PG_RETURN_VOID(); + } + + hash_seq_init(&scan, ConnectionHash); + while ((entry = (ConnCacheEntry *) hash_seq_search(&scan))) + { + ForeignServer *server; + bits16 flags = FSV_MISSING_OK; + Datum values[2]; + bool nulls[2]; + + /* We only look for open remote connections. */ + if (!entry->conn) + continue; + + server = GetForeignServerExtended(entry->serverid, flags); + + /* + * The foreign server may have been dropped in current explicit + * transaction. It is not possible to drop the server from another + * session when the connection associated with it is in use in the + * current transaction, if tried so, the drop query in another session + * blocks until the current transaction finishes. + * + * Even though the server is dropped in the current transaction, the + * cache can still have associated active connection entry, say we call + * such connections dangling. Since we can not fetch the server name + * from system catalogues for dangling connections, instead we show + * NULL value for server name in output. + * + * We could have done better by storing the server name in the cache + * entry instead of server oid so that it could be used in the output. + * But the server name in each cache entry requires 64 bytes of memory, + * which is huge, when there are many cached connections and the use + * case i.e. dropping the foreign server within the explicit current + * transaction seems rare. So, we chose to show NULL value for server + * name in output. + * + * Such dangling connections get closed either in next use or at the + * end of current explicit transaction in pgfdw_xact_callback. + */ + if (!server) + { + /* + * If the server has been dropped in the current explicit + * transaction, then this entry would have been invalidated in + * pgfdw_inval_callback at the end of drop sever command. Note that + * this connection would not have been closed in + * pgfdw_inval_callback because it is still being used in the + * current explicit transaction. So, assert that here. + */ + Assert(entry->conn && entry->xact_depth > 0 && entry->invalidated); + + /* Show null, if no server name was found. */ + values[0] = (Datum) NULL; + nulls[0] = true; + } + else + { + values[0] = CStringGetTextDatum(server->servername); + nulls[0] = false; + } + + values[1] = BoolGetDatum(!entry->invalidated); + nulls[1] = false; + + tuplestore_putvalues(tupstore, tupdesc, values, nulls); + } + + /* clean up and return the tuplestore */ + tuplestore_donestoring(tupstore); + + 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. The cache can be destroyed when there are no active + * connections left. + * + * 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; + bool is_in_use = false; + + if (!ConnectionHash) + PG_RETURN_BOOL(result); + + if (PG_NARGS() == 1) + { + ForeignServer *server = NULL; + char *servername = NULL; + uint32 hashvalue; + + servername = text_to_cstring(PG_GETARG_TEXT_PP(0)); + server = GetForeignServerByName(servername, true); + + if (!server) + ereport(ERROR, + (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST), + errmsg("foreign server \"%s\" does not exist", servername))); + + hashvalue = GetSysCacheHashValue1(FOREIGNSERVEROID, + ObjectIdGetDatum(server->serverid)); + result = disconnect_cached_connections(hashvalue, false, &is_in_use); + + /* + * 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"))); + } + else + { + result = disconnect_cached_connections(0, true, &is_in_use); + + /* + * 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 the + * 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 closed any connection, all are in use. */ + ereport(WARNING, + (errmsg("cannot close any connection because they are still in use"))); + } + } + } + + 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 destroys the cache when there are no active connections. And + * set is_in_use flag to true when there exists at least one connection that's + * being used in the current transaction. + * + * This function returns true in the following cases if at least one connection + * is disconnected. Otherwise it returns false. + */ +static bool +disconnect_cached_connections(uint32 hashvalue, bool all, bool *is_in_use) +{ + HASH_SEQ_STATUS scan; + ConnCacheEntry *entry; + bool result = false; + bool active_conn_exists = 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; + } + } + + /* + * If at least one active connection exists in the cache, mark it so + * that we don't destroy the cache. + */ + if (entry->conn && !active_conn_exists) + active_conn_exists = true; + } + + /* + * 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. + */ + if (!active_conn_exists) + { + hash_destroy(ConnectionHash); + ConnectionHash = NULL; + conn_cache_destroyed = true; + } + + return result; +} diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index c11092f8cc..4a0078b1e7 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -13,12 +13,22 @@ DO $d$ OPTIONS (dbname '$$||current_database()||$$', port '$$||current_setting('port')||$$' )$$; + EXECUTE $$CREATE SERVER temploopback1 FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (dbname '$$||current_database()||$$', + port '$$||current_setting('port')||$$' + )$$; + EXECUTE $$CREATE SERVER temploopback2 FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (dbname '$$||current_database()||$$', + port '$$||current_setting('port')||$$' + )$$; END; $d$; CREATE USER MAPPING FOR public SERVER testserver1 OPTIONS (user 'value', password 'value'); CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2; +CREATE USER MAPPING FOR public SERVER temploopback1; +CREATE USER MAPPING FOR public SERVER temploopback2; -- =================================================================== -- create objects used through FDW loopback server -- =================================================================== @@ -129,6 +139,16 @@ CREATE FOREIGN TABLE ft6 ( c2 int NOT NULL, c3 text ) SERVER loopback2 OPTIONS (schema_name 'S 1', table_name 'T 4'); +CREATE FOREIGN TABLE templbtbl1 ( + c1 int NOT NULL, + c2 int NOT NULL, + c3 text +) SERVER temploopback1 OPTIONS (schema_name 'S 1', table_name 'T 4'); +CREATE FOREIGN TABLE templbtbl2 ( + c1 int NOT NULL, + c2 int NOT NULL, + c3 text +) SERVER temploopback2 OPTIONS (schema_name 'S 1', table_name 'T 4'); -- =================================================================== -- tests for validator -- =================================================================== @@ -191,15 +211,17 @@ ALTER FOREIGN TABLE ft2 OPTIONS (schema_name 'S 1', table_name 'T 1'); ALTER FOREIGN TABLE ft1 ALTER COLUMN c1 OPTIONS (column_name 'C 1'); ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS (column_name 'C 1'); \det+ - List of foreign tables - Schema | Table | Server | FDW options | Description ---------+-------+-----------+---------------------------------------+------------- - public | ft1 | loopback | (schema_name 'S 1', table_name 'T 1') | - public | ft2 | loopback | (schema_name 'S 1', table_name 'T 1') | - public | ft4 | loopback | (schema_name 'S 1', table_name 'T 3') | - public | ft5 | loopback | (schema_name 'S 1', table_name 'T 4') | - public | ft6 | loopback2 | (schema_name 'S 1', table_name 'T 4') | -(5 rows) + List of foreign tables + Schema | Table | Server | FDW options | Description +--------+------------+---------------+---------------------------------------+------------- + public | ft1 | loopback | (schema_name 'S 1', table_name 'T 1') | + public | ft2 | loopback | (schema_name 'S 1', table_name 'T 1') | + public | ft4 | loopback | (schema_name 'S 1', table_name 'T 3') | + public | ft5 | loopback | (schema_name 'S 1', table_name 'T 4') | + public | ft6 | loopback2 | (schema_name 'S 1', table_name 'T 4') | + public | templbtbl1 | temploopback1 | (schema_name 'S 1', table_name 'T 4') | + public | templbtbl2 | temploopback2 | (schema_name 'S 1', table_name 'T 4') | +(7 rows) -- Test that alteration of server options causes reconnection -- Remote's errors might be non-English, so hide them to ensure stable results @@ -9053,3 +9075,377 @@ SELECT 1 FROM ft1 LIMIT 1; ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off'); -- The invalid connection gets closed in pgfdw_xact_callback during commit. COMMIT; +-- ======================================================================== +-- Test postgres_fdw_get_connections and postgres_fdw_disconnect functions +-- ======================================================================== +-- Discard all existing connections, before starting the tests. +SELECT postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Ensure to have temploopback1 and temploopback2 connections cached. +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +-- List all the existing cached connections. Should output temploopback1, +-- temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +-- temploopback1 connection is disconnected. temploopback2 connection still +-- exists. +SELECT postgres_fdw_disconnect('temploopback1'); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- List all the existing cached connections. Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback2 | t +(1 row) + +-- Cache exists, but the temploopback1 connection is not present in it. +SELECT postgres_fdw_disconnect('temploopback1'); /* FALSE */ + postgres_fdw_disconnect +------------------------- + f +(1 row) + +-- Cache exists, but the server name provided doesn't exist. +SELECT postgres_fdw_disconnect('unknownserver'); /* ERROR */ +ERROR: foreign server "unknownserver" does not exist +-- Cache and temploopback2 connection exist, so discard it. +SELECT postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Cache does not exist. +SELECT postgres_fdw_disconnect(); /* FALSE */ + postgres_fdw_disconnect +------------------------- + f +(1 row) + +-- Test the functions inside explicit xact. +-- Connections are being used in the xact, so they cannot be disconnected. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +SELECT * FROM postgres_fdw_disconnect('temploopback1'); /* WARNING and FALSE */ +WARNING: cannot close the connection because it is still in use + postgres_fdw_disconnect +------------------------- + f +(1 row) + +SELECT * FROM postgres_fdw_disconnect(); /* WARNING and FALSE */ +WARNING: cannot close any connection because they are still in use + postgres_fdw_disconnect +------------------------- + f +(1 row) + +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +COMMIT; +-- Should disconnect temploopback1, temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Connections can be closed in the xact because they are not in use. +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +BEGIN; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Should output nothing. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +-------------+------- +(0 rows) + +COMMIT; +-- temploopback1 connection is closed and temploopback2 is not, because it's +-- being used in the xact. +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +BEGIN; +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +SELECT * FROM postgres_fdw_disconnect('temploopback1'); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback2 | t +(1 row) + +COMMIT; +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- temploopback1 connection is closed and temploopback2 is not, because it's +-- being used in the xact. +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +BEGIN; +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +SELECT * FROM postgres_fdw_disconnect(); /* WARNING and TRUE */ +WARNING: cannot close all connections because some of them are still in use + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback2 | t +(1 row) + +COMMIT; +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- temploopback1 connection is invalidated and temploopback2 is not. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +ALTER SERVER temploopback1 OPTIONS (ADD use_remote_estimate 'off'); +-- Should output temploopback1 as invalid, temploopback2 as valid. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | f + temploopback2 | t +(2 rows) + +COMMIT; +-- Invalidated connection i.e. temploopback1 was closed at the end of the xact. +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback2 | t +(1 row) + +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + t +(1 row) + +-- Both temploopback1 and temploopback2 connections are invalidated. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +ALTER SERVER temploopback1 OPTIONS (SET use_remote_estimate 'off'); +ALTER SERVER temploopback2 OPTIONS (ADD use_remote_estimate 'off'); +-- Should output both temploopback1 and temploopback2 as invalid. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | f + temploopback2 | f +(2 rows) + +COMMIT; +-- Invalidated connections i.e. temploopback1 and temploopback2 were closed at +-- the end of the xact. Should output nothing. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +-------------+------- +(0 rows) + +-- No active connections. +SELECT * FROM postgres_fdw_disconnect(); /* FALSE */ + postgres_fdw_disconnect +------------------------- + f +(1 row) + +-- Both the servers were dropped inside the xact block, so a warning is +-- emitted. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +SELECT 1 FROM templbtbl2 LIMIT 1; + ?column? +---------- + 1 +(1 row) + +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback1 | t + temploopback2 | t +(2 rows) + +DROP SERVER temploopback1 CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to user mapping for public on server temploopback1 +drop cascades to foreign table templbtbl1 +-- Should output temploopback2 and null server name with valid false. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +---------------+------- + temploopback2 | t + | f +(2 rows) + +DROP SERVER temploopback2 CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to user mapping for public on server temploopback2 +drop cascades to foreign table templbtbl2 +-- Should output 2 rows of each null server name with valid false. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +-------------+------- + | f + | f +(2 rows) + +COMMIT; diff --git a/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql new file mode 100644 index 0000000000..b616316999 --- /dev/null +++ b/contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql @@ -0,0 +1,20 @@ +/* contrib/postgres_fdw/postgres_fdw--1.0--1.1.sql */ + +-- complain if script is sourced in psql, rather than via ALTER EXTENSION +\echo Use "ALTER EXTENSION postgres_fdw UPDATE TO '1.1'" to load this file. \quit + +CREATE FUNCTION postgres_fdw_get_connections (OUT server_name text, + OUT valid boolean) +RETURNS SETOF record +AS 'MODULE_PATHNAME','postgres_fdw_get_connections' +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/postgres_fdw.control b/contrib/postgres_fdw/postgres_fdw.control index f9ed490752..d489382064 100644 --- a/contrib/postgres_fdw/postgres_fdw.control +++ b/contrib/postgres_fdw/postgres_fdw.control @@ -1,5 +1,5 @@ # postgres_fdw extension comment = 'foreign-data wrapper for remote PostgreSQL servers' -default_version = '1.0' +default_version = '1.1' module_pathname = '$libdir/postgres_fdw' relocatable = true diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 25dbc08b98..3465cc0f56 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -15,6 +15,14 @@ DO $d$ OPTIONS (dbname '$$||current_database()||$$', port '$$||current_setting('port')||$$' )$$; + EXECUTE $$CREATE SERVER temploopback1 FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (dbname '$$||current_database()||$$', + port '$$||current_setting('port')||$$' + )$$; + EXECUTE $$CREATE SERVER temploopback2 FOREIGN DATA WRAPPER postgres_fdw + OPTIONS (dbname '$$||current_database()||$$', + port '$$||current_setting('port')||$$' + )$$; END; $d$; @@ -22,6 +30,8 @@ CREATE USER MAPPING FOR public SERVER testserver1 OPTIONS (user 'value', password 'value'); CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2; +CREATE USER MAPPING FOR public SERVER temploopback1; +CREATE USER MAPPING FOR public SERVER temploopback2; -- =================================================================== -- create objects used through FDW loopback server @@ -142,6 +152,17 @@ CREATE FOREIGN TABLE ft6 ( c3 text ) SERVER loopback2 OPTIONS (schema_name 'S 1', table_name 'T 4'); +CREATE FOREIGN TABLE templbtbl1 ( + c1 int NOT NULL, + c2 int NOT NULL, + c3 text +) SERVER temploopback1 OPTIONS (schema_name 'S 1', table_name 'T 4'); + +CREATE FOREIGN TABLE templbtbl2 ( + c1 int NOT NULL, + c2 int NOT NULL, + c3 text +) SERVER temploopback2 OPTIONS (schema_name 'S 1', table_name 'T 4'); -- =================================================================== -- tests for validator -- =================================================================== @@ -2711,3 +2732,142 @@ SELECT 1 FROM ft1 LIMIT 1; ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off'); -- The invalid connection gets closed in pgfdw_xact_callback during commit. COMMIT; + +-- ======================================================================== +-- Test postgres_fdw_get_connections and postgres_fdw_disconnect functions +-- ======================================================================== + +-- Discard all existing connections, before starting the tests. +SELECT postgres_fdw_disconnect(); /* TRUE */ + +-- Ensure to have temploopback1 and temploopback2 connections cached. +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; + +-- List all the existing cached connections. Should output temploopback1, +-- temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + +-- temploopback1 connection is disconnected. temploopback2 connection still +-- exists. +SELECT postgres_fdw_disconnect('temploopback1'); /* TRUE */ + +-- List all the existing cached connections. Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + +-- Cache exists, but the temploopback1 connection is not present in it. +SELECT postgres_fdw_disconnect('temploopback1'); /* FALSE */ + +-- Cache exists, but the server name provided doesn't exist. +SELECT postgres_fdw_disconnect('unknownserver'); /* ERROR */ + +-- Cache and temploopback2 connection exist, so discard it. +SELECT postgres_fdw_disconnect(); /* TRUE */ + +-- Cache does not exist. +SELECT postgres_fdw_disconnect(); /* FALSE */ + +-- Test the functions inside explicit xact. +-- Connections are being used in the xact, so they cannot be disconnected. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +SELECT * FROM postgres_fdw_disconnect('temploopback1'); /* WARNING and FALSE */ +SELECT * FROM postgres_fdw_disconnect(); /* WARNING and FALSE */ +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- Should disconnect temploopback1, temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + +-- Connections can be closed in the xact because they are not in use. +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; +BEGIN; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ +-- Should output nothing. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- temploopback1 connection is closed and temploopback2 is not, because it's +-- being used in the xact. +SELECT 1 FROM templbtbl1 LIMIT 1; +BEGIN; +SELECT 1 FROM templbtbl2 LIMIT 1; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +SELECT * FROM postgres_fdw_disconnect('temploopback1'); /* TRUE */ +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + +-- temploopback1 connection is closed and temploopback2 is not, because it's +-- being used in the xact. +SELECT 1 FROM templbtbl1 LIMIT 1; +BEGIN; +SELECT 1 FROM templbtbl2 LIMIT 1; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +SELECT * FROM postgres_fdw_disconnect(); /* WARNING and TRUE */ +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + +-- temploopback1 connection is invalidated and temploopback2 is not. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; +ALTER SERVER temploopback1 OPTIONS (ADD use_remote_estimate 'off'); +-- Should output temploopback1 as invalid, temploopback2 as valid. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- Invalidated connection i.e. temploopback1 was closed at the end of the xact. +-- Should output temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + +-- Should disconnect temploopback2. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + +-- Both temploopback1 and temploopback2 connections are invalidated. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; +ALTER SERVER temploopback1 OPTIONS (SET use_remote_estimate 'off'); +ALTER SERVER temploopback2 OPTIONS (ADD use_remote_estimate 'off'); +-- Should output both temploopback1 and temploopback2 as invalid. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; + +-- Invalidated connections i.e. temploopback1 and temploopback2 were closed at +-- the end of the xact. Should output nothing. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + +-- No active connections. +SELECT * FROM postgres_fdw_disconnect(); /* FALSE */ + +-- Both the servers were dropped inside the xact block, so a warning is +-- emitted. +BEGIN; +SELECT 1 FROM templbtbl1 LIMIT 1; +SELECT 1 FROM templbtbl2 LIMIT 1; +-- Should output temploopback1, temploopback2. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +DROP SERVER temploopback1 CASCADE; +-- Should output temploopback2 and null server name with valid false. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +DROP SERVER temploopback2 CASCADE; +-- Should output 2 rows of each null server name with valid false. +SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; +COMMIT; diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index e6fd2143c1..1da9599f59 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -479,6 +479,82 @@ OPTIONS (ADD password_required 'false'); </sect3> </sect2> +<sect2> + <title>Functions</title> + + <variablelist> + <varlistentry> + <term><function>postgres_fdw_get_connections(OUT server_name text, OUT valid boolean) returns setof record</function></term> + <listitem> + <para> + When called in the local session, it returns set of records made of the + foreign server names of all the open connections that are previously made + to the foreign servers and <literal>true</literal> or <literal>false</literal> + to show whether or not the connection is valid. The foreign server + connections can get invalidated due to alter statements on foreign server + or user mapping such connections get closed at the end of transaction. If + there are no open connections, then no record is returned. This function + issues a warning in case for any connection, the associated foreign + server has been dropped and the server name can not be fetched from the + system catalogues. Example usage of the function: + <screen> +postgres=# SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; + server_name | valid +-------------+------- + loopback1 | t + loopback2 | f +</screen> + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><function>postgres_fdw_disconnect(IN servername text) returns boolean</function></term> + <listitem> + <para> + When called in the local session with the 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 the current transaction, it is not discarded, instead + 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: + <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> + <sect2> <title>Connection Management</title> @@ -490,6 +566,20 @@ OPTIONS (ADD password_required 'false'); 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 ddaaa288e7115457d32420bd177435a98790514f Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com> Date: Sat, 16 Jan 2021 11:13:58 +0530 Subject: [PATCH v10 2/3] 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 | 50 +++++++++++++++++++ contrib/postgres_fdw/postgres_fdw.c | 16 ++++++ contrib/postgres_fdw/postgres_fdw.h | 3 ++ contrib/postgres_fdw/sql/postgres_fdw.sql | 30 +++++++++++ doc/src/sgml/postgres-fdw.sgml | 44 +++++++++++++++- 6 files changed, 151 insertions(+), 4 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 396ae63415..260e956195 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -815,6 +815,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) @@ -825,6 +826,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); @@ -959,16 +962,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 4a0078b1e7..a655c7025f 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9449,3 +9449,53 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; (2 rows) COMMIT; +-- =================================================================== +-- 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) + +-- Discard loopback connection. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + postgres_fdw_disconnect +------------------------- + 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 3465cc0f56..8a16dc40aa 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2871,3 +2871,33 @@ DROP SERVER temploopback2 CASCADE; -- Should output 2 rows of each null server name with valid false. SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; COMMIT; + +-- =================================================================== +-- 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; + +-- Discard loopback connection. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 1da9599f59..49344efff2 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 a67378b0f0d154e0c59381e39d7f0edef90dd064 Mon Sep 17 00:00:00 2001 From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com> Date: Sat, 16 Jan 2021 11:15:42 +0530 Subject: [PATCH v10 3/3] 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 | 35 ++++++++++++--- .../postgres_fdw/expected/postgres_fdw.out | 44 ++++++++++++++++++- contrib/postgres_fdw/option.c | 9 +++- contrib/postgres_fdw/sql/postgres_fdw.sql | 26 +++++++++++ doc/src/sgml/postgres-fdw.sgml | 43 ++++++++++++++++++ 5 files changed, 150 insertions(+), 7 deletions(-) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 260e956195..1e305312f7 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; /* @@ -125,6 +127,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) @@ -267,6 +271,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; @@ -291,6 +304,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)); @@ -962,15 +976,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); @@ -1124,7 +1141,15 @@ pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue) if (!ConnectionHash) return; - /* ConnectionHash must exist already, if we're registered */ + /* + * Since we can discard ConnectionHash with postgres_fdw_disconnect, we may + * have a NULL ConnectionHash. So return in that case. We do not need to + * invalidate the cache entries as the cache itself would have discarded + * with postgres_fdw_disconnect. + */ + if (!ConnectionHash) + return; + hash_seq_init(&scan, ConnectionHash); while ((entry = (ConnCacheEntry *) hash_seq_search(&scan))) { diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index a655c7025f..1057e44a6a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8933,7 +8933,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 @@ -9499,3 +9499,45 @@ SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ 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) + +-- Discard loopback connection. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + 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 8a16dc40aa..c9b780abe8 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -2901,3 +2901,29 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; -- Discard loopback connection. SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ + +-- =================================================================== +-- 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; + +-- Discard loopback connection. +SELECT * FROM postgres_fdw_disconnect(); /* TRUE */ diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 49344efff2..f554351dd0 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