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 |
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 |