public inbox for [email protected]help / color / mirror / Atom feed
Re: [Proposal] Add foreign-server health checks infrastructure 10+ messages / 4 participants [nested] [flat]
* Re: [Proposal] Add foreign-server health checks infrastructure @ 2022-01-18 15:42 Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Fujii Masao @ 2022-01-18 15:42 UTC (permalink / raw) To: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]> On 2021/12/15 15:40, [email protected] wrote: > Yeah, remote-checking timeout will be enable even ifa local transaction is opened. > In my understanding, postgres cannot distinguish whether opening transactions > are using only local object or not. > My first idea was that turning on the timeout when GetFdwRoutineXXX functions > were called, What about starting the timeout in GetConnection(), instead? > I attached which implements that. v05_0004_add_tests.patch failed to be applied to the master. Could you rebase it? - This option 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 option relies on kernel events exposed by Linux, macOS, illumos + and the BSD family of operating systems, and is not currently available + on other systems. The above change is included in both v5-0003-Use-WL_SOCKET_CLOSED-for-client_connection_check_.patch and v05_0002_add_doc.patch. If it should be in the former patch, it should be removed from your patch v05_0002_add_doc.patch. There seems no user of UnregisterCheckingRemoteServersCallback(). So how about removing it? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> @ 2022-01-20 08:21 ` [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: [email protected] @ 2022-01-20 08:21 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; 'Shinya Kato' <[email protected]> Dear Fujii-san, Thank you for your interest! I'll post new version within several days. > > Yeah, remote-checking timeout will be enable even ifa local transaction is > opened. > > In my understanding, postgres cannot distinguish whether opening transactions > > are using only local object or not. > > My first idea was that turning on the timeout when GetFdwRoutineXXX > functions > > were called, > > What about starting the timeout in GetConnection(), instead? Did you said about a function in postgres_fdw/connection.c? In my understanding that means that the timeout should be enabled or disabled by each FDW extensions. I did not find bad cases for that, so I'll change like that and make new APIs. > v05_0004_add_tests.patch failed to be applied to the master. Could you rebase it? It's caused because a testcase was added in postgres_fdw. Will rebase. > The above change is included in both > v5-0003-Use-WL_SOCKET_CLOSED-for-client_connection_check_.patch and > v05_0002_add_doc.patch. If it should be in the former patch, it should be removed > from your patch v05_0002_add_doc.patch. I confused about doc-patch. Sorry for inconvenience. > There seems no user of UnregisterCheckingRemoteServersCallback(). So how > about removing it? Previously I kept the API for any other extensions, but I cannot find use cases. Will remove. Best Regards, Hayato Kuroda FUJITSU LIMITED ^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> @ 2022-01-21 06:08 ` [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: [email protected] @ 2022-01-21 06:08 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]> Dear Fujii-san, Zhihong, I attached the latest version. The biggest change is that callbacks are no longer un-registered at the end of transactions. FDW developer must enable or disable timeout instead, via new APIs. The timer will be turned on when: * new GUC is >= 0, and * anyone calls TryEnableRemoteServerCheckingTimeout(). I think this version is reduced overhead, but it might not be developer friendly... Best Regards, Hayato Kuroda FUJITSU LIMITED Attachments: [application/x-zip-compressed] patches.zip (7.7K, ../../TYAPR01MB58669361410DFD6B01DF45E3F55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-patches.zip) download [application/octet-stream] v06_0002_add_doc.patch (2.9K, ../../TYAPR01MB58669361410DFD6B01DF45E3F55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com/3-v06_0002_add_doc.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 4cd9818acf..870221fab4 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1035,6 +1035,25 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-remote-servers-connection-check-interval" xreflabel="remote_servers_connection_check_interval"> + <term><varname>remote_servers_connection_check_interval</varname> (<type>integer</type>) + <indexterm> + <primary><varname>remote_servers_connection_check_interval</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the time interval between optional checks that the remote servers + are still alive. If this parameter is set to a valid value, registered + callback functions are executed at the specified time interval. + If your FDW extension does not implement a callback function, nothing + happens after the specified amount of time. If the value is specified + without units, it is taken as milliseconds. The default value is + <literal>0</literal>, this means the feature is disabled. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index dbf5dd3d15..f7482ab79f 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -2136,4 +2136,40 @@ GetForeignServerByName(const char *name, bool missing_ok); </sect1> + <sect1 id="fdw-checking-remote-servers"> + <title>Checking the Health of Remote Servers via Foreign Data Wrapper</title> + + <para> + For verifying the health of remote servers, callback functions can be + registered that runs periodically. The time interval is controlled by + <xref linkend="guc-remote-servers-connection-check-interval"/>. + The API for registering functions is: + <programlisting> +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, void *arg); + </programlisting> + Only functions that return void can be registered as callback functions. + Please set to void* arg a pointer if you want to pass an argument. + If you have problems with the health check results and want to + abort the transaction, please execute <function>ereport(ERROR)</function> + in the callback function. + </para> + <para> + Note that starting or stopping the periodic execution is FDW's responsibility. + If you want to start verification, the following function must be performed + after registering callback functions: + <programlisting> +int +TryEnableRemoteServerCheckingTimeout(void) + </programlisting> + If you want to stop, the following function must be performed: + <programlisting> +int +TryDisableRemoteServerCheckingTimeout(void) + </programlisting> + </para> + + </sect1> + + </chapter> [application/octet-stream] v06_0001_add_checking_infrastracture.patch (12.4K, ../../TYAPR01MB58669361410DFD6B01DF45E3F55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com/4-v06_0001_add_checking_infrastracture.patch) download | inline diff: diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index baf328b620..996386fdd4 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -269,6 +269,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) readmessage: HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); mtype = pq_getbyte(); if (mtype == EOF) @@ -300,6 +301,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); /* ... and process it */ switch (mtype) diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 294e22c78c..9622058aaa 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -26,7 +26,13 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/timeout.h" +/* for checking remote servers */ +int remote_servers_connection_check_interval = 0; +static CheckingRemoteServersCallbackItem *fdw_callbacks = NULL; +/* counter for keeping re-entrancy */ +static int timeout_counter = 0; /* * GetForeignDataWrapper - look up the foreign-data wrapper by OID. @@ -836,3 +842,98 @@ GetExistingLocalJoinPath(RelOptInfo *joinrel) } return NULL; } + +/* + * Register callbacks for checking remote servers. + * + * This function is intended for use by FDW extensions. + * The checking timeout will be fired after registering the first callback. + */ +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) +{ + CheckingRemoteServersCallbackItem *item; + + item = (CheckingRemoteServersCallbackItem *) + MemoryContextAlloc(TopMemoryContext, + sizeof(CheckingRemoteServersCallbackItem)); + item->callback = callback; + item->arg = arg; + item->next = fdw_callbacks; + fdw_callbacks = item; +} + +/* + * Call callbacks for checking remote servers. + * + * Note that this function will not return anything. + * Callback functions must throw ereport(ERROR) + * if disconnection has been detected. + */ +void +CallCheckingRemoteServersCallbacks(void) +{ + CheckingRemoteServersCallbackItem *item; + for (item = fdw_callbacks; item; item = item->next) + item->callback(item->arg); +} + +/* + * Increment counter, and enable timeout if it has been not started yet. + */ +int +TryEnableRemoteServerCheckingTimeout(void) +{ + timeout_counter++; + + if (remote_servers_connection_check_interval > 0 && + !get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + + return timeout_counter; +} + +/* + * Decrement counter, and disable timeout if it is no more needed. + * + * This functions must be called after TryEnableRemoteServerCheckingTimeout(). + */ +int +TryDisableRemoteServerCheckingTimeout(void) +{ + Assert(timeout_counter > 0); + timeout_counter--; + + if (timeout_counter == 0 && + get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + return timeout_counter; +} + +void +assign_remote_servers_connection_check_interval(int newval, + void *extra) +{ + /* Quick return if we don't have any callbacks */ + if (fdw_callbacks == NULL) + return; + + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + { + if (newval == 0) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + /* + * we don't have to do anything because + * new value will be used in ProcessInterrupts(). + */ + return; + } + + /* Start timeout if anyone wants to */ + if (newval > 0 && timeout_counter > 0) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, newval); +} \ No newline at end of file diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fda2e9360e..60a7e91889 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -44,6 +44,7 @@ #include "commands/prepare.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "jit/jit.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -350,6 +351,7 @@ SocketBackend(StringInfo inBuf) * Get message type code from the frontend. */ HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); qtype = pq_getbyte(); @@ -456,6 +458,7 @@ SocketBackend(StringInfo inBuf) */ if (pq_getmessage(inBuf, maxmsglen)) return EOF; /* suitable message already logged */ + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); return qtype; @@ -3214,6 +3217,28 @@ ProcessInterrupts(void) } } + if (CheckingRemoteServersTimeoutPending) + { + if (CheckingRemoteServersHoldoffCount != 0) + { + /* + * Skip checking foreign servers while reading messages. + */ + InterruptPending = true; + } + else + { + CheckingRemoteServersTimeoutPending = false; + + if (remote_servers_connection_check_interval > 0) + { + CallCheckingRemoteServersCallbacks(); + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + } + } + } + if (ClientConnectionLost) { QueryCancelPending = false; /* lost connection trumps QueryCancel */ diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index c26a1a73df..7efa9cff84 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -36,9 +36,11 @@ volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; volatile sig_atomic_t IdleSessionTimeoutPending = false; volatile sig_atomic_t ProcSignalBarrierPending = false; volatile sig_atomic_t LogMemoryContextPending = false; +volatile sig_atomic_t CheckingRemoteServersTimeoutPending = false; volatile uint32 InterruptHoldoffCount = 0; volatile uint32 QueryCancelHoldoffCount = 0; volatile uint32 CritSectionCount = 0; +volatile uint32 CheckingRemoteServersHoldoffCount = 0; int MyProcPid; pg_time_t MyStartTime; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 0236165f60..d828097076 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -73,6 +73,7 @@ static void LockTimeoutHandler(void); static void IdleInTransactionSessionTimeoutHandler(void); static void IdleSessionTimeoutHandler(void); static void ClientCheckTimeoutHandler(void); +static void CheckingRemoteServersTimeoutHandler(void); static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); @@ -621,6 +622,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, IdleInTransactionSessionTimeoutHandler); RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler); RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler); + RegisterTimeout(CHECKING_REMOTE_SERVERS_TIMEOUT, CheckingRemoteServersTimeoutHandler); } /* @@ -1239,6 +1241,14 @@ ClientCheckTimeoutHandler(void) SetLatch(MyLatch); } +static void +CheckingRemoteServersTimeoutHandler(void) +{ + CheckingRemoteServersTimeoutPending = true; + InterruptPending = true; + SetLatch(MyLatch); +} + /* * Returns true if at least one role is defined in this database cluster. */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 4c94f09c64..fc64c2e63a 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -52,6 +52,7 @@ #include "commands/vacuum.h" #include "commands/variable.h" #include "common/string.h" +#include "foreign/foreign.h" #include "funcapi.h" #include "jit/jit.h" #include "libpq/auth.h" @@ -106,6 +107,7 @@ #include "utils/queryjumble.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/timeout.h" #include "utils/tzparser.h" #include "utils/inval.h" #include "utils/varlena.h" @@ -3600,6 +3602,18 @@ static struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + + { + {"remote_servers_connection_check_interval", PGC_USERSET, CONN_AUTH_SETTINGS, + gettext_noop("Sets the time interval between checks for disconnection of remote servers."), + NULL, + GUC_UNIT_MS + }, + &remote_servers_connection_check_interval, + 0, 0, INT_MAX, + NULL, assign_remote_servers_connection_check_interval, NULL + }, + /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 817d5f5324..b97dc30514 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -90,6 +90,10 @@ # disconnection while running queries; # 0 for never +#remote_servers_connection_check_interval = 0 # time between time between checks for + # foreign server disconnection; + # 0 for never + # - Authentication - #authentication_timeout = 1min # 1s-600s diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index 75538110fc..76364ce8a3 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -81,4 +81,24 @@ extern List *GetForeignColumnOptions(Oid relid, AttrNumber attnum); extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok); extern Oid get_foreign_server_oid(const char *servername, bool missing_ok); + +/* functions and variables for fdw checking. */ +typedef void (*CheckingRemoteServersCallback) (void *arg); +typedef struct CheckingRemoteServersCallbackItem +{ + struct CheckingRemoteServersCallbackItem *next; + CheckingRemoteServersCallback callback; + void *arg; +} CheckingRemoteServersCallbackItem; + +extern void RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg); +extern void CallCheckingRemoteServersCallbacks(void); + +extern int TryEnableRemoteServerCheckingTimeout(void); +extern int TryDisableRemoteServerCheckingTimeout(void); + +extern int remote_servers_connection_check_interval; +extern void assign_remote_servers_connection_check_interval(int newval, + void *extra); #endif /* FOREIGN_H */ diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 02276d3edd..0319fe1605 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -98,10 +98,13 @@ extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending; extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending; extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost; +extern PGDLLIMPORT volatile sig_atomic_t CheckingRemoteServersTimeoutPending; + /* these are marked volatile because they are examined by signal handlers: */ extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount; extern PGDLLIMPORT volatile uint32 CritSectionCount; +extern PGDLLIMPORT volatile uint32 CheckingRemoteServersHoldoffCount; /* in tcop/postgres.c */ extern void ProcessInterrupts(void); @@ -126,7 +129,7 @@ do { \ /* Is ProcessInterrupts() guaranteed to clear InterruptPending? */ #define INTERRUPTS_CAN_BE_PROCESSED() \ (InterruptHoldoffCount == 0 && CritSectionCount == 0 && \ - QueryCancelHoldoffCount == 0) + QueryCancelHoldoffCount == 0 && CheckingRemoteServersHoldoffCount == 0) #define HOLD_INTERRUPTS() (InterruptHoldoffCount++) @@ -152,6 +155,13 @@ do { \ CritSectionCount--; \ } while(0) +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() (CheckingRemoteServersHoldoffCount++) + +#define RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS() \ +do { \ + Assert(CheckingRemoteServersHoldoffCount > 0); \ + CheckingRemoteServersHoldoffCount--; \ +} while(0) /***************************************************************************** * globals.h -- * diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h index 099f91c61d..7f02a8b491 100644 --- a/src/include/utils/timeout.h +++ b/src/include/utils/timeout.h @@ -34,6 +34,7 @@ typedef enum TimeoutId IDLE_SESSION_TIMEOUT, CLIENT_CONNECTION_CHECK_TIMEOUT, STARTUP_PROGRESS_TIMEOUT, + CHECKING_REMOTE_SERVERS_TIMEOUT, /* First user-definable timeout reason */ USER_TIMEOUT, /* Maximum number of timeout reasons */ ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> @ 2022-01-23 05:36 ` Fujii Masao <[email protected]> 2022-02-01 04:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Fujii Masao @ 2022-01-23 05:36 UTC (permalink / raw) To: [email protected] <[email protected]>; [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]> On 2022/01/21 15:08, [email protected] wrote: > Dear Fujii-san, Zhihong, > > I attached the latest version. Thanks for updating the patch! +int +TryDisableRemoteServerCheckingTimeout(void) When more than one FDWs are used, even if one FDW calls this function to disable the timeout, its remote-server-check-callback can still be called. Is this OK? Please imagine the case where two FDWs are used and they registered their callbacks. Even when one FDW calls TryDisableRemoteServerCheckingTimeout(), if another FDW has not called that yet, the timeout is still being enabled. If the timeout is triggered during that period, even the callback registered by the FDW that has already called TryDisableRemoteServerCheckingTimeout() would be called. + if (remote_servers_connection_check_interval > 0) + { + CallCheckingRemoteServersCallbacks(); + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); LockErrorCleanup() needs to be called before reporting the error, doesn't it? This can cause an error even while DoingCommandRead == true. Is that safe? > The biggest change is that callbacks are no longer un-registered at the end of transactions. > FDW developer must enable or disable timeout instead, via new APIs. > > The timer will be turned on when: > * new GUC is >= 0, and This can cause the timeout to be enabled even when no remote transaction is started? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> @ 2022-02-01 04:37 ` [email protected] <[email protected]> 2022-02-01 14:51 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: [email protected] @ 2022-02-01 04:37 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; +Cc: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]>; [email protected] <[email protected]> Dear Fujii-san, Thank you for reviewing! I attached the latest version. > When more than one FDWs are used, even if one FDW calls this function to > disable the timeout, its remote-server-check-callback can still be called. Is this > OK? > Please imagine the case where two FDWs are used and they registered their > callbacks. Even when one FDW calls TryDisableRemoteServerCheckingTimeout(), > if another FDW has not called that yet, the timeout is still being enabled. If the > timeout is triggered during that period, even the callback registered by the FDW > that has already called TryDisableRemoteServerCheckingTimeout() would be > called. Indeed and it should be avoided. I added a counter to CheckingRemoteServersCallbackItem. The register function returns the registered item, and it must be set as the argument for enable and disable functions. Callback functions will be called when item->counter is larger than zero. > + if (remote_servers_connection_check_interval > 0) > + { > + CallCheckingRemoteServersCallbacks(); > + > enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, > + > remote_servers_connection_check_interval); > > LockErrorCleanup() needs to be called before reporting the error, doesn't it? You are right. I think this suggests that error-reporting should be done in the core layer. For meaningful error reporting, I changed a callback specification that it should return ForeignServer* which points to downed remote server. > This can cause an error even while DoingCommandRead == true. Is that safe? I read codes again and I think it is not safe. It is OK when whereToSendOutput is DestRemote, but not good in InteractiveBackend(). I changed that if-statement for CheckingRemoteServersTimeoutPending is moved just after ClientConnectionLost, because the that may throw a FATAL error and disconnect from client. Best Regards, Hayato Kuroda FUJITSU LIMITED Attachments: [application/x-zip-compressed] pachset.zip (7.7K, ../../TYAPR01MB5866D5297140DA5908ECFD27F5269@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-pachset.zip) download [application/octet-stream] v07_0001_add_checking_infrastracture.patch (13.4K, ../../TYAPR01MB5866D5297140DA5908ECFD27F5269@TYAPR01MB5866.jpnprd01.prod.outlook.com/3-v07_0001_add_checking_infrastracture.patch) download | inline diff: diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index baf328b620..996386fdd4 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -269,6 +269,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) readmessage: HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); mtype = pq_getbyte(); if (mtype == EOF) @@ -300,6 +301,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); /* ... and process it */ switch (mtype) diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 294e22c78c..9779eadf01 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -26,7 +26,13 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/timeout.h" +/* for checking remote servers */ +int remote_servers_connection_check_interval = 0; +static CheckingRemoteServersCallbackItem *fdw_callbacks = NULL; +/* counter for keeping re-entrancy */ +static int timeout_counter = 0; /* * GetForeignDataWrapper - look up the foreign-data wrapper by OID. @@ -836,3 +842,111 @@ GetExistingLocalJoinPath(RelOptInfo *joinrel) } return NULL; } + +/* + * Register callbacks for checking remote servers. + * + * This function is intended for use by FDW extensions. + * The checking timeout will be fired after registering the first callback. + */ +CheckingRemoteServersCallbackItem * +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) +{ + CheckingRemoteServersCallbackItem *item; + + item = (CheckingRemoteServersCallbackItem *) + MemoryContextAlloc(TopMemoryContext, + sizeof(CheckingRemoteServersCallbackItem)); + item->callback = callback; + item->counter = 0; + item->arg = arg; + item->next = fdw_callbacks; + fdw_callbacks = item; + + return item; +} + +/* + * Call callbacks for checking remote servers if needed. + * This retruns the downed server, or NULL if all servers are good. + */ +ForeignServer * +CallCheckingRemoteServersCallbacks(void) +{ + CheckingRemoteServersCallbackItem *item; + ForeignServer *server = NULL; + + for (item = fdw_callbacks; item; item = item->next) + { + if (item->counter > 0) + { + server = item->callback(item->arg); + if (server != NULL) + break; + } + } + + return server; +} + +/* + * Increment counters, and enable timeout if it has been not started yet. + */ +int +TryEnableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item) +{ + timeout_counter++; + item->counter++; + + if (remote_servers_connection_check_interval > 0 && + !get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + + return timeout_counter; +} + +/* + * Decrement counters, and disable timeout if it is no more needed. + * + * This functions must be called after TryEnableRemoteServerCheckingTimeout(). + */ +int +TryDisableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item) +{ + Assert(timeout_counter > 0 && item->counter > 0); + timeout_counter--; + item->counter--; + + if (timeout_counter == 0 && + get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + return timeout_counter; +} + +void +assign_remote_servers_connection_check_interval(int newval, + void *extra) +{ + /* Quick return if we don't have any callbacks */ + if (fdw_callbacks == NULL) + return; + + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + { + if (newval == 0) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + /* + * we don't have to do anything because + * new value will be used in ProcessInterrupts(). + */ + return; + } + + /* Start timeout if anyone wants to */ + if (newval > 0 && timeout_counter > 0) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, newval); +} \ No newline at end of file diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fda2e9360e..b99fcf7b48 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -44,6 +44,7 @@ #include "commands/prepare.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "jit/jit.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -350,6 +351,7 @@ SocketBackend(StringInfo inBuf) * Get message type code from the frontend. */ HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); qtype = pq_getbyte(); @@ -456,6 +458,7 @@ SocketBackend(StringInfo inBuf) */ if (pq_getmessage(inBuf, maxmsglen)) return EOF; /* suitable message already logged */ + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); return qtype; @@ -3225,6 +3228,39 @@ ProcessInterrupts(void) errmsg("connection to client lost"))); } + if (CheckingRemoteServersTimeoutPending) + { + if (CheckingRemoteServersHoldoffCount != 0 || DoingCommandRead) + { + /* + * Skip checking foreign servers while reading messages or commands. + */ + InterruptPending = true; + } + else + { + CheckingRemoteServersTimeoutPending = false; + + /* Check remote servers and re-arm, if still configured. */ + if (remote_servers_connection_check_interval > 0) + { + ForeignServer *downedServer = CallCheckingRemoteServersCallbacks(); + if (downedServer != NULL) + { + LockErrorCleanup(); + ereport(ERROR, + errcode(ERRCODE_CONNECTION_FAILURE), + errmsg("foreign server \"%s\" disconnected due to the health-check failure", + downedServer->servername), + errdetail("Foreign server might be down."), + errhint("Please check the server and network health.")); + } + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + } + } + } + /* * If a recovery conflict happens while we are waiting for input from the * client, the client is presumably just sitting idle in a transaction, diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index c26a1a73df..7efa9cff84 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -36,9 +36,11 @@ volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; volatile sig_atomic_t IdleSessionTimeoutPending = false; volatile sig_atomic_t ProcSignalBarrierPending = false; volatile sig_atomic_t LogMemoryContextPending = false; +volatile sig_atomic_t CheckingRemoteServersTimeoutPending = false; volatile uint32 InterruptHoldoffCount = 0; volatile uint32 QueryCancelHoldoffCount = 0; volatile uint32 CritSectionCount = 0; +volatile uint32 CheckingRemoteServersHoldoffCount = 0; int MyProcPid; pg_time_t MyStartTime; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 5b9ed2f6f5..9b0b4ddd58 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -75,6 +75,7 @@ static void LockTimeoutHandler(void); static void IdleInTransactionSessionTimeoutHandler(void); static void IdleSessionTimeoutHandler(void); static void ClientCheckTimeoutHandler(void); +static void CheckingRemoteServersTimeoutHandler(void); static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); @@ -628,6 +629,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, IdleInTransactionSessionTimeoutHandler); RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler); RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler); + RegisterTimeout(CHECKING_REMOTE_SERVERS_TIMEOUT, CheckingRemoteServersTimeoutHandler); } /* @@ -1246,6 +1248,14 @@ ClientCheckTimeoutHandler(void) SetLatch(MyLatch); } +static void +CheckingRemoteServersTimeoutHandler(void) +{ + CheckingRemoteServersTimeoutPending = true; + InterruptPending = true; + SetLatch(MyLatch); +} + /* * Returns true if at least one role is defined in this database cluster. */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index b3fd42e0f1..6dfc171448 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -52,6 +52,7 @@ #include "commands/vacuum.h" #include "commands/variable.h" #include "common/string.h" +#include "foreign/foreign.h" #include "funcapi.h" #include "jit/jit.h" #include "libpq/auth.h" @@ -106,6 +107,7 @@ #include "utils/queryjumble.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/timeout.h" #include "utils/tzparser.h" #include "utils/inval.h" #include "utils/varlena.h" @@ -3600,6 +3602,18 @@ static struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + + { + {"remote_servers_connection_check_interval", PGC_USERSET, CONN_AUTH_SETTINGS, + gettext_noop("Sets the time interval between checks for disconnection of remote servers."), + NULL, + GUC_UNIT_MS + }, + &remote_servers_connection_check_interval, + 0, 0, INT_MAX, + NULL, assign_remote_servers_connection_check_interval, NULL + }, + /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 817d5f5324..b97dc30514 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -90,6 +90,10 @@ # disconnection while running queries; # 0 for never +#remote_servers_connection_check_interval = 0 # time between time between checks for + # foreign server disconnection; + # 0 for never + # - Authentication - #authentication_timeout = 1min # 1s-600s diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index 75538110fc..922e63922b 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -81,4 +81,26 @@ extern List *GetForeignColumnOptions(Oid relid, AttrNumber attnum); extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok); extern Oid get_foreign_server_oid(const char *servername, bool missing_ok); + +/* functions and variables for fdw checking. */ +typedef ForeignServer* (*CheckingRemoteServersCallback) (void *arg); +typedef struct CheckingRemoteServersCallbackItem +{ + struct CheckingRemoteServersCallbackItem *next; + CheckingRemoteServersCallback callback; + int counter; + void *arg; +} CheckingRemoteServersCallbackItem; + +extern CheckingRemoteServersCallbackItem * +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg); +extern ForeignServer *CallCheckingRemoteServersCallbacks(void); + +extern int TryEnableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item); +extern int TryDisableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item); + +extern int remote_servers_connection_check_interval; +extern void assign_remote_servers_connection_check_interval(int newval, + void *extra); #endif /* FOREIGN_H */ diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 02276d3edd..0319fe1605 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -98,10 +98,13 @@ extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending; extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending; extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost; +extern PGDLLIMPORT volatile sig_atomic_t CheckingRemoteServersTimeoutPending; + /* these are marked volatile because they are examined by signal handlers: */ extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount; extern PGDLLIMPORT volatile uint32 CritSectionCount; +extern PGDLLIMPORT volatile uint32 CheckingRemoteServersHoldoffCount; /* in tcop/postgres.c */ extern void ProcessInterrupts(void); @@ -126,7 +129,7 @@ do { \ /* Is ProcessInterrupts() guaranteed to clear InterruptPending? */ #define INTERRUPTS_CAN_BE_PROCESSED() \ (InterruptHoldoffCount == 0 && CritSectionCount == 0 && \ - QueryCancelHoldoffCount == 0) + QueryCancelHoldoffCount == 0 && CheckingRemoteServersHoldoffCount == 0) #define HOLD_INTERRUPTS() (InterruptHoldoffCount++) @@ -152,6 +155,13 @@ do { \ CritSectionCount--; \ } while(0) +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() (CheckingRemoteServersHoldoffCount++) + +#define RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS() \ +do { \ + Assert(CheckingRemoteServersHoldoffCount > 0); \ + CheckingRemoteServersHoldoffCount--; \ +} while(0) /***************************************************************************** * globals.h -- * diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h index 099f91c61d..7f02a8b491 100644 --- a/src/include/utils/timeout.h +++ b/src/include/utils/timeout.h @@ -34,6 +34,7 @@ typedef enum TimeoutId IDLE_SESSION_TIMEOUT, CLIENT_CONNECTION_CHECK_TIMEOUT, STARTUP_PROGRESS_TIMEOUT, + CHECKING_REMOTE_SERVERS_TIMEOUT, /* First user-definable timeout reason */ USER_TIMEOUT, /* Maximum number of timeout reasons */ [application/octet-stream] v07_0002_add_doc.patch (3.2K, ../../TYAPR01MB5866D5297140DA5908ECFD27F5269@TYAPR01MB5866.jpnprd01.prod.outlook.com/4-v07_0002_add_doc.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 692d8a2a17..fb400a3a91 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1035,6 +1035,25 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-remote-servers-connection-check-interval" xreflabel="remote_servers_connection_check_interval"> + <term><varname>remote_servers_connection_check_interval</varname> (<type>integer</type>) + <indexterm> + <primary><varname>remote_servers_connection_check_interval</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the time interval between optional checks that the remote servers + are still alive. If this parameter is set to a valid value, registered + callback functions are executed at the specified time interval. + If your FDW extension does not implement a callback function, nothing + happens after the specified amount of time. If the value is specified + without units, it is taken as milliseconds. The default value is + <literal>0</literal>, this means the feature is disabled. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index dbf5dd3d15..3b75a77067 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -2136,4 +2136,40 @@ GetForeignServerByName(const char *name, bool missing_ok); </sect1> + <sect1 id="fdw-checking-remote-servers"> + <title>Checking the Health of Remote Servers via Foreign Data Wrapper</title> + + <para> + For verifying the health of remote servers, callback functions can be + registered that runs periodically. The time interval is controlled by + <xref linkend="guc-remote-servers-connection-check-interval"/>. + The API for registering functions is: + <programlisting> +CheckingRemoteServersCallbackItem * +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, void *arg); + </programlisting> + Only functions that return <type>ForeignServer*</type> can be + registered as callback functions. Please set to <literal>void* arg</literal> + if you want to pass an argument. The returning value must be set as the argument + of following enable and disable fucntions. + If you have problems with the health check results and want to abort the transaction, + please return the pointer of downed server as <type>ForeignServer*</type>. + </para> + <para> + Note that starting or stopping the periodic execution is FDW's responsibility. + If you want to start verification, the following function must be performed + after registering callback functions: + <programlisting> +int +TryEnableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item) + </programlisting> + If you want to stop, the following function must be performed: + <programlisting> +int +TryDisableRemoteServerCheckingTimeout(CheckingRemoteServersCallbackItem *item) + </programlisting> + </para> + </sect1> + + </chapter> ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-02-01 04:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> @ 2022-02-01 14:51 ` Fujii Masao <[email protected]> 2022-02-02 12:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-02-16 02:50 ` Re: [Proposal] Add foreign-server health checks infrastructure Kyotaro Horiguchi <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Fujii Masao @ 2022-02-01 14:51 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]>; [email protected] <[email protected]> On 2022/02/01 13:37, [email protected] wrote: > Dear Fujii-san, > > Thank you for reviewing! I attached the latest version. Thanks! > Indeed and it should be avoided. I added a counter to CheckingRemoteServersCallbackItem. > The register function returns the registered item, and it must be set as the argument for > enable and disable functions. > Callback functions will be called when item->counter is larger than zero. This logic sounds complicated to me. I'm afraid that FDW developers may a bit easily misunderstand the logic and make the bug in their FDW. Isn't it simpler to just disable the timeout in core whenever the transaction ends whether committed or aborted, like statement_timeout is disabled after each command? For example, call something like DisableForeignCheckTimeout() in CommitTransaction() etc. > You are right. I think this suggests that error-reporting should be done in the core layer. > For meaningful error reporting, I changed a callback specification > that it should return ForeignServer* which points to downed remote server. This approach seems to assume that FDW must manage all the ForeignServer information so that the callback can return it. Is this assumption valid for all the FDW? How about making FDW trigger a query cancel interrupt by signaling SIGINT to the backend, instead? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION ^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-02-01 04:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-02-01 14:51 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> @ 2022-02-02 12:37 ` [email protected] <[email protected]> 2022-02-15 12:05 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: [email protected] @ 2022-02-02 12:37 UTC (permalink / raw) To: 'Fujii Masao' <[email protected]>; +Cc: [email protected] <[email protected]>; 'Shinya Kato' <[email protected]>; [email protected] <[email protected]> Dear Fujii-san, Thank you for good suggestions. > This logic sounds complicated to me. I'm afraid that FDW developers may a bit > easily misunderstand the logic and make the bug in their FDW. > Isn't it simpler to just disable the timeout in core whenever the transaction ends > whether committed or aborted, like statement_timeout is disabled after each > command? For example, call something like DisableForeignCheckTimeout() in > CommitTransaction() etc. Your idea is that stop the timer at the end of each transactions, right? I had not adopted that because I thought some developers might want not to stop the timer even if transactions ends. It caused complexed situation and not have good usecase, however, so your logic was implemented. > > You are right. I think this suggests that error-reporting should be done in the > core layer. > > For meaningful error reporting, I changed a callback specification > > that it should return ForeignServer* which points to downed remote server. > > This approach seems to assume that FDW must manage all the ForeignServer > information so that the callback can return it. Is this assumption valid for all the > FDW? Not sure, the assumption might be too optimistic. mysql_fdw can easily return ForeignServer* because it caches serverid, but dblink and maybe oracle_fdw cannot. > How about making FDW trigger a query cancel interrupt by signaling SIGINT to > the backend, instead? I understood that the error should be caught by QueryCancelPending. Could you check 0003? Does it follow that? Best Regards, Hayato Kuroda FUJITSU LIMITED Attachments: [application/x-zip-compressed] patchset.zip (7.4K, ../../TYAPR01MB5866BEDCA5096E78ACE25851F5279@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-patchset.zip) download [application/octet-stream] v08_0001_add_checking_infrastracture.patch (13.0K, ../../TYAPR01MB5866BEDCA5096E78ACE25851F5279@TYAPR01MB5866.jpnprd01.prod.outlook.com/3-v08_0001_add_checking_infrastracture.patch) download | inline diff: diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index c9516e03fa..1e321e18c4 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -39,6 +39,7 @@ #include "commands/trigger.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "libpq/be-fsstubs.h" #include "libpq/pqsignal.h" #include "miscadmin.h" @@ -2365,6 +2366,9 @@ CommitTransaction(void) XactTopFullTransactionId = InvalidFullTransactionId; nParallelCurrentXids = 0; + /* stop checking remote servers */ + DisableForeignCheckTimeout(); + /* * done with commit processing, set current transaction state back to * default @@ -2652,6 +2656,9 @@ PrepareTransaction(void) XactTopFullTransactionId = InvalidFullTransactionId; nParallelCurrentXids = 0; + /* stop checking remote servers */ + DisableForeignCheckTimeout(); + /* * done with 1st phase commit processing, set current transaction state * back to default diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index baf328b620..996386fdd4 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -269,6 +269,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) readmessage: HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); mtype = pq_getbyte(); if (mtype == EOF) @@ -300,6 +301,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); /* ... and process it */ switch (mtype) diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 294e22c78c..6084a77336 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -26,7 +26,11 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/timeout.h" +/* for checking remote servers */ +int remote_servers_connection_check_interval = 0; +static CheckingRemoteServersCallbackItem *fdw_callbacks = NULL; /* * GetForeignDataWrapper - look up the foreign-data wrapper by OID. @@ -836,3 +840,83 @@ GetExistingLocalJoinPath(RelOptInfo *joinrel) } return NULL; } + +/* + * Register callbacks for checking remote servers. + * + * This function is intended for use by FDW extensions. + * The checking timeout will be fired after registering the first callback. + */ +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) +{ + CheckingRemoteServersCallbackItem *item; + + item = (CheckingRemoteServersCallbackItem *) + MemoryContextAlloc(TopMemoryContext, + sizeof(CheckingRemoteServersCallbackItem)); + item->callback = callback; + item->arg = arg; + item->next = fdw_callbacks; + fdw_callbacks = item; +} + +/* + * Call callbacks for checking remote servers. + */ +void +CallCheckingRemoteServersCallbacks(void) +{ + CheckingRemoteServersCallbackItem *item; + + for (item = fdw_callbacks; item; item = item->next) + item->callback(item->arg); +} + +/* + * enable timeout if it has been not started yet. + */ +void +EnableForeignCheckTimeout(void) +{ + if (remote_servers_connection_check_interval > 0 && + !get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); +} + +/* + * disable timeout if enabled. + */ +void +DisableForeignCheckTimeout(void) +{ + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); +} + +void +assign_remote_servers_connection_check_interval(int newval, + void *extra) +{ + /* Quick return if we don't have any callbacks */ + if (fdw_callbacks == NULL) + return; + + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + { + if (newval == 0) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + /* + * we don't have to do anything because + * new value will be used in ProcessInterrupts(). + */ + return; + } + + /* Start timeout if anyone wants to */ + if (newval > 0) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, newval); +} \ No newline at end of file diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fda2e9360e..d5de5557e7 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -44,6 +44,7 @@ #include "commands/prepare.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "jit/jit.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -350,6 +351,7 @@ SocketBackend(StringInfo inBuf) * Get message type code from the frontend. */ HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); qtype = pq_getbyte(); @@ -456,6 +458,7 @@ SocketBackend(StringInfo inBuf) */ if (pq_getmessage(inBuf, maxmsglen)) return EOF; /* suitable message already logged */ + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); return qtype; @@ -3225,6 +3228,29 @@ ProcessInterrupts(void) errmsg("connection to client lost"))); } + if (CheckingRemoteServersTimeoutPending) + { + if (CheckingRemoteServersHoldoffCount != 0 || DoingCommandRead) + { + /* + * Skip checking foreign servers while reading messages or commands. + */ + InterruptPending = true; + } + else + { + CheckingRemoteServersTimeoutPending = false; + + /* Check remote servers and re-arm, if still configured. */ + if (remote_servers_connection_check_interval > 0) + { + CallCheckingRemoteServersCallbacks(); + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + } + } + } + /* * If a recovery conflict happens while we are waiting for input from the * client, the client is presumably just sitting idle in a transaction, diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index c26a1a73df..7efa9cff84 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -36,9 +36,11 @@ volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; volatile sig_atomic_t IdleSessionTimeoutPending = false; volatile sig_atomic_t ProcSignalBarrierPending = false; volatile sig_atomic_t LogMemoryContextPending = false; +volatile sig_atomic_t CheckingRemoteServersTimeoutPending = false; volatile uint32 InterruptHoldoffCount = 0; volatile uint32 QueryCancelHoldoffCount = 0; volatile uint32 CritSectionCount = 0; +volatile uint32 CheckingRemoteServersHoldoffCount = 0; int MyProcPid; pg_time_t MyStartTime; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 5b9ed2f6f5..9b0b4ddd58 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -75,6 +75,7 @@ static void LockTimeoutHandler(void); static void IdleInTransactionSessionTimeoutHandler(void); static void IdleSessionTimeoutHandler(void); static void ClientCheckTimeoutHandler(void); +static void CheckingRemoteServersTimeoutHandler(void); static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); @@ -628,6 +629,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, IdleInTransactionSessionTimeoutHandler); RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler); RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler); + RegisterTimeout(CHECKING_REMOTE_SERVERS_TIMEOUT, CheckingRemoteServersTimeoutHandler); } /* @@ -1246,6 +1248,14 @@ ClientCheckTimeoutHandler(void) SetLatch(MyLatch); } +static void +CheckingRemoteServersTimeoutHandler(void) +{ + CheckingRemoteServersTimeoutPending = true; + InterruptPending = true; + SetLatch(MyLatch); +} + /* * Returns true if at least one role is defined in this database cluster. */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index b3fd42e0f1..6dfc171448 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -52,6 +52,7 @@ #include "commands/vacuum.h" #include "commands/variable.h" #include "common/string.h" +#include "foreign/foreign.h" #include "funcapi.h" #include "jit/jit.h" #include "libpq/auth.h" @@ -106,6 +107,7 @@ #include "utils/queryjumble.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/timeout.h" #include "utils/tzparser.h" #include "utils/inval.h" #include "utils/varlena.h" @@ -3600,6 +3602,18 @@ static struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + + { + {"remote_servers_connection_check_interval", PGC_USERSET, CONN_AUTH_SETTINGS, + gettext_noop("Sets the time interval between checks for disconnection of remote servers."), + NULL, + GUC_UNIT_MS + }, + &remote_servers_connection_check_interval, + 0, 0, INT_MAX, + NULL, assign_remote_servers_connection_check_interval, NULL + }, + /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 817d5f5324..b97dc30514 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -90,6 +90,10 @@ # disconnection while running queries; # 0 for never +#remote_servers_connection_check_interval = 0 # time between time between checks for + # foreign server disconnection; + # 0 for never + # - Authentication - #authentication_timeout = 1min # 1s-600s diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index 75538110fc..900cc4e3aa 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -81,4 +81,24 @@ extern List *GetForeignColumnOptions(Oid relid, AttrNumber attnum); extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok); extern Oid get_foreign_server_oid(const char *servername, bool missing_ok); + +/* functions and variables for fdw checking. */ +typedef void (*CheckingRemoteServersCallback) (void *arg); +typedef struct CheckingRemoteServersCallbackItem +{ + struct CheckingRemoteServersCallbackItem *next; + CheckingRemoteServersCallback callback; + void *arg; +} CheckingRemoteServersCallbackItem; + +extern void RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg); +extern void CallCheckingRemoteServersCallbacks(void); + +extern void EnableForeignCheckTimeout(void); +extern void DisableForeignCheckTimeout(void); + +extern int remote_servers_connection_check_interval; +extern void assign_remote_servers_connection_check_interval(int newval, + void *extra); #endif /* FOREIGN_H */ diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 02276d3edd..0319fe1605 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -98,10 +98,13 @@ extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending; extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending; extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost; +extern PGDLLIMPORT volatile sig_atomic_t CheckingRemoteServersTimeoutPending; + /* these are marked volatile because they are examined by signal handlers: */ extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount; extern PGDLLIMPORT volatile uint32 CritSectionCount; +extern PGDLLIMPORT volatile uint32 CheckingRemoteServersHoldoffCount; /* in tcop/postgres.c */ extern void ProcessInterrupts(void); @@ -126,7 +129,7 @@ do { \ /* Is ProcessInterrupts() guaranteed to clear InterruptPending? */ #define INTERRUPTS_CAN_BE_PROCESSED() \ (InterruptHoldoffCount == 0 && CritSectionCount == 0 && \ - QueryCancelHoldoffCount == 0) + QueryCancelHoldoffCount == 0 && CheckingRemoteServersHoldoffCount == 0) #define HOLD_INTERRUPTS() (InterruptHoldoffCount++) @@ -152,6 +155,13 @@ do { \ CritSectionCount--; \ } while(0) +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() (CheckingRemoteServersHoldoffCount++) + +#define RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS() \ +do { \ + Assert(CheckingRemoteServersHoldoffCount > 0); \ + CheckingRemoteServersHoldoffCount--; \ +} while(0) /***************************************************************************** * globals.h -- * diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h index 099f91c61d..7f02a8b491 100644 --- a/src/include/utils/timeout.h +++ b/src/include/utils/timeout.h @@ -34,6 +34,7 @@ typedef enum TimeoutId IDLE_SESSION_TIMEOUT, CLIENT_CONNECTION_CHECK_TIMEOUT, STARTUP_PROGRESS_TIMEOUT, + CHECKING_REMOTE_SERVERS_TIMEOUT, /* First user-definable timeout reason */ USER_TIMEOUT, /* Maximum number of timeout reasons */ [application/octet-stream] v08_0002_add_doc.patch (2.8K, ../../TYAPR01MB5866BEDCA5096E78ACE25851F5279@TYAPR01MB5866.jpnprd01.prod.outlook.com/4-v08_0002_add_doc.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 692d8a2a17..fb400a3a91 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1035,6 +1035,25 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-remote-servers-connection-check-interval" xreflabel="remote_servers_connection_check_interval"> + <term><varname>remote_servers_connection_check_interval</varname> (<type>integer</type>) + <indexterm> + <primary><varname>remote_servers_connection_check_interval</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the time interval between optional checks that the remote servers + are still alive. If this parameter is set to a valid value, registered + callback functions are executed at the specified time interval. + If your FDW extension does not implement a callback function, nothing + happens after the specified amount of time. If the value is specified + without units, it is taken as milliseconds. The default value is + <literal>0</literal>, this means the feature is disabled. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index dbf5dd3d15..8e245f87dc 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -2136,4 +2136,34 @@ GetForeignServerByName(const char *name, bool missing_ok); </sect1> + <sect1 id="fdw-checking-remote-servers"> + <title>Checking the Health of Remote Servers via Foreign Data Wrapper</title> + + <para> + For verifying the health of remote servers, callback functions can be + registered that runs periodically. The time interval is controlled by + <xref linkend="guc-remote-servers-connection-check-interval"/>. + The API for registering functions is: + <programlisting> +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, void *arg); + </programlisting> + Only functions that return nothing can be registered as callback functions. + Please set to <literal>void* arg</literal> if you want to pass an argument. + If problems are found by the health check and want to abort the transaction, + please send a signal <systemitem>SIGINT</systemitem> to the backend process. + </para> + <para> + Note that starting the periodic run is FDW's responsibility. + If you want to start verification, the following function must be performed + after registering callback functions: + <programlisting> +int +EnableForeignCheckTimeout(void) + </programlisting> + The periodic run will stop at end of each transaction. + </para> + </sect1> + + </chapter> ^ permalink raw reply [nested|flat] 10+ messages in thread
* RE: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-02-01 04:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-02-01 14:51 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-02-02 12:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> @ 2022-02-15 12:05 ` [email protected] <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: [email protected] @ 2022-02-15 12:05 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: 'Shinya Kato' <[email protected]>; [email protected] <[email protected]>; 'Fujii Masao' <[email protected]> Dear Hackers, I found patches we depend have been committed, so rebased. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=50e570a59e7f86bb41f029a66b781fc79b8d5... In this version there is a little bit change in part of postgres_fdw. A system checking by WaitEventSetCanReportClosed() is added because some OSes cannot wait WL_SOCKET_CLOSED event. Note that test cannot be added in the regression test because cfbot may be not happy. In my environment a test that contained in previous patches works well. 0001, 0002 is not changed from previous version. Best Regards, Hayato Kuroda FUJITSU LIMITED Attachments: [application/octet-stream] v09_0003_helth_check_for_postgres_fdw.patch (4.2K, ../../TYAPR01MB5866F5860ED6E1D541559CF2F5349@TYAPR01MB5866.jpnprd01.prod.outlook.com/2-v09_0003_helth_check_for_postgres_fdw.patch) download | inline diff: diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 29fcb6a76e..10316d2f10 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -109,6 +109,7 @@ static void pgfdw_abort_cleanup(ConnCacheEntry *entry, const char *sql, bool toplevel); static bool UserMappingPasswordRequired(UserMapping *user); static bool disconnect_cached_connections(Oid serverid); +static bool pgfdw_connection_check_internal(PGconn *conn); /* * Get a PGconn which can be used to execute queries on the remote PostgreSQL @@ -153,11 +154,17 @@ GetConnection(UserMapping *user, bool will_prep_stmt, PgFdwConnState **state) pgfdw_inval_callback, (Datum) 0); CacheRegisterSyscacheCallback(USERMAPPINGOID, pgfdw_inval_callback, (Datum) 0); + + /* Register a callback fore checking remote servers */ + RegisterCheckingRemoteServersCallback(pgfdw_connection_check, NULL); } /* Set flag that we did GetConnection during the current transaction */ xact_got_connection = true; + /* Start checking timer */ + EnableForeignCheckTimeout(); + /* Create hash key for the entry. Assume no pad bytes in key struct */ key = user->umid; @@ -1709,3 +1716,71 @@ disconnect_cached_connections(Oid serverid) return result; } + +/* + * Callback function for checking remote servers. + * + * This function searches the hash table from the beginning + * and performs a health-check on each entry. + * + * Raise SIGINT if someone might be down, otherwise do nothing. + */ +void +pgfdw_connection_check(void *args) +{ + HASH_SEQ_STATUS scan; + ConnCacheEntry *entry; + + /* Exit immediately if hash is not initialized. */ + if (!ConnectionHash) + return; + + /* + * checking will be done by waiting WL_SOCKET_CLOSED event, + * so exit immediately if it cannot be used in this system. + */ + if (!WaitEventSetCanReportClosed()) + return; + + hash_seq_init(&scan, ConnectionHash); + while ((entry = (ConnCacheEntry *) hash_seq_search(&scan))) + { + if (entry->conn == NULL || entry->xact_depth == 0) + continue; + if (!pgfdw_connection_check_internal(entry->conn)) + { + ForeignServer *server = GetForeignServer(entry->serverid); + elog(LOG, "Foreign Server %s might be down.", server->servername); + disconnect_pg_server(entry); + raise(SIGINT); + } + } + + return; +} + +/* + * helper function for pgfdw_connection_check + */ +static bool +pgfdw_connection_check_internal(PGconn *conn) +{ + WaitEventSet *eventset; + WaitEvent events; + + Assert(WaitEventSetCanReportClosed()); + + eventset = CreateWaitEventSet(CurrentMemoryContext, 1); + AddWaitEventToSet(eventset, WL_SOCKET_CLOSED, PQsocket(conn), NULL, NULL); + + WaitEventSetWait(eventset, 0, &events, 1, 0); + + if (events.events & WL_SOCKET_CLOSED) + { + FreeWaitEventSet(eventset); + return false; + } + FreeWaitEventSet(eventset); + + return true; +} diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h index 8ae79e97e4..0f4f4641c6 100644 --- a/contrib/postgres_fdw/postgres_fdw.h +++ b/contrib/postgres_fdw/postgres_fdw.h @@ -151,6 +151,7 @@ extern PGresult *pgfdw_exec_query(PGconn *conn, const char *query, PgFdwConnState *state); extern void pgfdw_report_error(int elevel, PGresult *res, PGconn *conn, bool clear, const char *sql); +extern void pgfdw_connection_check(void *args); /* in option.c */ extern int ExtractConnectionOptions(List *defelems, diff --git a/doc/src/sgml/postgres-fdw.sgml b/doc/src/sgml/postgres-fdw.sgml index 2bb31f1125..228f4629a5 100644 --- a/doc/src/sgml/postgres-fdw.sgml +++ b/doc/src/sgml/postgres-fdw.sgml @@ -774,6 +774,16 @@ postgres=# SELECT postgres_fdw_disconnect_all(); connections that are no longer necessary, thereby releasing connections on the foreign server. </para> + + <para> + During a remote transaction, postgres_fdw checks the health of used connections + if <xref linkend="guc-remote-servers-connection-check-interval"/> sets to + greater than zero. When losing a remote connection is detected, + the running transaction is aborted. Note that this feature is performed + by polling the socket, and available platforms are the + same as <xref linkend="guc-client-connection-check-interval"/>. + </para> + </sect2> <sect2> [application/octet-stream] v09_0002_add_doc_about_infrastructure.patch (2.8K, ../../TYAPR01MB5866F5860ED6E1D541559CF2F5349@TYAPR01MB5866.jpnprd01.prod.outlook.com/3-v09_0002_add_doc_about_infrastructure.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 53b361e7a9..6e4f67aed9 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1035,6 +1035,25 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-remote-servers-connection-check-interval" xreflabel="remote_servers_connection_check_interval"> + <term><varname>remote_servers_connection_check_interval</varname> (<type>integer</type>) + <indexterm> + <primary><varname>remote_servers_connection_check_interval</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the time interval between optional checks that the remote servers + are still alive. If this parameter is set to a valid value, registered + callback functions are executed at the specified time interval. + If your FDW extension does not implement a callback function, nothing + happens after the specified amount of time. If the value is specified + without units, it is taken as milliseconds. The default value is + <literal>0</literal>, this means the feature is disabled. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index dbf5dd3d15..8e245f87dc 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -2136,4 +2136,34 @@ GetForeignServerByName(const char *name, bool missing_ok); </sect1> + <sect1 id="fdw-checking-remote-servers"> + <title>Checking the Health of Remote Servers via Foreign Data Wrapper</title> + + <para> + For verifying the health of remote servers, callback functions can be + registered that runs periodically. The time interval is controlled by + <xref linkend="guc-remote-servers-connection-check-interval"/>. + The API for registering functions is: + <programlisting> +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, void *arg); + </programlisting> + Only functions that return nothing can be registered as callback functions. + Please set to <literal>void* arg</literal> if you want to pass an argument. + If problems are found by the health check and want to abort the transaction, + please send a signal <systemitem>SIGINT</systemitem> to the backend process. + </para> + <para> + Note that starting the periodic run is FDW's responsibility. + If you want to start verification, the following function must be performed + after registering callback functions: + <programlisting> +int +EnableForeignCheckTimeout(void) + </programlisting> + The periodic run will stop at end of each transaction. + </para> + </sect1> + + </chapter> [application/octet-stream] v09_0001_add_checking_infrastracture.patch (13.0K, ../../TYAPR01MB5866F5860ED6E1D541559CF2F5349@TYAPR01MB5866.jpnprd01.prod.outlook.com/4-v09_0001_add_checking_infrastracture.patch) download | inline diff: diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index c9516e03fa..1e321e18c4 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -39,6 +39,7 @@ #include "commands/trigger.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "libpq/be-fsstubs.h" #include "libpq/pqsignal.h" #include "miscadmin.h" @@ -2365,6 +2366,9 @@ CommitTransaction(void) XactTopFullTransactionId = InvalidFullTransactionId; nParallelCurrentXids = 0; + /* stop checking remote servers */ + DisableForeignCheckTimeout(); + /* * done with commit processing, set current transaction state back to * default @@ -2652,6 +2656,9 @@ PrepareTransaction(void) XactTopFullTransactionId = InvalidFullTransactionId; nParallelCurrentXids = 0; + /* stop checking remote servers */ + DisableForeignCheckTimeout(); + /* * done with 1st phase commit processing, set current transaction state * back to default diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c index baf328b620..996386fdd4 100644 --- a/src/backend/commands/copyfromparse.c +++ b/src/backend/commands/copyfromparse.c @@ -269,6 +269,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) readmessage: HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); mtype = pq_getbyte(); if (mtype == EOF) @@ -300,6 +301,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread) ereport(ERROR, (errcode(ERRCODE_CONNECTION_FAILURE), errmsg("unexpected EOF on client connection with an open transaction"))); + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); /* ... and process it */ switch (mtype) diff --git a/src/backend/foreign/foreign.c b/src/backend/foreign/foreign.c index 294e22c78c..6084a77336 100644 --- a/src/backend/foreign/foreign.c +++ b/src/backend/foreign/foreign.c @@ -26,7 +26,11 @@ #include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#include "utils/timeout.h" +/* for checking remote servers */ +int remote_servers_connection_check_interval = 0; +static CheckingRemoteServersCallbackItem *fdw_callbacks = NULL; /* * GetForeignDataWrapper - look up the foreign-data wrapper by OID. @@ -836,3 +840,83 @@ GetExistingLocalJoinPath(RelOptInfo *joinrel) } return NULL; } + +/* + * Register callbacks for checking remote servers. + * + * This function is intended for use by FDW extensions. + * The checking timeout will be fired after registering the first callback. + */ +void +RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) +{ + CheckingRemoteServersCallbackItem *item; + + item = (CheckingRemoteServersCallbackItem *) + MemoryContextAlloc(TopMemoryContext, + sizeof(CheckingRemoteServersCallbackItem)); + item->callback = callback; + item->arg = arg; + item->next = fdw_callbacks; + fdw_callbacks = item; +} + +/* + * Call callbacks for checking remote servers. + */ +void +CallCheckingRemoteServersCallbacks(void) +{ + CheckingRemoteServersCallbackItem *item; + + for (item = fdw_callbacks; item; item = item->next) + item->callback(item->arg); +} + +/* + * enable timeout if it has been not started yet. + */ +void +EnableForeignCheckTimeout(void) +{ + if (remote_servers_connection_check_interval > 0 && + !get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); +} + +/* + * disable timeout if enabled. + */ +void +DisableForeignCheckTimeout(void) +{ + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); +} + +void +assign_remote_servers_connection_check_interval(int newval, + void *extra) +{ + /* Quick return if we don't have any callbacks */ + if (fdw_callbacks == NULL) + return; + + if (get_timeout_active(CHECKING_REMOTE_SERVERS_TIMEOUT)) + { + if (newval == 0) + disable_timeout(CHECKING_REMOTE_SERVERS_TIMEOUT, false); + + /* + * we don't have to do anything because + * new value will be used in ProcessInterrupts(). + */ + return; + } + + /* Start timeout if anyone wants to */ + if (newval > 0) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, newval); +} \ No newline at end of file diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index fda2e9360e..d5de5557e7 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -44,6 +44,7 @@ #include "commands/prepare.h" #include "common/pg_prng.h" #include "executor/spi.h" +#include "foreign/foreign.h" #include "jit/jit.h" #include "libpq/libpq.h" #include "libpq/pqformat.h" @@ -350,6 +351,7 @@ SocketBackend(StringInfo inBuf) * Get message type code from the frontend. */ HOLD_CANCEL_INTERRUPTS(); + HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS(); pq_startmsgread(); qtype = pq_getbyte(); @@ -456,6 +458,7 @@ SocketBackend(StringInfo inBuf) */ if (pq_getmessage(inBuf, maxmsglen)) return EOF; /* suitable message already logged */ + RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS(); RESUME_CANCEL_INTERRUPTS(); return qtype; @@ -3225,6 +3228,29 @@ ProcessInterrupts(void) errmsg("connection to client lost"))); } + if (CheckingRemoteServersTimeoutPending) + { + if (CheckingRemoteServersHoldoffCount != 0 || DoingCommandRead) + { + /* + * Skip checking foreign servers while reading messages or commands. + */ + InterruptPending = true; + } + else + { + CheckingRemoteServersTimeoutPending = false; + + /* Check remote servers and re-arm, if still configured. */ + if (remote_servers_connection_check_interval > 0) + { + CallCheckingRemoteServersCallbacks(); + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); + } + } + } + /* * If a recovery conflict happens while we are waiting for input from the * client, the client is presumably just sitting idle in a transaction, diff --git a/src/backend/utils/init/globals.c b/src/backend/utils/init/globals.c index c26a1a73df..7efa9cff84 100644 --- a/src/backend/utils/init/globals.c +++ b/src/backend/utils/init/globals.c @@ -36,9 +36,11 @@ volatile sig_atomic_t IdleInTransactionSessionTimeoutPending = false; volatile sig_atomic_t IdleSessionTimeoutPending = false; volatile sig_atomic_t ProcSignalBarrierPending = false; volatile sig_atomic_t LogMemoryContextPending = false; +volatile sig_atomic_t CheckingRemoteServersTimeoutPending = false; volatile uint32 InterruptHoldoffCount = 0; volatile uint32 QueryCancelHoldoffCount = 0; volatile uint32 CritSectionCount = 0; +volatile uint32 CheckingRemoteServersHoldoffCount = 0; int MyProcPid; pg_time_t MyStartTime; diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c index 02207a5b6f..9ce99df962 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -80,6 +80,7 @@ static void LockTimeoutHandler(void); static void IdleInTransactionSessionTimeoutHandler(void); static void IdleSessionTimeoutHandler(void); static void ClientCheckTimeoutHandler(void); +static void CheckingRemoteServersTimeoutHandler(void); static bool ThereIsAtLeastOneRole(void); static void process_startup_options(Port *port, bool am_superuser); static void process_settings(Oid databaseid, Oid roleid); @@ -700,6 +701,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username, IdleInTransactionSessionTimeoutHandler); RegisterTimeout(IDLE_SESSION_TIMEOUT, IdleSessionTimeoutHandler); RegisterTimeout(CLIENT_CONNECTION_CHECK_TIMEOUT, ClientCheckTimeoutHandler); + RegisterTimeout(CHECKING_REMOTE_SERVERS_TIMEOUT, CheckingRemoteServersTimeoutHandler); } /* @@ -1318,6 +1320,14 @@ ClientCheckTimeoutHandler(void) SetLatch(MyLatch); } +static void +CheckingRemoteServersTimeoutHandler(void) +{ + CheckingRemoteServersTimeoutPending = true; + InterruptPending = true; + SetLatch(MyLatch); +} + /* * Returns true if at least one role is defined in this database cluster. */ diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e2fe219aa8..d558117e64 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -52,6 +52,7 @@ #include "commands/vacuum.h" #include "commands/variable.h" #include "common/string.h" +#include "foreign/foreign.h" #include "funcapi.h" #include "jit/jit.h" #include "libpq/auth.h" @@ -106,6 +107,7 @@ #include "utils/queryjumble.h" #include "utils/rls.h" #include "utils/snapmgr.h" +#include "utils/timeout.h" #include "utils/tzparser.h" #include "utils/inval.h" #include "utils/varlena.h" @@ -3600,6 +3602,18 @@ static struct config_int ConfigureNamesInt[] = NULL, NULL, NULL }, + + { + {"remote_servers_connection_check_interval", PGC_USERSET, CONN_AUTH_SETTINGS, + gettext_noop("Sets the time interval between checks for disconnection of remote servers."), + NULL, + GUC_UNIT_MS + }, + &remote_servers_connection_check_interval, + 0, 0, INT_MAX, + NULL, assign_remote_servers_connection_check_interval, NULL + }, + /* End-of-list marker */ { {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 56d0bee6d9..04fa73967b 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -90,6 +90,10 @@ # disconnection while running queries; # 0 for never +#remote_servers_connection_check_interval = 0 # time between time between checks for + # foreign server disconnection; + # 0 for never + # - Authentication - #authentication_timeout = 1min # 1s-600s diff --git a/src/include/foreign/foreign.h b/src/include/foreign/foreign.h index 75538110fc..900cc4e3aa 100644 --- a/src/include/foreign/foreign.h +++ b/src/include/foreign/foreign.h @@ -81,4 +81,24 @@ extern List *GetForeignColumnOptions(Oid relid, AttrNumber attnum); extern Oid get_foreign_data_wrapper_oid(const char *fdwname, bool missing_ok); extern Oid get_foreign_server_oid(const char *servername, bool missing_ok); + +/* functions and variables for fdw checking. */ +typedef void (*CheckingRemoteServersCallback) (void *arg); +typedef struct CheckingRemoteServersCallbackItem +{ + struct CheckingRemoteServersCallbackItem *next; + CheckingRemoteServersCallback callback; + void *arg; +} CheckingRemoteServersCallbackItem; + +extern void RegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg); +extern void CallCheckingRemoteServersCallbacks(void); + +extern void EnableForeignCheckTimeout(void); +extern void DisableForeignCheckTimeout(void); + +extern int remote_servers_connection_check_interval; +extern void assign_remote_servers_connection_check_interval(int newval, + void *extra); #endif /* FOREIGN_H */ diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 0abc3ad540..b14152e66b 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -98,10 +98,13 @@ extern PGDLLIMPORT volatile sig_atomic_t LogMemoryContextPending; extern PGDLLIMPORT volatile sig_atomic_t CheckClientConnectionPending; extern PGDLLIMPORT volatile sig_atomic_t ClientConnectionLost; +extern PGDLLIMPORT volatile sig_atomic_t CheckingRemoteServersTimeoutPending; + /* these are marked volatile because they are examined by signal handlers: */ extern PGDLLIMPORT volatile uint32 InterruptHoldoffCount; extern PGDLLIMPORT volatile uint32 QueryCancelHoldoffCount; extern PGDLLIMPORT volatile uint32 CritSectionCount; +extern PGDLLIMPORT volatile uint32 CheckingRemoteServersHoldoffCount; /* in tcop/postgres.c */ extern void ProcessInterrupts(void); @@ -126,7 +129,7 @@ do { \ /* Is ProcessInterrupts() guaranteed to clear InterruptPending? */ #define INTERRUPTS_CAN_BE_PROCESSED() \ (InterruptHoldoffCount == 0 && CritSectionCount == 0 && \ - QueryCancelHoldoffCount == 0) + QueryCancelHoldoffCount == 0 && CheckingRemoteServersHoldoffCount == 0) #define HOLD_INTERRUPTS() (InterruptHoldoffCount++) @@ -152,6 +155,13 @@ do { \ CritSectionCount--; \ } while(0) +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() (CheckingRemoteServersHoldoffCount++) + +#define RESUME_CHECKING_REMOTE_SERVERS_INTERRUPTS() \ +do { \ + Assert(CheckingRemoteServersHoldoffCount > 0); \ + CheckingRemoteServersHoldoffCount--; \ +} while(0) /***************************************************************************** * globals.h -- * diff --git a/src/include/utils/timeout.h b/src/include/utils/timeout.h index 099f91c61d..7f02a8b491 100644 --- a/src/include/utils/timeout.h +++ b/src/include/utils/timeout.h @@ -34,6 +34,7 @@ typedef enum TimeoutId IDLE_SESSION_TIMEOUT, CLIENT_CONNECTION_CHECK_TIMEOUT, STARTUP_PROGRESS_TIMEOUT, + CHECKING_REMOTE_SERVERS_TIMEOUT, /* First user-definable timeout reason */ USER_TIMEOUT, /* Maximum number of timeout reasons */ ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [Proposal] Add foreign-server health checks infrastructure 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-21 06:08 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-01-23 05:36 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-02-01 04:37 ` RE: [Proposal] Add foreign-server health checks infrastructure [email protected] <[email protected]> 2022-02-01 14:51 ` Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> @ 2022-02-16 02:50 ` Kyotaro Horiguchi <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Kyotaro Horiguchi @ 2022-02-16 02:50 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected] At Tue, 1 Feb 2022 23:51:54 +0900, Fujii Masao <[email protected]> wrote in > This logic sounds complicated to me. I'm afraid that FDW developers > may a bit easily misunderstand the logic and make the bug in their > FDW. > Isn't it simpler to just disable the timeout in core whenever the > transaction ends whether committed or aborted, like statement_timeout > is disabled after each command? For example, call something like > DisableForeignCheckTimeout() in CommitTransaction() etc. > This approach seems to assume that FDW must manage all the > ForeignServer information so that the callback can return it. Is this > assumption valid for all the FDW? FWIW, I'm not sure this feature necessarily requires core support dedicated to FDWs. The core have USER_TIMEOUT feature already and FDWs are not necessarily connection based. It seems better if FDWs can implement health check feature without core support and it seems possible. Or at least the core feature should be more generic and simpler. Why don't we just expose InTransactionHealthCheckCallbacks or something and operating functions on it? > How about making FDW trigger a query cancel interrupt by signaling > SIGINT to the backend, instead? Mmm. AFAICS the running command will stop with "canceling statement due to user request", which is a hoax. We need a more decent message there. I understand that the motive of this patch is "to avoid wasted long local work when fdw-connection dies". In regard to the workload in your first mail, it is easily avoided by ending the transaction as soon as remote access ends. This feature doesn't work for the case "begin; <long local query>; <fdw access>". But the same measure also works in that case. So the only case where this feature is useful is "begin; <fdw-access>; <some long work>; <fdw-access>; end;". But in the first place how frequently do you expecting remote-connection close happens? If that happens so frequently, you might need to recheck the system health before implementing this feature. Since it is correctly detected when something really went wrong, I feel that it is a bit too complex for the usefulness especially for the core part. In conclusion, as my humble opinion I would like to propose to reduce this feature to: - Just periodically check health (in any aspect) of all live connections regardless of the session state. - If an existing connection is found to be dead, just try canceling the query (or sending query cancel). One issue with it is how to show the decent message for the query cancel, but maybe we can have a global variable that suggests the reason for the cancel. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 10+ messages in thread
* [PATCH v3 2/3] move archive module exports to dedicated headers @ 2023-01-28 05:16 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Nathan Bossart @ 2023-01-28 05:16 UTC (permalink / raw) --- contrib/basic_archive/basic_archive.c | 2 +- src/backend/postmaster/pgarch.c | 2 ++ src/backend/postmaster/shell_archive.c | 3 +- src/backend/utils/misc/guc_tables.c | 1 + src/include/postmaster/archive_module.h | 47 +++++++++++++++++++++++++ src/include/postmaster/pgarch.h | 39 -------------------- src/include/postmaster/shell_archive.h | 22 ++++++++++++ 7 files changed, 75 insertions(+), 41 deletions(-) create mode 100644 src/include/postmaster/archive_module.h create mode 100644 src/include/postmaster/shell_archive.h diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 3d29711a31..87bbb2174d 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -32,7 +32,7 @@ #include "common/int.h" #include "miscadmin.h" -#include "postmaster/pgarch.h" +#include "postmaster/archive_module.h" #include "storage/copydir.h" #include "storage/fd.h" #include "utils/guc.h" diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index dda6698509..ec47b2cc20 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -34,8 +34,10 @@ #include "lib/binaryheap.h" #include "libpq/pqsignal.h" #include "pgstat.h" +#include "postmaster/archive_module.h" #include "postmaster/interrupt.h" #include "postmaster/pgarch.h" +#include "postmaster/shell_archive.h" #include "storage/fd.h" #include "storage/ipc.h" #include "storage/latch.h" diff --git a/src/backend/postmaster/shell_archive.c b/src/backend/postmaster/shell_archive.c index 806b81c3f2..35f88c1222 100644 --- a/src/backend/postmaster/shell_archive.c +++ b/src/backend/postmaster/shell_archive.c @@ -20,7 +20,8 @@ #include "access/xlog.h" #include "common/percentrepl.h" #include "pgstat.h" -#include "postmaster/pgarch.h" +#include "postmaster/archive_module.h" +#include "postmaster/shell_archive.h" static bool shell_archive_configured(void); static bool shell_archive_file(const char *file, const char *path); diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index c5a95f5dcc..663c6e0011 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -52,6 +52,7 @@ #include "parser/parse_expr.h" #include "parser/parser.h" #include "pgstat.h" +#include "postmaster/archive_module.h" #include "postmaster/autovacuum.h" #include "postmaster/bgworker_internals.h" #include "postmaster/bgwriter.h" diff --git a/src/include/postmaster/archive_module.h b/src/include/postmaster/archive_module.h new file mode 100644 index 0000000000..f5015ff95d --- /dev/null +++ b/src/include/postmaster/archive_module.h @@ -0,0 +1,47 @@ +/*------------------------------------------------------------------------- + * + * archive_module.h + * Exports for archive modules. + * + * Copyright (c) 2022-2023, PostgreSQL Global Development Group + * + * src/include/postmaster/archive_module.h + * + *------------------------------------------------------------------------- + */ +#ifndef _ARCHIVE_MODULE_H +#define _ARCHIVE_MODULE_H + +/* + * The value of the archive_library GUC. + */ +extern PGDLLIMPORT char *XLogArchiveLibrary; + +/* + * Archive module callbacks + * + * These callback functions should be defined by archive libraries and returned + * via _PG_archive_module_init(). ArchiveFileCB is the only required callback. + * For more information about the purpose of each callback, refer to the + * archive modules documentation. + */ +typedef bool (*ArchiveCheckConfiguredCB) (void); +typedef bool (*ArchiveFileCB) (const char *file, const char *path); +typedef void (*ArchiveShutdownCB) (void); + +typedef struct ArchiveModuleCallbacks +{ + ArchiveCheckConfiguredCB check_configured_cb; + ArchiveFileCB archive_file_cb; + ArchiveShutdownCB shutdown_cb; +} ArchiveModuleCallbacks; + +/* + * Type of the shared library symbol _PG_archive_module_init that is looked + * up when loading an archive library. + */ +typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb); + +extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb); + +#endif /* _ARCHIVE_MODULE_H */ diff --git a/src/include/postmaster/pgarch.h b/src/include/postmaster/pgarch.h index bcd51dfad6..3bd4fac71e 100644 --- a/src/include/postmaster/pgarch.h +++ b/src/include/postmaster/pgarch.h @@ -33,43 +33,4 @@ extern void PgArchiverMain(void) pg_attribute_noreturn(); extern void PgArchWakeup(void); extern void PgArchForceDirScan(void); -/* - * The value of the archive_library GUC. - */ -extern PGDLLIMPORT char *XLogArchiveLibrary; - -/* - * Archive module callbacks - * - * These callback functions should be defined by archive libraries and returned - * via _PG_archive_module_init(). ArchiveFileCB is the only required callback. - * For more information about the purpose of each callback, refer to the - * archive modules documentation. - */ -typedef bool (*ArchiveCheckConfiguredCB) (void); -typedef bool (*ArchiveFileCB) (const char *file, const char *path); -typedef void (*ArchiveShutdownCB) (void); - -typedef struct ArchiveModuleCallbacks -{ - ArchiveCheckConfiguredCB check_configured_cb; - ArchiveFileCB archive_file_cb; - ArchiveShutdownCB shutdown_cb; -} ArchiveModuleCallbacks; - -/* - * Type of the shared library symbol _PG_archive_module_init that is looked - * up when loading an archive library. - */ -typedef void (*ArchiveModuleInit) (ArchiveModuleCallbacks *cb); - -extern PGDLLEXPORT void _PG_archive_module_init(ArchiveModuleCallbacks *cb); - -/* - * Since the logic for archiving via a shell command is in the core server - * and does not need to be loaded via a shared library, it has a special - * initialization function. - */ -extern void shell_archive_init(ArchiveModuleCallbacks *cb); - #endif /* _PGARCH_H */ diff --git a/src/include/postmaster/shell_archive.h b/src/include/postmaster/shell_archive.h new file mode 100644 index 0000000000..f6943c0aba --- /dev/null +++ b/src/include/postmaster/shell_archive.h @@ -0,0 +1,22 @@ +/*------------------------------------------------------------------------- + * + * shell_archive.h + * Exports for archiving via shell. + * + * Copyright (c) 2022-2023, PostgreSQL Global Development Group + * + * src/include/postmaster/shell_archive.h + * + *------------------------------------------------------------------------- + */ +#ifndef _SHELL_ARCHIVE_H +#define _SHELL_ARCHIVE_H + +/* + * Since the logic for archiving via a shell command is in the core server + * and does not need to be loaded via a shared library, it has a special + * initialization function. + */ +extern void shell_archive_init(ArchiveModuleCallbacks *cb); + +#endif /* _SHELL_ARCHIVE_H */ -- 2.25.1 --sm4nu43k4a2Rpi4c Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0003-restructure-archive-modules-API.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2023-01-28 05:16 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-01-18 15:42 Re: [Proposal] Add foreign-server health checks infrastructure Fujii Masao <[email protected]> 2022-01-20 08:21 ` [email protected] <[email protected]> 2022-01-21 06:08 ` [email protected] <[email protected]> 2022-01-23 05:36 ` Fujii Masao <[email protected]> 2022-02-01 04:37 ` [email protected] <[email protected]> 2022-02-01 14:51 ` Fujii Masao <[email protected]> 2022-02-02 12:37 ` [email protected] <[email protected]> 2022-02-15 12:05 ` [email protected] <[email protected]> 2022-02-16 02:50 ` Kyotaro Horiguchi <[email protected]> 2023-01-28 05:16 [PATCH v3 2/3] move archive module exports to dedicated headers Nathan Bossart <[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