RE: [Proposal] Add foreign-server health checks infrastructure - Mailing list pgsql-hackers

From Hayato Kuroda (Fujitsu)
Subject RE: [Proposal] Add foreign-server health checks infrastructure
Date
Msg-id TYAPR01MB586622DBD4A0C3BA57136899F5B49@TYAPR01MB5866.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: [Proposal] Add foreign-server health checks infrastructure  (vignesh C <vignesh21@gmail.com>)
Responses Re: [Proposal] Add foreign-server health checks infrastructure
List pgsql-hackers
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)
> +pqSocketPoll(int sock, int forRead,
> +                        int forWrite, int forConnCheck, time_t end_time)
>  {
>         /* We use poll(2) if available, otherwise select(2) */
>  #ifdef HAVE_POLL
> @@ -1092,7 +1133,11 @@ pqSocketPoll(int sock, int forRead, int
> forWrite, time_t end_time)
>         int                     timeout_ms;
> 
>         if (!forRead && !forWrite)
> -               return 0;

Comments were added. This missing is intentional, because with the patch present
we do not implement checking feature for kqueue(). If you want to check the
detailed implementation of that, please see 0004 patch attached on [1].

> 2) Can this condition be added to the parent if condition:
>         if (!forRead && !forWrite)
> -               return 0;
> +       {
> +               /* Connection check can be available on some limted platforms
> */
> +               if (!(forConnCheck && PQconnCheckable()))
> +                       return 0;
> +       }

Changed, and added comments atop the if-statement.

> 3) Can we add a comment for PQconnCheckable:
> +/* Check whether the postgres server is still alive or not */
> +extern int PQconnCheck(PGconn *conn);
> +extern int PQconnCheckable(void);
> +

Added. And I found the tab should be used to divide data type and name, so fixed.

> 4) "Note that if 0 is returned and forConnCheck is requested" doesn't
> sound right, it can be changed to "Note that if 0 is returned when
> forConnCheck is requested"
> + * If neither forRead, forWrite nor forConnCheck are set, immediately return a
> + * timeout condition (without waiting). Return >0 if condition is met, 0 if a
> + * timeout occurred, -1 if an error or interrupt occurred. Note that if 0 is
> + * returned and forConnCheck is requested, it means that the socket has not
> + * matched POLLRDHUP event and the connection is not closed by the socket
> peer.

Fixed.

[1]:
https://www.postgresql.org/message-id/TYAPR01MB58669EAAC02493BFF9F39B06F5D99%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED


Attachment

pgsql-hackers by date:

Previous
From: John Naylor
Date:
Subject: Re: [PoC] Improve dead tuple storage for lazy vacuum
Next
From: Masahiko Sawada
Date:
Subject: Re: Testing autovacuum wraparound (including failsafe)