RE: [Proposal] Add foreign-server health checks infrastructure

From: "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>
To: "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com>
Cc: Andres Freund <andres(at)anarazel(dot)de>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "Shinya11(dot)Kato(at)oss(dot)nttdata(dot)com" <Shinya11(dot)Kato(at)oss(dot)nttdata(dot)com>, "zyu(at)yugabyte(dot)com" <zyu(at)yugabyte(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, 'Önder Kalacı' <onderkalaci(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
Subject: RE: [Proposal] Add foreign-server health checks infrastructure
Date: 2022-10-17 13:01:37
Message-ID: TYAPR01MB5866D32CDA028B31619E79EAF5299@TYAPR01MB5866.jpnprd01.prod.outlook.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dear Osumi-san,

Thanks for reviewing! PSA v17 patchset.

> (1) v16-0001 : definition of a new structure
>
> CheckingRemoteServersCallbackItem can be added to typedefs.list.

Added.

> (2) v16-0001 : UnRegisterCheckingRemoteServersCallback's header comment
>
> +void
> +UnRegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback
> callback,
> +
> void *arg)
> +{
>
> Looks require a header comment for this function,
> because in this file, all other functions have each one.

Added. Additionally, I renamed this function to Unregister...,
this follows other unregister functions.

> (3) v16-0002 : better place to declare new variables
>
> New variables 'old' and 'server' in GetConnection() can be moved to
> a scope of 'if' statement where those are used.
>
> @@ -138,6 +149,8 @@ GetConnection(UserMapping *user, bool will_prep_stmt,
> PgFdwConnState **state)
> ConnCacheEntry *entry;
> ConnCacheKey key;
> MemoryContext ccxt = CurrentMemoryContext;
> + MemoryContext old;
> + ForeignServer *server = GetForeignServer(user->serverid);

Fixed.

> (4) v16-0002 : typo in pgfdw_connection_check_internal()
>
>
> + /* return false is if the socket seems to be closed. */
>
> It should be "return false if the socket seems to be closed" ?

Fixed.

> (5) v16-0002 : pgfdw_connection_check's message
>
> When I tested the new feature, I got a below message.
>
> "ERROR: Foreign Server my_external_server might be down."
>
> I think we can wrap the server name with double quotes
> so that the message is more aligned with existing error message
> like connect_pg_server.

Fixed.
...I'm not sure whether this message is still need, because
the disconnection was delegated to XactCallback, and the function did following output:

```
WARNING: FATAL: terminating connection due to administrator command
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
```

> (6) v16-003 : typo
>
> + Authors needs to register the callback function via following API.
>
> Should be "Authors need to register the ...".

Fixed.

> (7) v16-0004 : unnecessary file
>
> I think we can remove a new file which looks like a log file.
>
> .../postgres_fdw/expected/postgres_fdw_1.out

This is needed to pass the test on the windows platform. See:
https://www.postgresql.org/docs/devel/regress-variant.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Attachment Content-Type Size
v17-0001-Add-an-infrastracture-for-checking-remote-server.patch application/octet-stream 9.3 KB
v17-0002-postgres_fdw-Implement-health-check-feature.patch application/octet-stream 12.4 KB
v17-0003-add-doc.patch application/octet-stream 4.7 KB
v17-0004-add-test.patch application/octet-stream 1.1 MB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2022-10-17 13:15:35 Re: thinko in basic_archive.c
Previous Message kuroda.hayato@fujitsu.com 2022-10-17 12:25:21 RE: [Proposal] Add foreign-server health checks infrastructure