public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v19 03/10] Add tests on pg_ls_dir before changing it 4+ messages / 3 participants [nested] [flat]
* [PATCH v19 03/10] Add tests on pg_ls_dir before changing it @ 2020-03-17 18:16 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Justin Pryzby @ 2020-03-17 18:16 UTC (permalink / raw) --- src/test/regress/expected/misc_functions.out | 18 ++++++++++++++++++ src/test/regress/sql/misc_functions.sql | 5 +++++ 2 files changed, 23 insertions(+) diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index d3acb98d04..2e87c548eb 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -201,6 +201,24 @@ select count(*) > 0 from t (1 row) +select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true + name +------ + . +(1 row) + +select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false + name +------ +(0 rows) + +select pg_ls_dir('does not exist', true, false); -- ok with missingok=true + pg_ls_dir +----------- +(0 rows) + +select pg_ls_dir('does not exist'); -- fails with missingok=false +ERROR: could not open directory "does not exist": No such file or directory -- -- Test adding a support function to a subject function -- diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index 094e8f8296..f6857ad177 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -60,6 +60,11 @@ select count(*) > 0 from where spcname = 'pg_default') pts join pg_database db on pts.pts = db.oid; +select * from (select pg_ls_dir('.', false, true) as name) as ls where ls.name='.'; -- include_dot_dirs=true +select * from (select pg_ls_dir('.', false, false) as name) as ls where ls.name='.'; -- include_dot_dirs=false +select pg_ls_dir('does not exist', true, false); -- ok with missingok=true +select pg_ls_dir('does not exist'); -- fails with missingok=false + -- -- Test adding a support function to a subject function -- -- 2.17.0 --S0GG+JvAI2G0KxBG Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v19-0004-Add-pg_ls_dir_metadata-to-list-a-dir-with-file-m.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [Proposal] Add foreign-server health checks infrastructure @ 2023-03-01 02:34 Katsuragi Yuta <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Katsuragi Yuta @ 2023-03-01 02:34 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: 'Ted Yu' <[email protected]>; Tom Lane <[email protected]>; vignesh C <[email protected]>; [email protected]; Önder Kalacı <[email protected]>; Fujii Masao <[email protected]>; [email protected]; Kyotaro Horiguchi <[email protected]> Hi Kuroda-san, Thank you for updating the patch! >> I rethought the pqSocketPoll part. Current interpretation of >> arguments seems a little bit confusing because a specific pattern >> of arguments has a different meaning. What do you think about >> introducing a new argument like `int forConnCheck`? This seems >> straightforward and readable. > > I think it may be better, so fixed. > But now we must consider another thing - will we support combination > of conncheck > and {read|write} or timeout? Especially about timeout, if someone > calls pqSocketPoll() > with forConnCheck = 1 and end_time = -1, the process may be stuck > because it waits > till socket peer closed connection. > Currently the forConnCheck can be specified with other request, but > timeout must be zero. Yes, we need to consider these. I'm wondering whether we need a special care for the combination of event and timeout. Surely, if forConnCheck is set and end_time = -1, pqSocketPoll blocks until the connection close. However, the behavior matches the meaning of the arguments and does not seem confusing (also not an error state). Do we need to restrict this kind of usage in the pqSocketPoll side? I think like the following might be fine. ``` if (!forRead && !forWrite) { if (!(forConnCheck && PQconnCheckable())) return 0; } ``` > While making the patch, I come up with idea that > postgres_fdw_verify_connection_states* > returns NULL if PQconnCheck() cannot work on this platform. This can be > done by > adding PQconnCheckable(). It may be reasonable because the checking is > not really > done on this environment. How do you think? I agree with you. Followings are comments for v33. Please check. 0001: 1. the comment of PQconnCheck +/* + * Check whether the socket peer closed connection or not. + * Check whether the socket peer closed 'the' connection or not? 2. the comment of pqSocketCheck - * or both. Returns >0 if one or more conditions are met, 0 if it timed - * out, -1 if an error occurred. + * or both. Moreover, this function can check the health of socket on some + * limited platforms if end_time is 0. the health of socket -> the health of the connection? if end_time is 0 -> if forConnCehck is specified? 3. the comment of pqSocketPoll - * If neither forRead nor forWrite are set, immediately return a timeout - * condition (without waiting). Return >0 if condition is met, 0 - * if a timeout occurred, -1 if an error or interrupt occurred. + * Moreover, this function can check the health of socket on some limited + * platforms if end_time is 0. the health of socket -> the health of the connection? if end_time is 0 -> if forConnCehck is specified? 4. the code of pqSocketPoll +#if defined(POLLRDHUP) + if (forConnCheck) + input_fd.events |= POLLRDHUP; +#endif I think it is better to use PQconnCheckable() to remove the macro. 5. the code of pqSocketCheck and the definition of pqSocketPoll - result = pqSocketPoll(conn->sock, forRead, forWrite, end_time); + result = pqSocketPoll(conn->sock, forRead, forWrite, forConnCheck, end_time); -pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) +pqSocketPoll(int sock, int forRead, int forWrite, int forConnCheck, time_t end_time) Should these be divided into two lines? 6. the comment of verify_cached_connections + * This function emits warnings if a disconnection is found, and this returns + * false only when the lastly verified server seems to be disconnected. It seems better to write the case where this function returns true. 7. the comment of postgres_fdw_verify_connection_states + * This function emits a warning if a disconnection is found. This returns + * false only when the verified server seems to be disconnected, and reutrns + * NULL if the connection check had not been done. It seems better to write the case where this function returns true. 8. the code of + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("%s", str.data), + errdetail("Socket close is detected."), + errhint("Plsease check the health of server."))); Is it better to use "Connection close is detected" rather than "Socket close is detected"? 9. the document of postgres_fdw The document of postgres_fdw_verify_connection_states_* is a little bit old. Could you update it? regards, -- Katsuragi Yuta Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 4+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure @ 2023-03-03 12:24 Hayato Kuroda (Fujitsu) <[email protected]> parent: Katsuragi Yuta <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2023-03-03 12:24 UTC (permalink / raw) To: 'Katsuragi Yuta' <[email protected]>; +Cc: 'Ted Yu' <[email protected]>; Tom Lane <[email protected]>; vignesh C <[email protected]>; [email protected] <[email protected]>; Önder Kalacı <[email protected]>; Fujii Masao <[email protected]>; [email protected] <[email protected]>; Kyotaro Horiguchi <[email protected]> Hi Katsuragi-san, Thank you for reviewing! PSA new version. > >> I rethought the pqSocketPoll part. Current interpretation of > >> arguments seems a little bit confusing because a specific pattern > >> of arguments has a different meaning. What do you think about > >> introducing a new argument like `int forConnCheck`? This seems > >> straightforward and readable. > > > > I think it may be better, so fixed. > > But now we must consider another thing - will we support combination > > of conncheck > > and {read|write} or timeout? Especially about timeout, if someone > > calls pqSocketPoll() > > with forConnCheck = 1 and end_time = -1, the process may be stuck > > because it waits > > till socket peer closed connection. > > Currently the forConnCheck can be specified with other request, but > > timeout must be zero. > > Yes, we need to consider these. > I'm wondering whether we need a special care for the combination > of event and timeout. Surely, if forConnCheck is set and end_time = -1, > pqSocketPoll blocks until the connection close. However, the > behavior matches the meaning of the arguments and does not seem > confusing (also not an error state). Do we need to restrict this > kind of usage in the pqSocketPoll side? I think like the following > might be fine. > > ``` > if (!forRead && !forWrite) > { > if (!(forConnCheck && PQconnCheckable())) > return 0; > } > ``` Seems right, I was too pessimistic. Fixed. > > While making the patch, I come up with idea that > > postgres_fdw_verify_connection_states* > > returns NULL if PQconnCheck() cannot work on this platform. This can be > > done by > > adding PQconnCheckable(). It may be reasonable because the checking is > > not really > > done on this environment. How do you think? > > I agree with you. Changed. > Followings are comments for v33. Please check. > 0001: > 1. the comment of PQconnCheck > +/* > + * Check whether the socket peer closed connection or not. > + * > > Check whether the socket peer closed 'the' connection or not? Changed. > 2. the comment of pqSocketCheck > - * or both. Returns >0 if one or more conditions are met, 0 if it > timed > - * out, -1 if an error occurred. > + * or both. Moreover, this function can check the health of socket on > some > + * limited platforms if end_time is 0. > > the health of socket -> the health of the connection? > if end_time is 0 -> if forConnCehck is specified? Fixed. > 3. the comment of pqSocketPoll > - * If neither forRead nor forWrite are set, immediately return a > timeout > - * condition (without waiting). Return >0 if condition is met, 0 > - * if a timeout occurred, -1 if an error or interrupt occurred. > + * Moreover, this function can check the health of socket on some > limited > + * platforms if end_time is 0. > > the health of socket -> the health of the connection? > if end_time is 0 -> if forConnCehck is specified? Fixed. > 4. the code of pqSocketPoll > +#if defined(POLLRDHUP) > + if (forConnCheck) > + input_fd.events |= POLLRDHUP; > +#endif > > I think it is better to use PQconnCheckable() to remove the macro. IIUC the macro is needed. In FreeBSD, macOS and other platforms do not have the macro POLLRDHUP so they cannot compile. I checked by my CI and got following error. ``` ... FAILED: src/interfaces/libpq/libpq.a.p/fe-misc.c.o ... ../src/interfaces/libpq/fe-misc.c:1149:22: error: use of undeclared identifier 'POLLRDHUP' input_fd.events |= POLLRDHUP; ```` It must be invisible from them. > 5. the code of pqSocketCheck and the definition of pqSocketPoll > - result = pqSocketPoll(conn->sock, forRead, forWrite, > end_time); > + result = pqSocketPoll(conn->sock, forRead, forWrite, > forConnCheck, > end_time); > > -pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) > +pqSocketPoll(int sock, int forRead, int forWrite, int forConnCheck, > time_t end_time) > > Should these be divided into two lines? pgindent did not say anything about them, but I divided. > 6. the comment of verify_cached_connections > + * This function emits warnings if a disconnection is found, and this > returns > + * false only when the lastly verified server seems to be disconnected. Updated. I think comments were not correct, so these were also fixed. > It seems better to write the case where this function returns > true. > 7. the comment of postgres_fdw_verify_connection_states > + * This function emits a warning if a disconnection is found. This > returns > + * false only when the verified server seems to be disconnected, and > reutrns > + * NULL if the connection check had not been done. > > It seems better to write the case where this function returns > true. > 8. the code of > + (errcode(ERRCODE_CONNECTION_FAILURE), > + errmsg("%s", str.data), > + errdetail("Socket close is detected."), > + errhint("Plsease check the health of > server."))); > > Is it better to use "Connection close is detected" rather than > "Socket close is detected"? Changed. > 9. the document of postgres_fdw > The document of postgres_fdw_verify_connection_states_* is a little > bit old. Could you update it? Updated. IIUC postgres_fdw_verify_connection_states returns * true, if the connection is verified. * false, if the connection seems to be disconnected. * NULL, if this is not the supported platform or connection has not been established. And postgres_fdw_verify_connection_states_all returns * true if all the connections are verified. * false, if one of connections seems to be disconnected. * NULL, if this is not the supported platform or this backend has never established connections Best Regards, Hayato Kuroda FUJITSU LIMITED Attachments: [application/octet-stream] v34-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch (8.7K, ../../TYAPR01MB586674A3C56AECB9B150BC84F5B39@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-v34-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch) download | inline diff: From 0b3a6479c4ccadf97ab48925bfd42fe3088887e8 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Fri, 27 Jan 2023 03:17:18 +0000 Subject: [PATCH v34 1/3] Add PQconnCheck and PQconnCheckable to libpq PQconnCheck() function allows to check the status of the connection by polling the socket. This function is currently available only on systems that support the non-standard POLLRDHUP extension to the poll system call, including Linux. PQconnCheckable() checks whether the above function is available or not. --- doc/src/sgml/libpq.sgml | 38 +++++++++++++++ src/interfaces/libpq/exports.txt | 2 + src/interfaces/libpq/fe-misc.c | 79 ++++++++++++++++++++++++++------ src/interfaces/libpq/libpq-fe.h | 4 ++ 4 files changed, 108 insertions(+), 15 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 3ccd8ff942..b6461cf82f 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2679,6 +2679,44 @@ void *PQgetssl(const PGconn *conn); </listitem> </varlistentry> + <varlistentry id="libpq-PQconnCheck"> + <term><function>PQconnCheck</function><indexterm><primary>PQconnCheck</primary></indexterm></term> + <listitem> + <para> + Returns the health of the connection. + +<synopsis> +int PQconnCheck(PGconn *conn); +</synopsis> + </para> + + <para> + This function checks the health of the connection. Unlike <xref linkend="libpq-PQstatus"/>, + this check is performed by polling the corresponding socket. This + function is currently available only on systems that support the + non-standard <symbol>POLLRDHUP</symbol> extension to the <symbol>poll</symbol> + system call, including Linux. <xref linkend="libpq-PQconnCheck"/> + returns greater than zero if the remote peer seems to be closed, returns + <literal>0</literal> if the socket is valid, and returns <literal>-1</literal> + if the connection has already been closed or an error has occurred. + </para> + </listitem> + </varlistentry> + + <varlistentry id="libpq-PQconnCheckable"> + <term><function>PQconnCheckable</function><indexterm><primary>PQconnCheckable</primary></indexterm></term> + <listitem> + <para> + Returns true (1) or false (0) to indicate if the <xref linkend="libpq-PQconnCheck"/> + function is supported on this platform. + +<synopsis> +int PQconnCheckable(void); +</synopsis> + </para> + </listitem> + </varlistentry> + </variablelist> </para> diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index e8bcc88370..e7f0d435bd 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -186,3 +186,5 @@ PQpipelineStatus 183 PQsetTraceFlags 184 PQmblenBounded 185 PQsendFlushRequest 186 +PQconnCheck 187 +PQconnCheckable 188 diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c index 3653a1a8a6..fba5359dcb 100644 --- a/src/interfaces/libpq/fe-misc.c +++ b/src/interfaces/libpq/fe-misc.c @@ -53,9 +53,10 @@ static int pqPutMsgBytes(const void *buf, size_t len, PGconn *conn); static int pqSendSome(PGconn *conn, int len); -static int pqSocketCheck(PGconn *conn, int forRead, int forWrite, - time_t end_time); -static int pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time); +static int pqSocketCheck(PGconn *conn, int forRead, + int forWrite, int forConnCheck, time_t end_time); +static int pqSocketPoll(int sock, int forRead, + int forWrite, int forConnCheck, time_t end_time); /* * PQlibVersion: return the libpq version number @@ -993,7 +994,7 @@ pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time) { int result; - result = pqSocketCheck(conn, forRead, forWrite, finish_time); + result = pqSocketCheck(conn, forRead, forWrite, 0, finish_time); if (result < 0) return -1; /* errorMessage is already set */ @@ -1014,7 +1015,7 @@ pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time) int pqReadReady(PGconn *conn) { - return pqSocketCheck(conn, 1, 0, (time_t) 0); + return pqSocketCheck(conn, 1, 0, 0, (time_t) 0); } /* @@ -1024,19 +1025,52 @@ pqReadReady(PGconn *conn) int pqWriteReady(PGconn *conn) { - return pqSocketCheck(conn, 0, 1, (time_t) 0); + return pqSocketCheck(conn, 0, 1, 0, (time_t) 0); +} + +/* + * Check whether the socket peer closed the connection or not. + * + * Returns >0 if remote peer seems to be closed, 0 if it is valid, + * -1 if the input connection is bad or an error occurred. + */ +int +PQconnCheck(PGconn *conn) +{ + return pqSocketCheck(conn, 0, 0, 1, (time_t) 0); +} + +/* + * Check whether PQconnCheck() can work on this platform. + * + * Returns true (1) if this can use PQconnCheck(), otherwise false (0). + */ +int +PQconnCheckable(void) +{ +#if (defined(HAVE_POLL) && defined(POLLRDHUP)) + return true; +#else + return false; +#endif } /* * Checks a socket, using poll or select, for data to be read, written, - * or both. Returns >0 if one or more conditions are met, 0 if it timed - * out, -1 if an error occurred. + * or both. Moreover, this function can check the health of the connetion on + * some limited platforms if forConnCehck is specified. + * + * Returns >0 if one or more conditions are met, 0 if it timed out, -1 if an + * error occurred. Note that if 0 is returned and forConnCheck is requested, it + * means that the socket has not matched POLLRDHUP event and the socket has + * still survived. * * If SSL is in use, the SSL buffer is checked prior to checking the socket * for read data directly. */ static int -pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time) +pqSocketCheck(PGconn *conn, int forRead, + int forWrite, int forConnCheck, time_t end_time) { int result; @@ -1059,7 +1093,8 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time) /* We will retry as long as we get EINTR */ do - result = pqSocketPoll(conn->sock, forRead, forWrite, end_time); + result = pqSocketPoll(conn->sock, forRead, + forWrite, forConnCheck, end_time); while (result < 0 && SOCK_ERRNO == EINTR); if (result < 0) @@ -1076,15 +1111,21 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time) /* * Check a file descriptor for read and/or write data, possibly waiting. - * If neither forRead nor forWrite are set, immediately return a timeout - * condition (without waiting). Return >0 if condition is met, 0 - * if a timeout occurred, -1 if an error or interrupt occurred. + * Moreover, this function can check the health of connection on some limited + * platform if forConnCehck is specified. + * + * If neither forRead, forWrite nor forConnCheck are set, immediately return a + * timeout condition (without waiting). Return >0 if condition is met, 0 if a + * timeout occurred, -1 if an error or interrupt occurred. Note that if 0 is + * returned and forConnCheck is requested, it means that the socket has not + * matched POLLRDHUP event and the socket has still survived. * * Timeout is infinite if end_time is -1. Timeout is immediate (no blocking) * if end_time is 0 (or indeed, any time before now). */ static int -pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) +pqSocketPoll(int sock, int forRead, + int forWrite, int forConnCheck, time_t end_time) { /* We use poll(2) if available, otherwise select(2) */ #ifdef HAVE_POLL @@ -1092,7 +1133,11 @@ pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) int timeout_ms; if (!forRead && !forWrite) - return 0; + { + /* Connection check can be available on some limted platforms */ + if (!(forConnCheck && PQconnCheckable())) + return 0; + } input_fd.fd = sock; input_fd.events = POLLERR; @@ -1102,6 +1147,10 @@ pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) input_fd.events |= POLLIN; if (forWrite) input_fd.events |= POLLOUT; +#if defined(POLLRDHUP) + if (forConnCheck) + input_fd.events |= POLLRDHUP; +#endif /* Compute appropriate timeout interval */ if (end_time == ((time_t) -1)) diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index f3d9220496..e1bd0cd7b7 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -648,6 +648,10 @@ extern int PQdsplen(const char *s, int encoding); /* Get encoding id from environment variable PGCLIENTENCODING */ extern int PQenv2encoding(void); +/* Check whether the postgres server is still alive or not */ +extern int PQconnCheck(PGconn *conn); +extern int PQconnCheckable(void); + /* === in fe-auth.c === */ extern char *PQencryptPassword(const char *passwd, const char *user); -- 2.27.0 [application/octet-stream] v34-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch (12.3K, ../../TYAPR01MB586674A3C56AECB9B150BC84F5B39@TYAPR01MB5866.jpnprd01.prod.outlook.com/3-v34-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch) download | inline diff: From 2da884f1dab6278a1a3fd71faf02f659da71934f Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Fri, 27 Jan 2023 03:17:30 +0000 Subject: [PATCH v34 2/3] postgres_fdw: add postgres_fdw_verify_connection_states This function can verify the status of connections that are establieshed by postgres_fdw. This check wil be done by PQconnCheck(), which means this is available only on systems that support the non-standard POLLRDHUP extension to the poll system call, including Linux. This returns true if existing connection is not closed by the remote peer. --- contrib/postgres_fdw/Makefile | 2 +- contrib/postgres_fdw/connection.c | 162 ++++++++++++++++++ contrib/postgres_fdw/meson.build | 1 + .../postgres_fdw/postgres_fdw--1.1--1.2.sql | 19 ++ contrib/postgres_fdw/postgres_fdw.control | 2 +- doc/src/sgml/postgres-fdw.sgml | 74 ++++++++ 6 files changed, 258 insertions(+), 2 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/Makefile index 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.c index 12b54f15cd..99aff6304e 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -87,6 +87,9 @@ static bool xact_got_connection = false; PG_FUNCTION_INFO_V1(postgres_fdw_get_connections); PG_FUNCTION_INFO_V1(postgres_fdw_disconnect); PG_FUNCTION_INFO_V1(postgres_fdw_disconnect_all); +PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states); +PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states_all); +PG_FUNCTION_INFO_V1(postgres_fdw_can_verify_connection_states); /* prototypes of private functions */ static void make_new_connection(ConnCacheEntry *entry, UserMapping *user); @@ -117,6 +120,7 @@ static void pgfdw_finish_pre_subcommit_cleanup(List *pending_entries, int curlevel); static bool UserMappingPasswordRequired(UserMapping *user); static bool disconnect_cached_connections(Oid serverid); +static bool verify_cached_connections(Oid serverid, bool *checked); /* * Get a PGconn which can be used to execute queries on the remote PostgreSQL @@ -1832,3 +1836,161 @@ disconnect_cached_connections(Oid serverid) return result; } + +/* + * Workhorse to verify cached connections. + * + * This function scans all the connection cache entries and verifies the + * connections whose foreign server OID matches with the specified one. If + * InvalidOid is specified, it verifies all the cached connections. + * + * This function emits warnings if a disconnection is found. This return true + * if disconnections cannot be found, otherwise return false. + * + * checked will be set to true if PQconnCheck() is called at least once. + */ +static bool +verify_cached_connections(Oid serverid, bool *checked) +{ + HASH_SEQ_STATUS scan; + ConnCacheEntry *entry; + bool all = !OidIsValid(serverid); + bool result = true; + StringInfoData str; + + *checked = false; + + Assert(ConnectionHash); + + hash_seq_init(&scan, ConnectionHash); + while ((entry = (ConnCacheEntry *) hash_seq_search(&scan))) + { + /* Ignore cache entry if no open connection right now */ + if (!entry->conn) + continue; + + /* Skip if the entry is invalidated */ + if (entry->invalidated) + continue; + + if (all || entry->serverid == serverid) + { + if (PQconnCheck(entry->conn)) + { + /* A foreign server might be down, so construct a message */ + ForeignServer *server = GetForeignServer(entry->serverid); + + if (result) + { + /* + * Initialize and add a prefix if this is the first + * disconnection we found. + */ + initStringInfo(&str); + appendStringInfo(&str, "could not connect to server "); + + result = false; + } + else + appendStringInfo(&str, ", "); + + appendStringInfo(&str, "\"%s\"", server->servername); + } + + /* Set a flag to notify the caller */ + *checked = true; + } + } + + /* Raise a warning if disconnections are found */ + if (!result) + { + Assert(str.len); + ereport(WARNING, + (errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("%s", str.data), + errdetail("Connection close is detected."), + errhint("Plsease check the health of server."))); + pfree(str.data); + } + + return result; +} + +/* + * Verify the specified cached connections. + * + * This function verifies the connections that are established by postgres_fdw + * from the local session to the foreign server with the given name. + * + * This function emits a warning if a disconnection is found. This returns true + * if existing connection is not closed by the remote peer. false is returned + * if the local session seems to be disconnected from other servers. NULL is + * returned if a connection to the specified foreign server has not been + * established yet, or this function is not available on this platform. + */ +Datum +postgres_fdw_verify_connection_states(PG_FUNCTION_ARGS) +{ + ForeignServer *server; + char *servername; + bool result, + checked = false; + + /* quick exit if the checking does not work well on this platfrom */ + if (!PQconnCheckable()) + PG_RETURN_NULL(); + + /* quick exit if connection cache has not been initialized yet */ + if (!ConnectionHash) + PG_RETURN_NULL(); + + servername = text_to_cstring(PG_GETARG_TEXT_PP(0)); + server = GetForeignServerByName(servername, false); + + result = verify_cached_connections(server->serverid, &checked); + + /* Return the result if checking function was called, otherwise NULL */ + if (checked) + PG_RETURN_BOOL(result); + else + PG_RETURN_NULL(); +} + +/* + * Verify all the cached connections. + * + * This function verifies all the connections that are established by postgres_fdw + * from the local session to the foreign servers. + */ +Datum +postgres_fdw_verify_connection_states_all(PG_FUNCTION_ARGS) +{ + bool result, + checked = false; + + /* quick exit if the checking does not work well on this platfrom */ + if (!PQconnCheckable()) + PG_RETURN_NULL(); + + /* quick exit if connection cache has not been initialized yet */ + if (!ConnectionHash) + PG_RETURN_NULL(); + + result = verify_cached_connections(InvalidOid, &checked); + + /* Return the result if checking function was called, otherwise NULL */ + if (checked) + PG_RETURN_BOOL(result); + else + PG_RETURN_NULL(); +} + +/* + * Check whether functions for verifying cached connections work well or not + */ +Datum +postgres_fdw_can_verify_connection_states(PG_FUNCTION_ARGS) +{ + PG_RETURN_BOOL(PQconnCheckable()); +} diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build index 2b451f165e..29118d47bb 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.sql new file mode 100644 index 0000000000..a8556b4c9f --- /dev/null +++ b/contrib/postgres_fdw/postgres_fdw--1.1--1.2.sql @@ -0,0 +1,19 @@ +/* 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 + +CREATE FUNCTION postgres_fdw_verify_connection_states (text) +RETURNS bool +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT PARALLEL RESTRICTED; + +CREATE FUNCTION postgres_fdw_verify_connection_states_all () +RETURNS bool +AS 'MODULE_PATHNAME' +LANGUAGE C STRICT PARALLEL RESTRICTED; + +CREATE FUNCTION postgres_fdw_can_verify_connection_states () +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.control index 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/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 644f51835b..85e21f8904 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -826,6 +826,80 @@ postgres=# SELECT postgres_fdw_disconnect_all(); </para> </listitem> </varlistentry> + + <varlistentry> + <term><function>postgres_fdw_verify_connection_states(server_name text) returns boolean</function></term> + <listitem> + <para> + This function checks the status of remote connections established by + <filename>postgres_fdw</filename> from the local session to the foreign + server with the given name. This check is performed by polling the socket + and allows long-running transactions to be aborted sooner if the kernel + reports that the connection is closed. This function is currently + available only on systems that support the non-standard <symbol>POLLRDHUP</symbol> + extension to the <symbol>poll</symbol> system call, including Linux. This + returns <literal>true</literal> if existing connection is not closed by + the remote peer. <literal>false</literal> is returned if the local + session seems to be disconnected from other servers. <literal>NULL</literal> + is returned if a connection to the specified foreign server has not been + established yet, or this function is not available on this platform. + If no foreign server with the given name is found, an error is reported. + Example usage of the function: +<screen> +postgres=# SELECT postgres_fdw_verify_connection_states('loopback1'); + postgres_fdw_verify_connection_states +--------------------------------------- + t +</screen> + </para> + </listitem> + </varlistentry> + <varlistentry> + <term><function>postgres_fdw_verify_connection_states_all() returns boolean</function></term> + <listitem> + <para> + This function checks the status of all the remote connections established + by <filename>postgres_fdw</filename> from the local session to the + foreign servers. This check is performed by polling the socket and allows + long-running transactions to be aborted sooner if the kernel reports + that the connection is closed. This function is currently available only + on systems that support the non-standard <symbol>POLLRDHUP</symbol> + extension to the <symbol>poll</symbol> system call, including Linux. This + returns <literal>true</literal> if all connections are not closed by the + remote peer. <literal>false</literal> is returned if the local session + seems to be disconnected from at least one remote server. <literal>NULL</literal> + is returned if the local session does not have connection caches, or this + function is not available on this platform. Example usage of the + function: +<screen> +postgres=# SELECT postgres_fdw_verify_connection_states_all(); + postgres_fdw_verify_connection_states_all +------------------------------------------- + t +</screen> + </para> + </listitem> + </varlistentry> + + <varlistentry> + <term><function>postgres_fdw_can_verify_connection_states() returns boolean</function></term> + <listitem> + <para> + This function checks whether <function>postgres_fdw_verify_connection_states</function> + and <function>postgres_fdw_verify_connection_states</function> work well + 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_states(); + postgres_fdw_can_verify_connection_states +------------------------------------------- + t +</screen> + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> -- 2.27.0 [application/octet-stream] v34-0003-add-test.patch (5.3K, ../../TYAPR01MB586674A3C56AECB9B150BC84F5B39@TYAPR01MB5866.jpnprd01.prod.outlook.com/4-v34-0003-add-test.patch) download | inline diff: From 139c1e4d64ae8bfeb8e3eb2a174eb0ab762f6412 Mon Sep 17 00:00:00 2001 From: Hayato Kuroda <[email protected]> Date: Fri, 27 Jan 2023 03:17:36 +0000 Subject: [PATCH v34 3/3] add test --- .../postgres_fdw/expected/postgres_fdw.out | 64 +++++++++++++++++++ contrib/postgres_fdw/sql/postgres_fdw.sql | 62 ++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 04a3ef450c..6446f36092 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -11783,3 +11783,67 @@ ANALYZE analyze_table; -- cleanup DROP FOREIGN TABLE analyze_ftable; DROP TABLE analyze_table; +-- =================================================================== +-- test for postgres_fdw_verify_foreign_servers function +-- =================================================================== +-- 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(use_all boolean) 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_states(); + + -- 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; + + IF use_all IS TRUE THEN + SELECT INTO result postgres_fdw_verify_connection_states_all(); + ELSE + SELECT INTO result postgres_fdw_verify_connection_states('loopback'); + END IF; + + RESET client_min_messages; + ELSE + result = false; + END IF; + + -- If result is FALSE, we succeeded to detect the disconnection or it could + -- not be done on this platform. Raise an message. + IF result IS FALSE THEN + RAISE INFO 'postgres_fdw_verify_connection_states_all() 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(false); +INFO: 00000 +ERROR: 08006 +CALL test_verify_function(true); +INFO: 00000 +ERROR: 08006 +-- Clean up +\set VERBOSITY default +RESET debug_discard_caches; diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 4f3088c03e..b413f96e32 100644 --- a/contrib/postgres_fdw/sql/postgres_fdw.sql +++ b/contrib/postgres_fdw/sql/postgres_fdw.sql @@ -3979,3 +3979,65 @@ ANALYZE analyze_table; -- cleanup DROP FOREIGN TABLE analyze_ftable; DROP TABLE analyze_table; + +-- =================================================================== +-- test for postgres_fdw_verify_foreign_servers function +-- =================================================================== + +-- 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(use_all boolean) 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_states(); + + -- 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; + + IF use_all IS TRUE THEN + SELECT INTO result postgres_fdw_verify_connection_states_all(); + ELSE + SELECT INTO result postgres_fdw_verify_connection_states('loopback'); + END IF; + + RESET client_min_messages; + ELSE + result = false; + END IF; + + -- If result is FALSE, we succeeded to detect the disconnection or it could + -- not be done on this platform. Raise an message. + IF result IS FALSE THEN + RAISE INFO 'postgres_fdw_verify_connection_states_all() 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(false); +CALL test_verify_function(true); + +-- Clean up +\set VERBOSITY default +RESET debug_discard_caches; -- 2.27.0 ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: [Proposal] Add foreign-server health checks infrastructure @ 2023-03-06 10:13 Katsuragi Yuta <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Katsuragi Yuta @ 2023-03-06 10:13 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: 'Ted Yu' <[email protected]>; Tom Lane <[email protected]>; vignesh C <[email protected]>; [email protected]; Önder Kalacı <[email protected]>; Fujii Masao <[email protected]>; [email protected]; Kyotaro Horiguchi <[email protected]> Hi Kuroda-san, Thank you for updating the patch! >> 4. the code of pqSocketPoll >> +#if defined(POLLRDHUP) >> + if (forConnCheck) >> + input_fd.events |= POLLRDHUP; >> +#endif >> >> I think it is better to use PQconnCheckable() to remove the macro. > > IIUC the macro is needed. In FreeBSD, macOS and other platforms do not > have the > macro POLLRDHUP so they cannot compile. I checked by my CI and got > following error. > > ``` > ... > FAILED: src/interfaces/libpq/libpq.a.p/fe-misc.c.o > ... > ../src/interfaces/libpq/fe-misc.c:1149:22: error: use of undeclared > identifier 'POLLRDHUP' > input_fd.events |= POLLRDHUP; > ```` > > It must be invisible from them. Sorry, my mistake... >> 9. the document of postgres_fdw >> The document of postgres_fdw_verify_connection_states_* is a little >> bit old. Could you update it? > > Updated. IIUC postgres_fdw_verify_connection_states returns > > * true, if the connection is verified. > * false, if the connection seems to be disconnected. > * NULL, if this is not the supported platform or connection has not > been established. > > And postgres_fdw_verify_connection_states_all returns > > * true if all the connections are verified. > * false, if one of connections seems to be disconnected. > * NULL, if this is not the supported platform or this backend has > never established connections I think 'backend has never established connections' is a little bit strong. I think the following cases also return NULL. The case where a connection was established, however the connection is now closed by the postgres_fdw_disconnect() or something. NULL is also returned if the connection is invalidated. So, I think 'NULL, if no valid connection is established or the function is not supported on the platform.' is better. What do you think? Followings are my comments for v34. Please check. 0001: 1. the document of pqConnCheck + <literal>0</literal> if the socket is valid, and returns <literal>-1</literal> + if the connection has already been closed or an error has occurred. 1.1 if the socket is valid -> returns 0 if the 'connection is not closed'? 1.2 returns -1 if the connection has already been closed <- Let me ask a question. Isn't this a situation where we would like to check using this function? Is 'error has occurred' insufficient? 2. the comment of pqSocketCheck + * means that the socket has not matched POLLRDHUP event and the socket has + * still survived. socket has still survived -> connection is not closed by the socket peer? 3. the comment of pqSocketPoll + * returned and forConnCheck is requested, it means that the socket has not + * matched POLLRDHUP event and the socket has still survived. socket has still survived -> connection is not closed by the socket peer? 0002: 4. the comment of verify_cached_connections + * This function emits warnings if a disconnection is found. This return true + * if disconnections cannot be found, otherwise return false. return ture -> return's' true return false -> return's' false 5. the comment of postgres_fdw_verify_connection_states + * if the local session seems to be disconnected from other servers. NULL is + * returned if a connection to the specified foreign server has not been + * established yet, or this function is not available on this platform. Considering the above discussion, 'NULL is returned if a valid connection to the specified foreign server is not established or this function...' seems better. What do you think? 6. the document of postgres_fdw_verify_connection_states <literal>NULL</literal> + is returned if a connection to the specified foreign server has not been + established yet, or this function is not available on this platform The same as comment no.5. 7. the document of postgres_fdw_verify_connection_states_all <literal>NULL</literal> + is returned if the local session does not have connection caches, or this + function is not available on this platform. I think there is a case where a connection cache exists but valid connections do not exist and NULL is returned (disconnection case). Almost the same document as the postgres_fdw_verify_connection_states case (comment no.5) seems better. 8. the document of postgres_fdw_can_verify_connection_states + This function checks whether <function>postgres_fdw_verify_connection_states</function> + and <function>postgres_fdw_verify_connection_states</function> work well Should the latter (or former) postgres_fdw_verify_connection_states be postgres_fdw_verify_connection_states_all? regards, -- Katsuragi Yuta Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-03-06 10:13 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-17 18:16 [PATCH v19 03/10] Add tests on pg_ls_dir before changing it Justin Pryzby <[email protected]> 2023-03-01 02:34 Re: [Proposal] Add foreign-server health checks infrastructure Katsuragi Yuta <[email protected]> 2023-03-03 12:24 ` RE: [Proposal] Add foreign-server health checks infrastructure Hayato Kuroda (Fujitsu) <[email protected]> 2023-03-06 10:13 ` Re: [Proposal] Add foreign-server health checks infrastructure Katsuragi Yuta <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox