From: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Katsuragi Yuta <katsuragiy@oss.nttdata.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Ted Yu <yuzhihong@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Cc: Önder Kalacı <onderkalaci@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com <Shinya11.Kato@oss.nttdata.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shubham Khanna <khannashubham1197@gmail.com>
Subject: RE: [Proposal] Add foreign-server health checks infrastructure
Date: Thu, 18 Jul 2024 10:49:04 +0000
Message-ID: <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <be9382f7-5072-4760-8b3f-31d6dffa8d62@oss.nttdata.com>
References: <TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<f94c09551891249fe316bfe55015b3e7@oss.nttdata.com>
<TYCPR01MB58700498838127C54BC9BD48F5B69@TYCPR01MB5870.jpnprd01.prod.outlook.com>
<f723a26e8c34001ae32e3399951b8066@oss.nttdata.com>
<TYAPR01MB5866F8EA3C06E1B43E42E4E4F5B79@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CALDaNm2H9ZyiPAOJCKHo5dHYv-u6u+8bv_T9PkdrvqVHnxyrmQ@mail.gmail.com>
<TYAPR01MB586622DBD4A0C3BA57136899F5B49@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<36981865ac13b6c40d53bf3f98667eee@oss.nttdata.com>
<300fccae11de8a4ed1a2d970db1a8680@oss.nttdata.com>
<TYAPR01MB5866DBFEC61E81CCE9F67142F5B99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<b492a935-6c7e-8c08-e485-3c1d64d7d10f@oss.nttdata.com>
<TYAPR01MB5866D498F593B4AD7D63C148F5939@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CAHv8RjJvxDc2U3xUfvBwirw9Wk4oq8FVggv1VUhXNw6zQU8VZQ@mail.gmail.com>
<OS3PR01MB9882E5D54A472AB0A5A098D8F58EA@OS3PR01MB9882.jpnprd01.prod.outlook.com>
<be9382f7-5072-4760-8b3f-31d6dffa8d62@oss.nttdata.com>
Dear Fujii-san,
Hi, long time no see :-).
Thanks for reviewing the patch! PSA new version.
> I've just started reviewing them.
>
> Here are the review comments for 0001 patch:
>
> Do we really need postgres_fdw_verify_connection_all()? The proposed feature
> aims to check if all postgres_fdw connections used within the transaction
> are still open. If any of those connections are closed, the transaction
> can't be committed successfully, so users can roll back immediately upon
> detecting a closed connection.
>
> However, postgres_fdw_verify_connection_all() checks all connections made in
> the session, not just those used in the current transaction. This means
> users can't determine if they should roll back the transaction based on
> its return value. Therefore, I'm concerned that
> postgres_fdw_verify_connection_all() might not be very useful. Thoughts?
Right. My primal motivation is to detect the disconnection from remotes and abort
current transaction as soon as possible. In this case, as I posted in [1],
...all() is not so helpful.
I agreed to remove the function from patch set. Done.
> Considering the purpose of this feature, wouldn't it be better to extend
> postgres_fdw_get_connections() to include a "used_in_xact" column
> (indicating whether the connection has been used in the current transaction)
> and a "closed" column (indicating whether the connection has been closed)?
> This approach might be more effective than introducing a new function
> like the postgres_fdw_verify_connection family?
>
> If it's too much to check if the connection is closed by default whenever
> calling postgres_fdw_get_connections(), we could modify it to accept
> an argument indicating whether to perform this check. Thoughts?
Sounds interesting. If we can accept to change the definition of pre-existing
function, it seems better. To keep the default behavior, an input parameter
should be added. Attached patch tried to implement.
> Here are the review comments for 0003 patch:
>
> The source comment in postgres_fdw_get_connections() should mention
> the return value user_name.
Document was updated.
> We also need to handle the case where the user mapping used by
> the connection cache has been dropped. Otherwise, this could
> lead to an error.
>
> -----------------------------
> =# BEGIN;
> =*# SELECT * FROM postgres_fdw_get_connections();
> server_name | user_name | valid
> -------------+-----------+-------
> loopback | public | t
> (1 row)
>
> =*# DROP USER MAPPING FOR public SERVER loopback ;
> =*# SELECT * FROM postgres_fdw_get_connections();
> ERROR: cache lookup failed for user mapping 16409
> -----------------------------
Fixed.
> -SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
> +SELECT server_name, valid FROM postgres_fdw_get_connections() ORDER BY
> 1;
>
> Shouldn't this test also check if the returned user_name is valid?
You meant to say that we should print the user_name, right? Done.
> + server_name | user_name | validvalid
> +-------------+------------------------
> + loopback1 | postgres | t
> + loopback2 | | f
>
> The column name "validvalid" should be "valid".
Right, fixed.
> How can we cause the record with server_name != NULL but user_name = NULL?
Now this can happen when user_name is dropped, but the example was updated.
Below contains a summary of changes.
0001:
- Instead of adding new functions, postgres_fdw_get_connections() was extended.
Some tricks were added to support old versions. I followed pg_stat_statements.c.
Attributes were added after the `valid` to preserve ordering of the old version.
- I found an inconsistency of name between source and doc,
so I unified to postgres_fdw_can_verify_connection().
- Also, test patch (0002) was combined into this.
0002:
- user_name was added after the `valid` to preserve ordering of the old version.
- GetUserMappingFromOid() is allowed to miss a tuple.
[1]: https://www.postgresql.org/message-id/TYAPR01MB58668728393648C2F7DC7C85F5399%40TYAPR01MB5866.jpnprd0...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:download all (2 files, .tar.gz)v41-0001-postgres_fdw-Allow-postgres_fdw_get_connections-.patch (application/octet-stream, 19.4K) download
From 60cd57abf3a161ccbe5c0e6cdd506fd0db965f04 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Fri, 27 Jan 2023 03:17:30 +0000
Subject: [PATCH v41 1/2] postgres_fdw: Allow postgres_fdw_get_connections() to
check connection
If requested, postgres_fdw_get_connections() can verify the status of
connections that are establieshed by postgres_fdw. This check wil be
done by polling the socket. This feature is currently available only
on systems that support the non-standard POLLRDHUP extension to the
poll system call, including Linux.
"closed" column is set to false if existing connection is not closed
by the remote peer.
---
contrib/postgres_fdw/Makefile | 2 +-
contrib/postgres_fdw/connection.c | 151 +++++++++++++++++-
.../postgres_fdw/expected/postgres_fdw.out | 65 +++++++-
contrib/postgres_fdw/meson.build | 1 +
.../postgres_fdw/postgres_fdw--1.1--1.2.sql | 22 +++
contrib/postgres_fdw/postgres_fdw.control | 2 +-
contrib/postgres_fdw/sql/postgres_fdw.sql | 57 +++++++
doc/src/sgml/postgres-fdw.sgml | 71 ++++++--
8 files changed, 344 insertions(+), 27 deletions(-)
create mode 100644 contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql
diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefileindex c1b0cad453..6d23768389 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 postgres_fdw--1.0--1.1.sql+DATA = postgres_fdw--1.0.sql postgres_fdw--1.0--1.1.sql postgres_fdw--1.1--1.2.sql
REGRESS = postgres_fdw
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.cindex 33e8054f64..edc2f5eb46 100644--- a/contrib/postgres_fdw/connection.c+++ b/contrib/postgres_fdw/connection.c@@ -12,6 +12,10 @@
*/
#include "postgres.h"
+#if HAVE_POLL_H+#include <poll.h>+#endif+
#include "access/htup_details.h"
#include "access/xact.h"
#include "catalog/pg_user_mapping.h"
@@ -107,12 +111,20 @@ static uint32 pgfdw_we_get_result = 0;
(entry)->xact_depth, (entry)->xact_depth); \
} while(0)
+enum pgfdwVersion+{+ PGFDW_V1_0 = 0,+ PGFDW_V1_2,+} pgfdwVersion;+
/*
* SQL functions
*/
PG_FUNCTION_INFO_V1(postgres_fdw_get_connections);
+PG_FUNCTION_INFO_V1(postgres_fdw_get_connections_1_2);
PG_FUNCTION_INFO_V1(postgres_fdw_disconnect);
PG_FUNCTION_INFO_V1(postgres_fdw_disconnect_all);
+PG_FUNCTION_INFO_V1(postgres_fdw_can_verify_connection);
/* prototypes of private functions */
static void make_new_connection(ConnCacheEntry *entry, UserMapping *user);
@@ -159,6 +171,12 @@ static void pgfdw_security_check(const char **keywords, const char **values,
UserMapping *user, PGconn *conn);
static bool UserMappingPasswordRequired(UserMapping *user);
static bool disconnect_cached_connections(Oid serverid);
+static Datum postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo,+ enum pgfdwVersion api_version);++/* Low layer-like functions. They are used for verifying connections. */+static int pgfdw_conn_check(PGconn *conn);+static bool pgfdw_conn_checkable(void);
/*
* Get a PGconn which can be used to execute queries on the remote PostgreSQL
@@ -1978,22 +1996,31 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
}
/*
- * List active foreign server connections.+ * Internal function for postgres_fdw_get_connections variants.
*
- * This function takes no input parameter and returns setof record made of- * following values:+ * If the api_version is 1.0, 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.
*
+ * If the version is 1.2 and later, this function takes an input parameter,+ * which indicates the need for a health check. Regarding the returned record,+ * this returns two additional values:+ * - used_in_xact - indicates whether the server has been used in a transaction+ * or not.+ * - closed - true if the local session seems to be disconnected from other+ * servers.+ *
* No records are returned when there are no cached connections at all.
*/
-Datum-postgres_fdw_get_connections(PG_FUNCTION_ARGS)+static Datum+postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo,+ enum pgfdwVersion api_version)
{
-#define POSTGRES_FDW_GET_CONNECTIONS_COLS 2+#define POSTGRES_FDW_GET_CONNECTIONS_COLS 4
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
HASH_SEQ_STATUS scan;
ConnCacheEntry *entry;
@@ -2061,12 +2088,55 @@ postgres_fdw_get_connections(PG_FUNCTION_ARGS)
values[1] = BoolGetDatum(!entry->invalidated);
+ /* Add additional attributes if the version is 1.2 or later */+ if (api_version == PGFDW_V1_2)+ {+ bool require_verify = PG_GETARG_BOOL(1);++ /* Has this server been used in the transaction? */+ values[2] = BoolGetDatum(entry->xact_depth > 0);++ /*+ * If requested and the connection is not invalidated, check the+ * status of the remote connection from the backend process and+ * return the result. Otherwise returns NULL.+ */+ if (require_verify && !entry->invalidated && entry->conn)+ {+ values[3] = BoolGetDatum(pgfdw_conn_checkable() ?+ pgfdw_conn_check(entry->conn) != 0 :+ false);+ }+ else+ nulls[3] = true;+ }+
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
}
PG_RETURN_VOID();
}
+/*+ * List active foreign server connections.+ *+ * The SQL API of this function has changed in version, which could verify the+ * status of remote connections. The actual implementation was moved to the+ * internal function, and we could switch by the api_version to support the old+ * SQL declaration.+ */+Datum+postgres_fdw_get_connections_1_2(PG_FUNCTION_ARGS)+{+ return postgres_fdw_get_connections_internal(fcinfo, PGFDW_V1_2);+}++Datum+postgres_fdw_get_connections(PG_FUNCTION_ARGS)+{+ return postgres_fdw_get_connections_internal(fcinfo, PGFDW_V1_0);+}+
/*
* Disconnect the specified cached connections.
*
@@ -2192,3 +2262,72 @@ disconnect_cached_connections(Oid serverid)
return result;
}
++/*+ * Check whether functions for verifying cached connections work well or not+ */+Datum+postgres_fdw_can_verify_connection(PG_FUNCTION_ARGS)+{+ PG_RETURN_BOOL(pgfdw_conn_checkable());+}++/*+ * Check whether the socket peer closed the connection or not.+ *+ * Returns >0 if input connection is bad or remote peer seems to be closed,+ * 0 if it is valid, and -1 if an error occurred.+ */+static int+pgfdw_conn_check(PGconn *conn)+{+ int sock = PQsocket(conn);++ if (!pgfdw_conn_checkable())+ return 0;++ if (!conn || PQstatus(conn) != CONNECTION_OK || sock == PGINVALID_SOCKET)+ return -1;++#if (defined(HAVE_POLL) && defined(POLLRDHUP))+ {+ /*+ * This platform seems to have poll(2), and can wait POLLRDHUP event.+ * So construct pollfd and directly call it.+ */+ struct pollfd input_fd;+ int result;++ input_fd.fd = sock;+ input_fd.events = POLLRDHUP;+ input_fd.revents = 0;++ do+ result = poll(&input_fd, 1, 0);+ while (result < 0 && errno == EINTR);++ if (result < 0)+ return -1;++ return input_fd.revents;+ }+#else+ /* Do not support socket checking on this platform, return 0 */+ return 0;+#endif+}++/*+ * Check whether pgfdw_conn_check() can work on this platform.+ *+ * Returns true if this can use pgfdw_conn_check(), otherwise false.+ */+static bool+pgfdw_conn_checkable(void)+{+#if (defined(HAVE_POLL) && defined(POLLRDHUP))+ return true;+#else+ return false;+#endif+}diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.outindex 0cc77190dc..4fcf73070c 100644--- a/contrib/postgres_fdw/expected/postgres_fdw.out+++ b/contrib/postgres_fdw/expected/postgres_fdw.out@@ -10435,10 +10435,10 @@ drop cascades to foreign table ft7
-- should be output as invalid connections. Also the server name for
-- loopback3 should be NULL because the server was dropped.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid --------------+-------- loopback | f- | f+ server_name | valid | used_in_xact | closed +-------------+-------+--------------+--------+ loopback | f | t | + | f | t |
(2 rows)
-- The invalid connections get closed in pgfdw_xact_callback during commit.
@@ -12257,3 +12257,60 @@ ANALYZE analyze_table;
-- cleanup
DROP FOREIGN TABLE analyze_ftable;
DROP TABLE analyze_table;
+-- ===================================================================+-- test for postgres_fdw_get_connections function with require_verify = true+-- ===================================================================+-- Disable debug_discard_caches in order to manage remote connections+SET debug_discard_caches TO '0';+-- -- The text of the error might vary across platforms, so only show SQLSTATE.+\set VERBOSITY sqlstate+-- Disconnect once and set application_name to an arbitrary value+SELECT 1 FROM postgres_fdw_disconnect_all();+ ?column? +----------+ 1+(1 row)++ALTER SERVER loopback OPTIONS (SET application_name 'healthcheck');+-- Define procedure for testing verify functions+CREATE PROCEDURE test_verify_function() AS $$+DECLARE+ can_verify boolean;+ result boolean;+BEGIN+ PERFORM 1 FROM ft1 LIMIT 1;++ -- Terminate the remote backend process+ PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity+ WHERE application_name = 'healthcheck';++ -- Check whether we can do health check on this platform+ SELECT INTO can_verify postgres_fdw_can_verify_connection();++ -- If the checking can be done on this platform, call it+ IF can_verify IS TRUE THEN+ -- Set client_min_messages to ERROR temporary because the following+ -- function only throws a WARNING on the supported platform.+ SET LOCAL client_min_messages TO ERROR;++ SELECT closed INTO result FROM postgres_fdw_get_connections(true);++ RESET client_min_messages;+ ELSE+ result = true;+ END IF;++ -- If result is TRUE, we succeeded to detect the disconnection or it could+ -- not be done on this platform. Raise an message.+ IF result IS TRUE THEN+ RAISE INFO 'postgres_fdw_get_connections could detect the disconnection, or health check cannot be used on this platform';+ END IF;+END;+$$ LANGUAGE plpgsql;+-- ..And call above function+CALL test_verify_function();+INFO: 00000+ERROR: 08006+-- Clean up+\set VERBOSITY default+RESET debug_discard_caches;diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.buildindex 2b86d8a6ee..5f60d8ef97 100644--- a/contrib/postgres_fdw/meson.build+++ b/contrib/postgres_fdw/meson.build@@ -26,6 +26,7 @@ install_data(
'postgres_fdw.control',
'postgres_fdw--1.0.sql',
'postgres_fdw--1.0--1.1.sql',
+ 'postgres_fdw--1.1--1.2.sql',
kwargs: contrib_data_args,
)
diff --git a/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql b/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sqlnew file mode 100644index 0000000000..6af10110ca--- /dev/null+++ b/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql@@ -0,0 +1,22 @@+/* contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql */++-- complain if script is sourced in psql, rather than via ALTER EXTENSION+\echo Use "ALTER EXTENSION postgres_fdw UPDATE TO '1.2'" to load this file. \quit++/* First we have to remove it from the extension */+ALTER EXTENSION postgres_fdw DROP FUNCTION postgres_fdw_get_connections ();++/* Then we can drop it */+DROP FUNCTION postgres_fdw_get_connections ();++CREATE FUNCTION postgres_fdw_get_connections+ (IN require_verify boolean DEFAULT false, OUT server_name text,+ OUT valid boolean, OUT used_in_xact boolean, OUT closed boolean)+RETURNS SETOF record+AS 'MODULE_PATHNAME', 'postgres_fdw_get_connections_1_2'+LANGUAGE C STRICT PARALLEL RESTRICTED;++CREATE FUNCTION postgres_fdw_can_verify_connection ()+RETURNS bool+AS 'MODULE_PATHNAME'+LANGUAGE C STRICT PARALLEL SAFE;diff --git a/contrib/postgres_fdw/postgres_fdw.control b/contrib/postgres_fdw/postgres_fdw.controlindex d489382064..a4b800be4f 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.1'+default_version = '1.2'
module_pathname = '$libdir/postgres_fdw'
relocatable = true
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sqlindex b57f8cfda6..fa9c4dd36f 100644--- a/contrib/postgres_fdw/sql/postgres_fdw.sql+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql@@ -4218,3 +4218,60 @@ ANALYZE analyze_table;
-- cleanup
DROP FOREIGN TABLE analyze_ftable;
DROP TABLE analyze_table;
++-- ===================================================================+-- test for postgres_fdw_get_connections function with require_verify = true+-- ===================================================================++-- Disable debug_discard_caches in order to manage remote connections+SET debug_discard_caches TO '0';++-- -- The text of the error might vary across platforms, so only show SQLSTATE.+\set VERBOSITY sqlstate++-- Disconnect once and set application_name to an arbitrary value+SELECT 1 FROM postgres_fdw_disconnect_all();+ALTER SERVER loopback OPTIONS (SET application_name 'healthcheck');++-- Define procedure for testing verify functions+CREATE PROCEDURE test_verify_function() AS $$+DECLARE+ can_verify boolean;+ result boolean;+BEGIN+ PERFORM 1 FROM ft1 LIMIT 1;++ -- Terminate the remote backend process+ PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity+ WHERE application_name = 'healthcheck';++ -- Check whether we can do health check on this platform+ SELECT INTO can_verify postgres_fdw_can_verify_connection();++ -- If the checking can be done on this platform, call it+ IF can_verify IS TRUE THEN+ -- Set client_min_messages to ERROR temporary because the following+ -- function only throws a WARNING on the supported platform.+ SET LOCAL client_min_messages TO ERROR;++ SELECT closed INTO result FROM postgres_fdw_get_connections(true);++ RESET client_min_messages;+ ELSE+ result = true;+ END IF;++ -- If result is TRUE, we succeeded to detect the disconnection or it could+ -- not be done on this platform. Raise an message.+ IF result IS TRUE THEN+ RAISE INFO 'postgres_fdw_get_connections could detect the disconnection, or health check cannot be used on this platform';+ END IF;+END;+$$ LANGUAGE plpgsql;++-- ..And call above function+CALL test_verify_function();++-- Clean up+\set VERBOSITY default+RESET debug_discard_caches;diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgmlindex 1a600382e2..fa2b50450d 100644--- a/doc/src/sgml/postgres-fdw.sgml+++ b/doc/src/sgml/postgres-fdw.sgml@@ -777,27 +777,48 @@ OPTIONS (ADD password_required 'false');
<variablelist>
<varlistentry>
- <term><function>postgres_fdw_get_connections(OUT server_name text, OUT valid boolean) returns setof record</function></term>+ <term><function>postgres_fdw_get_connections(IN require_verify boolean DEFAULT false,+ OUT server_name text, OUT valid boolean, OUT used_in_xact boolean, OUT closed boolean)+ returns setof record</function></term>
<listitem>
<para>
This function returns the foreign server names of all the open
connections that <filename>postgres_fdw</filename> established from
- the local session to the foreign servers. It also returns whether- each connection is valid or not. <literal>false</literal> is returned- if the foreign server connection is used in the current local- transaction but its foreign server or user mapping is changed or- dropped (Note that server name of an invalid connection will be- <literal>NULL</literal> if the server is dropped),- and then such invalid connection will be closed at- the end of that transaction. <literal>true</literal> is returned- otherwise. If there are no open connections, no record is returned.+ the local session to the foreign servers. It also returns whether each+ connection is valid and has been used within a transaction.+ <literal>valid</literal> is returned as <literal>false</literal> if the+ foreign server connection is used in the current local transaction but+ its foreign server or user mapping is changed or dropped (Note that+ server name of an invalid connection will be <literal>NULL</literal> if+ the server is dropped), and then such invalid connection will be closed+ at the end of that transaction. The attribute is set to <literal>true</literal>+ otherwise.+ </para>+ <para>+ If <literal>require_verify</literal> is set to <literal>true</literal>,+ the function checks the status of remote connections from the local+ session to the foreign server. This check is performed by polling the+ socket, which allows long-running transactions to be aborted sooner if+ the kernel reports that the connection is closed. This feature is+ currently available only on systems that support the non-standard+ <symbol>POLLRDHUP</symbol> extension to the <symbol>poll</symbol> system+ call, including Linux. <literal>closed</literal> attribute indicates the+ result of this check. <literal>false</literal> means the existing+ connection is not closed by the remote peer. <literal>true</literal>+ means if either checked connection has been closed.+ <literal>NULL</literal> means a valid connection to the specified foreign+ server is not established, this function is not available on this+ platform, or <literal>require_verify</literal> is <literal>false</literal>.+ </para>+ <para>+ If there are no open connections, no record is returned.
Example usage of the function:
<screen>
-postgres=# SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;- server_name | valid--------------+-------- loopback1 | t- loopback2 | f+postgres=*# SELECT * FROM postgres_fdw_get_connections(true) ORDER BY 1;+ server_name | valid | used_in_xact | closed +-------------+-------+--------------+--------+ loopback1 | t | t | f+ loopback2 | t | t | f
</screen>
</para>
</listitem>
@@ -847,6 +868,26 @@ postgres=# SELECT postgres_fdw_disconnect_all();
</para>
</listitem>
</varlistentry>
++ <varlistentry>+ <term><function>postgres_fdw_can_verify_connection() returns boolean</function></term>+ <listitem>+ <para>+ This function checks whether <function>postgres_fdw_get_connections</function>+ can verify the status of remote connections or not. This returns+ <literal>true</literal> if it can be used, otherwise returns <literal>false</literal>.+ Example usage of the function:++<screen>+postgres=# SELECT postgres_fdw_can_verify_connection();+ postgres_fdw_can_verify_connection+------------------------------------+ t+</screen>+ </para>+ </listitem>+ </varlistentry>+
</variablelist>
</sect2>
--
2.43.0
v41-0002-Extend-postgres_fdw_get_connections-to-return-us.patch (application/octet-stream, 9.4K) download
From aeeb82bee861dceb9eae3d8a46226d878c20d2cd Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 18 Jul 2024 10:11:03 +0000
Subject: [PATCH v41 2/2] Extend postgres_fdw_get_connections to return user
name
---
contrib/postgres_fdw/connection.c | 33 +++++++++++++++---
.../postgres_fdw/expected/postgres_fdw.out | 13 +++----
.../postgres_fdw/postgres_fdw--1.1--1.2.sql | 3 +-
contrib/postgres_fdw/sql/postgres_fdw.sql | 5 +--
doc/src/sgml/postgres-fdw.sgml | 12 +++----
src/backend/foreign/foreign.c | 34 +++++++++++++++++++
src/include/foreign/foreign.h | 1 +
7 files changed, 81 insertions(+), 20 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.cindex edc2f5eb46..18abb0994e 100644--- a/contrib/postgres_fdw/connection.c+++ b/contrib/postgres_fdw/connection.c@@ -2008,7 +2008,10 @@ pgfdw_finish_abort_cleanup(List *pending_entries, List *cancel_requested,
*
* If the version is 1.2 and later, this function takes an input parameter,
* which indicates the need for a health check. Regarding the returned record,
- * this returns two additional values:+ * this returns three additional values:+ * - user_name - user mapping 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.
* - used_in_xact - indicates whether the server has been used in a transaction
* or not.
* - closed - true if the local session seems to be disconnected from other
@@ -2020,7 +2023,7 @@ static Datum
postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo,
enum pgfdwVersion api_version)
{
-#define POSTGRES_FDW_GET_CONNECTIONS_COLS 4+#define POSTGRES_FDW_GET_CONNECTIONS_COLS 5
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
HASH_SEQ_STATUS scan;
ConnCacheEntry *entry;
@@ -2092,9 +2095,29 @@ postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo,
if (api_version == PGFDW_V1_2)
{
bool require_verify = PG_GETARG_BOOL(1);
+ UserMapping *user = GetUserMappingFromOid(entry->key, true);++ /*+ * The same as in the foreign server case, user mappings can also+ * be dropped in the current explicit transaction, so the same+ * check as in the server case is required.+ */+ if (user)+ values[2] = CStringGetTextDatum(MappingUserName(user->userid));+ else+ {+ /*+ * If we reach here, this entry must have been invalidated in+ * pgfdw_inval_callback, same as server case.+ */+ Assert(entry->conn && entry->xact_depth > 0 &&+ entry->invalidated);++ nulls[2] = true;+ }
/* Has this server been used in the transaction? */
- values[2] = BoolGetDatum(entry->xact_depth > 0);+ values[3] = BoolGetDatum(entry->xact_depth > 0);
/*
* If requested and the connection is not invalidated, check the
@@ -2103,12 +2126,12 @@ postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo,
*/
if (require_verify && !entry->invalidated && entry->conn)
{
- values[3] = BoolGetDatum(pgfdw_conn_checkable() ?+ values[4] = BoolGetDatum(pgfdw_conn_checkable() ?
pgfdw_conn_check(entry->conn) != 0 :
false);
}
else
- nulls[3] = true;+ nulls[4] = true;
}
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc, values, nulls);
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.outindex 4fcf73070c..ece6b4cf29 100644--- a/contrib/postgres_fdw/expected/postgres_fdw.out+++ b/contrib/postgres_fdw/expected/postgres_fdw.out@@ -10432,13 +10432,14 @@ NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to user mapping for public on server loopback3
drop cascades to foreign table ft7
-- List all the existing cached connections. loopback and loopback3
--- should be output as invalid connections. Also the server name for--- loopback3 should be NULL because the server was dropped.+-- should be output as invalid connections. Also the server name and user name+-- for loopback3 should be NULL because both server and user mapping were+-- dropped.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid | used_in_xact | closed --------------+-------+--------------+--------- loopback | f | t | - | f | t | + server_name | valid | user_name | used_in_xact | closed +-------------+-------+-----------+--------------+--------+ loopback | f | hayato | t | + | f | | t |
(2 rows)
-- The invalid connections get closed in pgfdw_xact_callback during commit.
diff --git a/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql b/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sqlindex 6af10110ca..0b477a2082 100644--- a/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql+++ b/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql@@ -11,7 +11,8 @@ DROP FUNCTION postgres_fdw_get_connections ();
CREATE FUNCTION postgres_fdw_get_connections
(IN require_verify boolean DEFAULT false, OUT server_name text,
- OUT valid boolean, OUT used_in_xact boolean, OUT closed boolean)+ OUT valid boolean, OUT user_name text, OUT used_in_xact boolean,+ OUT closed boolean)
RETURNS SETOF record
AS 'MODULE_PATHNAME', 'postgres_fdw_get_connections_1_2'
LANGUAGE C STRICT PARALLEL RESTRICTED;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sqlindex fa9c4dd36f..12d254d7f0 100644--- a/contrib/postgres_fdw/sql/postgres_fdw.sql+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql@@ -3357,8 +3357,9 @@ SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
DROP SERVER loopback3 CASCADE;
-- List all the existing cached connections. loopback and loopback3
--- should be output as invalid connections. Also the server name for--- loopback3 should be NULL because the server was dropped.+-- should be output as invalid connections. Also the server name and user name+-- for loopback3 should be NULL because both server and user mapping were+-- dropped.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
-- The invalid connections get closed in pgfdw_xact_callback during commit.
COMMIT;
diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgmlindex fa2b50450d..dab796f37f 100644--- a/doc/src/sgml/postgres-fdw.sgml+++ b/doc/src/sgml/postgres-fdw.sgml@@ -782,8 +782,8 @@ OPTIONS (ADD password_required 'false');
returns setof record</function></term>
<listitem>
<para>
- This function returns the foreign server names of all the open- connections that <filename>postgres_fdw</filename> established from+ This function returns the foreign server names and user names of all the+ open connections that <filename>postgres_fdw</filename> established from
the local session to the foreign servers. It also returns whether each
connection is valid and has been used within a transaction.
<literal>valid</literal> is returned as <literal>false</literal> if the
@@ -815,10 +815,10 @@ OPTIONS (ADD password_required 'false');
Example usage of the function:
<screen>
postgres=*# SELECT * FROM postgres_fdw_get_connections(true) ORDER BY 1;
- server_name | valid | used_in_xact | closed --------------+-------+--------------+--------- loopback1 | t | t | f- loopback2 | t | t | f+ server_name | valid | user_name | used_in_xact | closed +-------------+-------+-----------+--------------+--------+ loopback1 | t | public | t | f+ loopback2 | t | public | t | f
</screen>
</para>
</listitem>
diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.cindex f4f35728b4..c7079dd569 100644--- a/src/backend/foreign/foreign.c+++ b/src/backend/foreign/foreign.c@@ -246,6 +246,40 @@ GetUserMapping(Oid userid, Oid serverid)
}
+/*+ * GetUserMappingFromOid - look up the user mapping by its oid.+ */+UserMapping *+GetUserMappingFromOid(Oid usermappigid, bool missing_ok)+{+ HeapTuple tp;+ UserMapping *um;+ Form_pg_user_mapping umform;++ tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(usermappigid));++ if (!HeapTupleIsValid(tp))+ {+ if (!missing_ok)+ elog(ERROR, "cache lookup failed for user mapping %u",+ usermappigid);++ return NULL;+ }++ umform = ((Form_pg_user_mapping) GETSTRUCT(tp));++ um = (UserMapping *) palloc(sizeof(UserMapping));+ um->umid = umform->oid;+ um->userid = umform->umuser;+ um->serverid = umform->umserver;++ ReleaseSysCache(tp);++ return um;+}++
/*
* GetForeignTable - look up the foreign table definition by relation oid.
*/
diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.hindex 82b8153100..b3941edac0 100644--- a/src/include/foreign/foreign.h+++ b/src/include/foreign/foreign.h@@ -71,6 +71,7 @@ extern ForeignServer *GetForeignServerByName(const char *srvname,
bool missing_ok);
extern UserMapping *GetUserMapping(Oid userid, Oid serverid);
extern ForeignDataWrapper *GetForeignDataWrapper(Oid fdwid);
+extern UserMapping *GetUserMappingFromOid(Oid usermappigid, bool missing_ok);
extern ForeignDataWrapper *GetForeignDataWrapperExtended(Oid fdwid,
bits16 flags);
extern ForeignDataWrapper *GetForeignDataWrapperByName(const char *fdwname,
--
2.43.0
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: kuroda.hayato@fujitsu.com, masao.fujii@oss.nttdata.com, katsuragiy@oss.nttdata.com, vignesh21@gmail.com, yuzhihong@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, onderkalaci@gmail.com, Shinya11.Kato@oss.nttdata.com, horikyota.ntt@gmail.com, khannashubham1197@gmail.com
Subject: RE: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox