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!
>
> > + {
> > + {"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
