Horton

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

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread View on postgr.es
From: "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>
To: 'Zhihong Yu' <zyu@yugabyte.com>
Cc: Shinya Kato <Shinya11.Kato@oss.nttdata.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2021-11-29T08:51:40Z
Lists: pgsql-hackers 
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) ?

Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and HOLD_CANCEL_INTERRUPTS():

```
#define HOLD_INTERRUPTS()  (InterruptHoldoffCount++)

#define RESUME_INTERRUPTS() \
do { \
        Assert(InterruptHoldoffCount > 0); \
        InterruptHoldoffCount--; \
} while(0)

#define HOLD_CANCEL_INTERRUPTS()  (QueryCancelHoldoffCount++)

#define RESUME_CANCEL_INTERRUPTS() \
do { \
        Assert(QueryCancelHoldoffCount > 0); \
        QueryCancelHoldoffCount--; \
} while(0)

#define START_CRIT_SECTION()  (CritSectionCount++)

#define END_CRIT_SECTION() \
do { \
        Assert(CritSectionCount > 0); \
        CritSectionCount--; \
} while(0)
```

So I want to keep the current style. Could you tell me if you have any other reasons?

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

+1. Will fix.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

Commits

  1. postgres_fdw: Extend postgres_fdw_get_connections to return user name.

  2. postgres_fdw: Fix bug in connection status check.

  3. postgres_fdw: Add connection status check to postgres_fdw_get_connections().

  4. postgres_fdw: Add "used_in_xact" column to postgres_fdw_get_connections().

  5. doc: Enhance documentation for postgres_fdw_get_connections() output columns.

  6. Avoid reference to nonexistent array element in ExecInitAgg().

  7. Add WL_SOCKET_CLOSED for socket shutdown events.

  8. postgres_fdw: reestablish new connection if cached one is detected as broken.