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

2023-12-11 Thread Hayato Kuroda (Fujitsu)
Dear Shubham, > > I am failing to apply the latest > Patch-"v39-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch" > for review. Please find the error I am facing: > D:\Project\Postgres>git am > D:\Project\Patch\v39-0001-postgres_fdw-add-postgres_fdw_verify_connection > -.patch >

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

2023-12-11 Thread Shubham Khanna
On Mon, Dec 11, 2023 at 2:08 PM Hayato Kuroda (Fujitsu) wrote: > > Dear Fujii-san, Tom, > > Thank you for giving a suggestion! PSA new version. > > > Regarding 0001 patch, on second thought, to me, it seems odd to expose > > a function that doesn't have anything to directly do with PostgreSQL > >

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

2023-04-05 Thread Hayato Kuroda (Fujitsu)
Dear hackers, > Dear Fujii-san, Tom, > > Thank you for giving a suggestion! PSA new version. I have reviewed and revised patches by myself. * Fix handing of poll() based on the Horiguchi-san's point * Fix WARNING message that shows user name which is used for connection * PQconnCheck(),

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

2023-04-04 Thread Hayato Kuroda (Fujitsu)
Dear Fujii-san, Tom, Thank you for giving a suggestion! PSA new version. > Regarding 0001 patch, on second thought, to me, it seems odd to expose > a function that doesn't have anything to directly do with PostgreSQL > as a libpq function. The function simply calls poll() on the socket > with

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

2023-04-03 Thread Tom Lane
Fujii Masao writes: > Regarding 0001 patch, on second thought, to me, it seems odd to expose > a function that doesn't have anything to directly do with PostgreSQL > as a libpq function. The function simply calls poll() on the socket > with POLLRDHUP if it is supported. While it is certainly

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

2023-04-03 Thread Fujii Masao
On 2023/03/13 16:05, Hayato Kuroda (Fujitsu) wrote: Thank you so much for your reviewing! Now we can wait comments from senior members and committers. Thanks for working on this patch! Regarding 0001 patch, on second thought, to me, it seems odd to expose a function that doesn't have

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

2023-03-13 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, > Hi, > > I updated the status of the patch to ready for committer. > > regards, Thank you so much for your reviewing! Now we can wait comments from senior members and committers. Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2023-03-13 Thread Katsuragi Yuta
On 2023-03-10 18:07, Katsuragi Yuta wrote: On 2023-03-08 13:40, Hayato Kuroda (Fujitsu) wrote: Dear Vignesh, Thank you for reviewing! PSA new version. Hi, Thank you for the comments, Vignesh. Thank you for updating the patch, Kuroda-san. This fix looks fine to me. And also, there seems

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

2023-03-10 Thread Katsuragi Yuta
On 2023-03-08 13:40, Hayato Kuroda (Fujitsu) wrote: Dear Vignesh, Thank you for reviewing! PSA new version. Hi, Thank you for the comments, Vignesh. Thank you for updating the patch, Kuroda-san. This fix looks fine to me. And also, there seems no other comments from this post [1]. So, I'm

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

2023-03-07 Thread Hayato Kuroda (Fujitsu)
Dear Vignesh, Thank you for reviewing! PSA new version. > > Few comments: > 1) There is no handling of forConnCheck in #else HAVE_POLL, if this is > intentional we could add some comments for the same: > static int > -pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time) >

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

2023-03-07 Thread vignesh C
On Tue, 7 Mar 2023 at 09:53, Hayato Kuroda (Fujitsu) wrote: > > Dear Katsuragi-san, > > Thank you for reviewing! PSA new version patches. > > > I think we can update the status to ready for committer after > > this fix, if there is no objection. > > That's a very good news for me! How about other

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

2023-03-06 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! PSA new version patches. > I think we can update the status to ready for committer after > this fix, if there is no objection. That's a very good news for me! How about other people? > >> 7. the document of postgres_fdw_verify_connection_states_all

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

2023-03-06 Thread Katsuragi Yuta
Hi Kuroda-san, Thank you for updating the patch! I think we can update the status to ready for committer after this fix, if there is no objection. 7. the document of postgres_fdw_verify_connection_states_all NULL + is returned if the local session does not have connection caches, or

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

2023-03-06 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! PSA new version. > >> 4. the code of pqSocketPoll > >> +#if defined(POLLRDHUP) > >> + if (forConnCheck) > >> + input_fd.events |= POLLRDHUP; > >> +#endif > >> > >> I think it is better to use PQconnCheckable() to remove the macro. > > > >

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

2023-03-06 Thread Katsuragi Yuta
Hi Kuroda-san, Thank you for updating the patch! 4. the code of pqSocketPoll +#if defined(POLLRDHUP) + if (forConnCheck) + input_fd.events |= POLLRDHUP; +#endif I think it is better to use PQconnCheckable() to remove the macro. IIUC the macro is needed. In FreeBSD, macOS

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

2023-03-03 Thread Hayato Kuroda (Fujitsu)
Hi Katsuragi-san, Thank you for reviewing! PSA new version. > >> I rethought the pqSocketPoll part. Current interpretation of > >> arguments seems a little bit confusing because a specific pattern > >> of arguments has a different meaning. What do you think about > >> introducing a new argument

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

2023-02-28 Thread Katsuragi Yuta
Hi Kuroda-san, Thank you for updating the patch! I rethought the pqSocketPoll part. Current interpretation of arguments seems a little bit confusing because a specific pattern of arguments has a different meaning. What do you think about introducing a new argument like `int forConnCheck`?

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

2023-02-22 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! New patch set can be available on [1]. > I rethought the pqSocketPoll part. Current interpretation of > arguments seems a little bit confusing because a specific pattern > of arguments has a different meaning. What do you think about > introducing a

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

2023-02-22 Thread Hayato Kuroda (Fujitsu)
Dear Peter, Thank you for reviewing! PSA new version. > 1. > PQconnCheck() function allows to check the status of the socket by polling > the socket. This function is currently available only on systems that > support the non-standard POLLRDHUP extension to the poll system call, > including

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

2023-02-21 Thread Katsuragi Yuta
Hi Kuroda-san, Thank you for updating the patch! On 2023-02-20 15:42, Hayato Kuroda (Fujitsu) wrote: Dear Katsuragi-san, Thank you for reviewing! PSA new version. I rethought the pqSocketPoll part. Current interpretation of arguments seems a little bit confusing because a specific pattern

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

2023-02-20 Thread Peter Smith
Here are some review comments for v32-0001. == Commit message 1. PQconnCheck() function allows to check the status of the socket by polling the socket. This function is currently available only on systems that support the non-standard POLLRDHUP extension to the poll system call, including

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

2023-02-19 Thread Hayato Kuroda (Fujitsu)
Dear Peter, Thank you for reviewing! Latest version can be seen in [1]. > 1. > PQcanConnCheck seemed like a strange API name. Maybe it can have the > same prefix as the other? > > e.g. > > - PQconnCheck() > - PGconnCheckSupported() > > or > > - PQconnCheck() > - PGconnCheckable() > I choose

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

2023-02-19 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! PSA new version. > 0001: > Extending pqSocketPoll seems to be a better way because we can > avoid having multiple similar functions. I also would like to hear > horiguchi-san's opinion whether this matches his expectation. > Improvements of

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

2023-02-19 Thread Peter Smith
Here is a code review only for patch v31-0001. == General Comment 1. PQcanConnCheck seemed like a strange API name. Maybe it can have the same prefix as the other? e.g. - PQconnCheck() - PGconnCheckSupported() or - PQconnCheck() - PGconnCheckable() == Commit Message 2.

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

2023-02-17 Thread Katsuragi Yuta
On 2023-02-09 23:39, Hayato Kuroda (Fujitsu) wrote: Dear Katsuragi-san, Thank you for reviewing! PSA new version patches. Thank you for updating the patch! These are my comments, please check. 0001: Extending pqSocketPoll seems to be a better way because we can avoid having multiple similar

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

2023-02-09 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! PSA new version patches. > 0001: > + while (result < 0 && errno == EINTR); > + > + if (result < 0) > + return -1; > > this `return -1` is not indented properly. This part is no longer needed. Please see another

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

2023-02-09 Thread Hayato Kuroda (Fujitsu)
Dear Horiguchi-san, Thank you for checking! The patch will be attached to another mail. > At Fri, 27 Jan 2023 06:57:01 +, "Hayato Kuroda (Fujitsu)" > wrote in > > I found cfbot failure, PSA fixed version. > > + Unlike , this function checks socket > + health. This check is

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

2023-02-08 Thread Kyotaro Horiguchi
At Fri, 27 Jan 2023 06:57:01 +, "Hayato Kuroda (Fujitsu)" wrote in > I found cfbot failure, PSA fixed version. + Unlike , this function checks socket + health. This check is performed by polling the socket. This function is + currently available only on systems that

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

2023-02-07 Thread Katsuragi Yuta
On 2023-01-27 15:57, Hayato Kuroda (Fujitsu) wrote: I found cfbot failure, PSA fixed version. Sorry for noise. Best Regards, Hayato Kuroda FUJITSU LIMITED Hi Kuroda-san, Thank you for updating the patch! Sorry for the late reply. 0001: + while (result < 0 && errno == EINTR); + +

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

2023-01-26 Thread Hayato Kuroda (Fujitsu)
I found cfbot failure, PSA fixed version. Sorry for noise. Best Regards, Hayato Kuroda FUJITSU LIMITED v29-0003-add-test.patch Description: v29-0003-add-test.patch v29-0004-add-kqueue-support-for-PQconnCheck-and-PQcanConn.patch Description:

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

2023-01-26 Thread Hayato Kuroda (Fujitsu)
Dear hackers, I have updated my patch for error handling and kqueue() support. Actually I do not have BSD-like machine, but I developed by using github CICD. I think at first we should focus on 0001-0003, and then work for 0004. Followings are change notes and my analysis. 0001 * Fix missed

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

2023-01-25 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reading the patch! PSA new version. > Thank you for updating the patch! > > +/* Check whether the postgres server is still alive or not */ > +extern int PQConnCheck(PGconn *conn); > +extern int PQCanConnCheck(void); > > Aren't these PQconnCheck and

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

2023-01-25 Thread Katsuragi Yuta
On 2023-01-23 14:40, Hayato Kuroda (Fujitsu) wrote: Dear Ted, Thanks for reviewing! PSA new version. For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch , `pqConnCheck_internal` only has one caller which is quite short. Can pqConnCheck_internal and PQConnCheck be merged into one

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

2023-01-22 Thread Hayato Kuroda (Fujitsu)
Dear Ted, Thanks for reviewing! PSA new version. > For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch , > `pqConnCheck_internal` only has one caller which is quite short. > Can pqConnCheck_internal and PQConnCheck be merged into one func ? I divided the function for feature

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

2023-01-22 Thread Hayato Kuroda (Fujitsu)
> Thank you for reviewing! PSA new patch set. Sorry, I missed the updated file in the patch. New version will be posted soon. Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2023-01-21 Thread Ted Yu
On Sat, Jan 21, 2023 at 4:03 AM Hayato Kuroda (Fujitsu) < kuroda.hay...@fujitsu.com> wrote: > Dear Katsuragi-san, > > Thank you for reviewing! PSA new patch set. > > > ## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch > > + > > + > > PQCanConncheckPQCan > > Conncheck > > + >

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

2023-01-21 Thread Hayato Kuroda (Fujitsu)
Dear Katsuragi-san, Thank you for reviewing! PSA new patch set. > ## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch > + > + > PQCanConncheckPQCan > Conncheck > + > + > + Returns the status of the socket. > > Is this description right? I think this description is

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

2023-01-20 Thread Katsuragi Yuta
On 2023-01-11 19:04, Hayato Kuroda (Fujitsu) wrote: Dear hackers, I was not sure, but the cfbot could not be accepted the previous version. I made the patch again from HEAD(5f6401) without any changes, so I did not count up the version number. Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2023-01-11 Thread Hayato Kuroda (Fujitsu)
Dear hackers, I was not sure, but the cfbot could not be accepted the previous version. I made the patch again from HEAD(5f6401) without any changes, so I did not count up the version number. Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2023-01-10 Thread Hayato Kuroda (Fujitsu)
Dear Ted, Thank you for reviewing! PSA new version. > + /* quick exit if connection cache has been not initialized yet. */ > > been not initialized -> not been initialized Fixed. > + > (errcode(ERRCODE_CONNECTION_FAILURE), > +

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

2023-01-10 Thread Ted Yu
On Tue, Jan 10, 2023 at 8:26 AM Hayato Kuroda (Fujitsu) < kuroda.hay...@fujitsu.com> wrote: > Dear tom, > > > I think that it's a really bad idea to require postgres_fdw.sql > > to have two expected-files: that will be a maintenance nightmare. > > Please put whatever it is that needs a variant

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

2023-01-10 Thread Hayato Kuroda (Fujitsu)
Dear tom, > I think that it's a really bad idea to require postgres_fdw.sql > to have two expected-files: that will be a maintenance nightmare. > Please put whatever it is that needs a variant expected-file > into its own, hopefully very small and seldom-changed, test script. > Or rethink whether

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

2023-01-09 Thread Tom Lane
"Hayato Kuroda (Fujitsu)" writes: > Thanks for reporting. PSA rebased version. > These can be applied work well on my HEAD(bd8d45). I think that it's a really bad idea to require postgres_fdw.sql to have two expected-files: that will be a maintenance nightmare. Please put whatever it is that

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

2023-01-03 Thread vignesh C
On Tue, 20 Dec 2022 at 07:22, Hayato Kuroda (Fujitsu) wrote: > > Dear hackers, > > PSA rebased patches. The patch does not apply on top of HEAD as in [1], please post a rebased patch: === Applying patches on top of PostgreSQL commit ID 92957ed98c5c565362ce665266132a7f08f6b0c0 === === applying

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

2022-12-06 Thread Andres Freund
Hi, On 2022-11-25 11:41:45 +, Hayato Kuroda (Fujitsu) wrote: > Sorry for my late reply. I understood that we got agreement the basic design > of first version. Thanks! > I attached new version patches. This is failing on cfbot / CI, as have prior versions.

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

2022-11-11 Thread Önder Kalacı
Hi Hayota Kuroda, > I (and my company) worried about overnight batch processing that > contains some accesses to foreign servers. If the transaction is opened > overnight and > one of foreign servers is crashed during it, the transaction must be > rollbacked. > But there is a possibility that

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

2022-10-19 Thread Önder Kalacı
Hi, > As far as I can think of, it should probably be a single background task > > checking whether the server is down. If so, sending an invalidation > message > > to all the backends such that related backends could act on the > > invalidation and throw an error. This is to cover the use-case

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

2022-10-17 Thread osumi.takami...@fujitsu.com
On Monday, October 17, 2022 9:25 PM Kuroda, Hayato/黒田 隼人 wrote: > > I mainly followed the steps there and > > replaced the command "SELECT" for the remote table at 6-9 with "INSERT" > > command. > > Then, after waiting for few seconds, the "COMMIT" succeeded like below > > output, even after the

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

2022-10-17 Thread kuroda.hay...@fujitsu.com
Dear Osumi-san, > I mainly followed the steps there and > replaced the command "SELECT" for the remote table at 6-9 with "INSERT" > command. > Then, after waiting for few seconds, the "COMMIT" succeeded like below output, > even after the server stop of the worker side. > Additionally, the last

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

2022-10-17 Thread kuroda.hay...@fujitsu.com
Dear Horiguchi-san, > Might be on slight different direction, but it looks to me a bit too > much to use WaitEventSet to check only if a socket is live or not. > > A quick search in the tree told me that we could use pqSocketCheck() > instead, and I think it would be the something that "could

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

2022-10-17 Thread Kyotaro Horiguchi
At Mon, 17 Oct 2022 07:27:21 +, "kuroda.hay...@fujitsu.com" wrote in > > In other words, a variation of pgfdw_connection_check_internal() > > could potentially go into interfaces/libpq/libpq-fe.h > > (backend/libpq/pqcomm.c or src/interfaces/libpq/fe-connect.c). > > Hmm, IIUC libpq related

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

2022-10-17 Thread kuroda.hay...@fujitsu.com
Dear Önder, Thanks for giving suggestions! > Still, the reestablish mechanism can be further simplified with > WL_SOCKET_CLOSED event such as the following (where we should probably > rename pgfdw_connection_check_internal): Sounds reasonable. I think it may be included in this patch. I will

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

2022-10-16 Thread osumi.takami...@fujitsu.com
On Wednesday, October 5, 2022 6:27 PM kuroda.hay...@fujitsu.com wrote: > Thanks for giving many comments! Based on off and on discussions, I modified > my patch. Here are my other quick review comments on v16. (1) v16-0001 : definition of a new structure CheckingRemoteServersCallbackItem can

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

2022-10-16 Thread osumi.takami...@fujitsu.com
Hi, On Wednesday, October 5, 2022 6:27 PM kuroda.hay...@fujitsu.com wrote: > Thanks for giving many comments! Based on off and on discussions, I modified > my patch. Thank you for your patch set ! While reviewing and testing the new v16, I've met a possible issue by slightly adjusting the

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

2022-10-12 Thread Önder Kalacı
Hi, Sounds reasonable. Do you mean that we can add additional GUC like > "postgres_fdw.initial_check", > wait WL_SOCKET_CLOSED if the conneciton is found in the hash table, and do > reconnection if it might be closed, right? > > Alright, it took me sometime to realize that postgres_fdw already

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

2022-10-03 Thread kuroda.hay...@fujitsu.com
Dear Önder, > As far as I can see this patch is mostly useful for detecting the failures > on the initial remote command. This is especially common when the remote > server does a failover/switchover and postgres_fdw uses a cached connection > to access to the remote server. Sounds reasonable.

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

2022-10-03 Thread Önder Kalacı
Hi Hayato Kuroda, > If the checking function is called not periodically but GetConnection(), > it means that the health of foreign servers will be check only when remote > connections are used. > So following workload described in [1] cannot handle the issue. > > BEGIN --- remote operations---

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

2022-10-03 Thread kuroda.hay...@fujitsu.com
Dear Önder, Thank you for being interest to my patch! Your suggestions will be included to newer version. > In other words, what is the trade-off for calling > pgfdw_connection_check_internal() inside GetConnection() when we are about > to use a "cached" connection? I think that might simplify

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

2022-10-03 Thread Önder Kalacı
Hi Hayato Kuroda, Thanks for the patch. I think there are some non-fdw extensions out there which could benefit from this logic. That's why I want to first learn some more about high-level design/goals of the patch a little more. +/* > + * Call callbacks for checking remote servers. > + */ >

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

2022-10-03 Thread kuroda.hay...@fujitsu.com
Dear Andres, > This seems to reliably fail on windows. See Thanks for reporting. Actually this feature cannot be used on Windows machine. To check the status of each socket that connects to the foreign server, the socket event WL_SOCKET_CLOSED is used. The event is only enabled on some OSes, and

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

2022-10-02 Thread Andres Freund
Hi, On 2022-09-21 11:56:56 +, kuroda.hay...@fujitsu.com wrote: > PSA rebased patches. I reviewed my myself and they contain changes. > E.g., move GUC-related code to option.c. This seems to reliably fail on windows. See https://cirrus-ci.com/task/6454408568373248

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

2022-09-21 Thread kuroda.hay...@fujitsu.com
Dear Fujii-san, Thanks for checking! > These failed to be applied to the master branch cleanly. Could you update > them? PSA rebased patches. I reviewed my myself and they contain changes. E.g., move GUC-related code to option.c. > + this option relies on kernel events exposed by Linux,

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

2022-09-17 Thread Fujii Masao
On 2022/03/04 15:17, kuroda.hay...@fujitsu.com wrote: Hi Hackers, It's not happy, but I'm not sure about a good solution. I made a timer reschedule if connection lost had detected. But if queries in the transaction are quite short, catching SIGINT may be fail. Attached uses another way:

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

2022-03-03 Thread kuroda.hay...@fujitsu.com
Hi Hackers, > It's not happy, but I'm not sure about a good solution. I made a timer > reschedule > if connection lost had detected. But if queries in the transaction are quite > short, > catching SIGINT may be fail. Attached uses another way: sets pending flags again if DoingCommandRead is

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

2022-02-23 Thread kuroda.hay...@fujitsu.com
Dear Fujii-san, Thank you for your quick reviewing! I attached new version. I found previous patches have wrong name. Sorry. > The connection check timer is re-scheduled repeatedly even while the backend > is > in idle state or is running a local transaction that doesn't access to any >

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

2022-02-21 Thread Fujii Masao
On 2022/02/22 15:41, kuroda.hay...@fujitsu.com wrote: Cfbot is still angry because of missing PGDLLIMPORT, so attached. Thanks for updating the patches! The connection check timer is re-scheduled repeatedly even while the backend is in idle state or is running a local transaction that

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

2022-02-21 Thread kuroda.hay...@fujitsu.com
Cfbot is still angry because of missing PGDLLIMPORT, so attached. Best Regards, Hayato Kuroda FUJITSU LIMITED v12_0001_add_checking_infrastracture.patch Description: v12_0001_add_checking_infrastracture.patch v12_0002_add_health_check.patch Description: v12_0002_add_health_check.patch

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

2022-02-21 Thread kuroda.hay...@fujitsu.com
Dear Fujii-san, > cfbot is reporting that the 0002 patch fails to be applied cleanly. Could you > update > the patch? > http://cfbot.cputube.org/patch_37_3388.log Thanks for reporting and sorry for inconvenience. I repo was not latest version. Attached can be applied to 52e4f0c Best Regards,

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

2022-02-21 Thread Fujii Masao
On 2022/02/22 11:53, kuroda.hay...@fujitsu.com wrote: How do you think? Thanks for updating the patches! I will read them. cfbot is reporting that the 0002 patch fails to be applied cleanly. Could you update the patch? http://cfbot.cputube.org/patch_37_3388.log Regards, -- Fujii Masao

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

2022-02-21 Thread kuroda.hay...@fujitsu.com
Dear Horiguchi-san, Fujii-san, > > I understood here as removing following mechanism from core: > > > > * disable timeout at end of tx. > > * skip if held off or read commands > > I think we're on the same page. Anyway query cancel interrupt is > ignored while rading input. > > > > - If an

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

2022-02-21 Thread kuroda.hay...@fujitsu.com
Dear Fujii-san, > Isn't it a very special case where many FDWs use their own user timeouts? > Could > you tell me the assumption that you're thinking, especially how many FDWs are > working? I came up with the case like star schema, which postgres database connects data store. If each dbms are

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

2022-02-18 Thread Fujii Masao
On 2022/02/17 19:35, kuroda.hay...@fujitsu.com wrote: Dear Horiguchi-san, I think we just don't need to add the special timeout kind to the core. postgres_fdw can use USER_TIMEOUT and it would be suffiction to keep running health checking regardless of transaction state then fire query

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

2022-02-17 Thread kuroda.hay...@fujitsu.com
Dear Horiguchi-san, > I think we just don't need to add the special timeout kind to the > core. postgres_fdw can use USER_TIMEOUT and it would be suffiction to > keep running health checking regardless of transaction state then fire > query cancel if disconnection happens. As I said in the

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

2022-02-17 Thread Kyotaro Horiguchi
At Thu, 17 Feb 2022 04:32:26 +, "kuroda.hay...@fujitsu.com" wrote in > > I understood here as removing following mechanism from core: > > > > * disable timeout at end of tx. > > While reading again and this part might be wrong. > Sorry for inconvenience. > But anyway some codes should be

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

2022-02-17 Thread Kyotaro Horiguchi
Hi, Kuroda-san. At Thu, 17 Feb 2022 04:11:09 +, "kuroda.hay...@fujitsu.com" wrote in > Dear Horiguchi-san, > > Thank you for giving your suggestions. I want to confirm your saying. > > > FWIW, I'm not sure this feature necessarily requires core support > > dedicated to FDWs. The core

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

2022-02-16 Thread kuroda.hay...@fujitsu.com
> I understood here as removing following mechanism from core: > > * disable timeout at end of tx. While reading again and this part might be wrong. Sorry for inconvenience. But anyway some codes should be (re)moved from core, right? Best Regards, Hayato Kuroda FUJITSU LIMITED

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

2022-02-16 Thread kuroda.hay...@fujitsu.com
Dear Horiguchi-san, Thank you for giving your suggestions. I want to confirm your saying. > 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

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

2022-02-15 Thread Kyotaro Horiguchi
At Tue, 1 Feb 2022 23:51:54 +0900, Fujii Masao 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

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

2022-02-15 Thread kuroda.hay...@fujitsu.com
Dear Hackers, I found patches we depend have been committed, so rebased. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=50e570a59e7f86bb41f029a66b781fc79b8d50f1 In this version there is a little bit change in part of postgres_fdw. A system checking by

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

2022-02-02 Thread kuroda.hay...@fujitsu.com
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

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

2022-02-01 Thread Fujii Masao
On 2022/02/01 13:37, kuroda.hay...@fujitsu.com 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

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

2022-01-31 Thread kuroda.hay...@fujitsu.com
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

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

2022-01-22 Thread Fujii Masao
On 2022/01/21 15:08, kuroda.hay...@fujitsu.com 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,

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

2022-01-20 Thread kuroda.hay...@fujitsu.com
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

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

2022-01-20 Thread kuroda.hay...@fujitsu.com
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

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

2022-01-19 Thread kuroda.hay...@fujitsu.com
Dear Zhihong, Thank you for reviewing! And I have to apologize for the late. I'll post new patchset later. > + UnregisterAllCheckingRemoteServersCallback(); > > UnregisterAllCheckingRemoteServersCallback > -> UnregisterAllCheckingRemoteServersCallbacks Will fix. > +

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

2022-01-18 Thread Fujii Masao
On 2021/12/15 15:40, kuroda.hay...@fujitsu.com 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

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

2022-01-05 Thread Zhihong Yu
On Tue, Jan 4, 2022 at 10:21 PM Shinya Kato wrote: > Thank you for the new patch! > > On 2021-12-15 15:40, kuroda.hay...@fujitsu.com wrote: > > Dear Kato-san, > > > > Thank you for giving comments! And sorry for late reply. > > I rebased my patches. > > > >> Even for local-only transaction, I

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

2022-01-04 Thread Shinya Kato
Thank you for the new patch! On 2021-12-15 15:40, kuroda.hay...@fujitsu.com wrote: Dear Kato-san, Thank you for giving comments! And sorry for late reply. I rebased my patches. Even for local-only transaction, I thought it useless to execute CallCheckingRemoteServersCallbacks() and

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

2021-12-14 Thread kuroda.hay...@fujitsu.com
Dear Kato-san, Thank you for giving comments! And sorry for late reply. I rebased my patches. > Even for local-only transaction, I thought it useless to execute > CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I > make it so that it determines outside whether it contains

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

2021-12-06 Thread Shinya Kato
On 2021-11-29 21:36, Zhihong Yu wrote: On Mon, Nov 29, 2021 at 12:51 AM kuroda.hay...@fujitsu.com wrote: Dear Zhihong, Thank you for giving comments! I'll post new patches later. +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() (CheckingRemoteServersHoldoffCount++) The macro contains

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

2021-11-29 Thread Zhihong Yu
On Mon, Nov 29, 2021 at 12:51 AM kuroda.hay...@fujitsu.com < kuroda.hay...@fujitsu.com> wrote: > Dear Zhihong, > > Thank you for giving comments! I'll post new patches later. > > > +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() > (CheckingRemoteServersHoldoffCount++) > > > > The macro

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

2021-11-29 Thread kuroda.hay...@fujitsu.com
Dear Hackers, I attached new version that is based from Zhihong's comments. And I newly attached a doc patch. I think the infrastructure part is no longer WIP. Could you review them? Best Regards, Hayato Kuroda FUJITSU LIMITED <> v04_0001_add_checking_infrastracture.patch Description:

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

2021-11-29 Thread kuroda.hay...@fujitsu.com
Dear Zhihong, Thank you for giving comments! I'll post new patches later. > +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS() > (CheckingRemoteServersHoldoffCount++) > > The macro contains only one operation. Can the macro be removed (with > `CheckingRemoteServersHoldoffCount++` inlined) ?

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

2021-11-28 Thread Zhihong Yu
On Tue, Nov 23, 2021 at 8:57 PM kuroda.hay...@fujitsu.com < kuroda.hay...@fujitsu.com> wrote: > Dear Kato-san, > > Thank you for reviewing! > > > Thank you for sending the patches! > > I confirmed that they can be compiled and tested successfully on CentOS > > 8. > > Thanks! > > > + { > > +

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

2021-11-23 Thread kuroda.hay...@fujitsu.com
Dear Kato-san, I found the missing space, and I added a test. I'm very happy if you review this. Best Regards, Hayato Kuroda FUJITSU LIMITED v03_0001_add_checking_infrastracture.patch Description: v03_0001_add_checking_infrastracture.patch <>

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

2021-11-23 Thread kuroda.hay...@fujitsu.com
Dear Kato-san, Thank you for reviewing! > Thank you for sending the patches! > I confirmed that they can be compiled and tested successfully on CentOS > 8. Thanks! > + { > + {"remote_servers_connection_check_interval", PGC_USERSET, > CONN_AUTH_SETTINGS, > +

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

2021-11-19 Thread Shinya Kato
On 2021-11-18 21:43, kuroda.hay...@fujitsu.com wrote: Dear Kato-san, Thank you for your interest! > I also want you to review the postgres_fdw part, > but I think it should not be attached because cfbot cannot understand > such a dependency > and will throw build error. Do you know how to

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

2021-11-18 Thread kuroda.hay...@fujitsu.com
Dear Kato-san, Thank you for your interest! > > I also want you to review the postgres_fdw part, > > but I think it should not be attached because cfbot cannot understand > > such a dependency > > and will throw build error. Do you know how to deal with them in this > > case? > > I don't know

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

2021-11-15 Thread Shinya Kato
Hi, Thank you for the patch! On 2021-10-30 12:50, kuroda.hay...@fujitsu.com wrote: ## Background Currently there is no way to check the status of an foreign server in PostgreSQL. If an foreign server's postmaster goes down, or if the network between servers is lost, the backend process will

  1   2   >