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

On Tue, Nov 23, 2021 at 8:57 PM kuroda.hayato@fujitsu.com <
kuroda.hayato@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!
>
> > +     {
> > +             {"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,
> > +     },
> >
> > If you don't use check_hook, assign_hook and show_hook, you should
> > explicitly write "NULL, NULL, NULL", as show below.
>
> Yeah I forgot the line. Fixed.
>
> > +                     ereport(ERROR,
> > +
> >       errcode(ERRCODE_CONNECTION_FAILURE),
> > +                                     errmsg("Postgres foreign server %s
> > might be down.",
> > +
> >       server->servername));
> >
> > According to [1], error messages should start with a lowercase letter
> > and not use a period.
> > Also, along with the rest of the code, it is a good idea to enclose the
> > server name in double quotes.
>
> I confirmed the postgres error-reporting policy and fixed to follow that.
> How do you think?
>
> Attached are the latest version.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
> Hi,

+#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()
 (CheckingRemoteServersHoldoffCount++)

The macro contains only one operation. Can the macro be removed (with
`CheckingRemoteServersHoldoffCount++` inlined) ?

+   if (CheckingRemoteServersTimeoutPending &&
CheckingRemoteServersHoldoffCount != 0)
+   {
+       /*
+        * Skip checking foreign servers while reading messages.
+        */
+       InterruptPending = true;
+   }
+   else if (CheckingRemoteServersTimeoutPending)

Would the code be more readable if the above two if blocks be moved inside
one enclosing if block (factoring the common condition)?

+   if (CheckingRemoteServersTimeoutPending)

Cheers