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

On 2024/07/18 19:49, Hayato Kuroda (Fujitsu) wrote:
>> Shouldn't this test also check if the returned user_name is valid?
>
> You meant to say that we should print the user_name, right? Done.

Yes, I think it's better to test if the value in the user_name column is as expected.

> - I found an inconsistency of name between source and doc,
> so I unified to postgres_fdw_can_verify_connection().

I'm unsure about the necessity of introducing a standalone function to check
POLLRDHUP availability. Instead of providing postgres_fdw_can_verify_connection(),
could we modify postgres_fdw_get_connections() to return NULL in the "closed"
column on platforms where POLLRDHUP isn't supported?

> - Also, test patch (0002) was combined into this.
> 0002:
> - user_name was added after the `valid` to preserve ordering of the old version.

Do we really need to keep this ordering? Wouldn't it be more intuitive to
have the user_name column next to server_name? In pg_stat_statements,
for example, the ordering isn't always preserved, as seen with
WAL-related columns being added in the middle.

> Thanks for reviewing the patch! PSA new version.

Thanks for updating the patches!

The regression test for postgres_fdw failed with the following diff.

----------------------
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
server_name | valid | user_name | used_in_xact | closed
-------------+-------+-----------+--------------+--------
- loopback | f | hayato | t |
+ loopback | f | runner | t |
| f | | t |
(2 rows)
----------------------

+ * If requested and the connection is not invalidated, check the
+ * status of the remote connection from the backend process and
+ * return the result. Otherwise returns NULL.
+ */
+ if (require_verify && !entry->invalidated && entry->conn)

Should we also consider checking invalidated connections? Even though
a connection is marked as invalidated, it can still be used until
the current transaction ends. Therefore, if an invalidated connection
is used in this transaction (i.e., used_in_xact = true) and has
already been closed (closed = true), users might want to consider
rolling back the transaction promptly. Thought?

+ -- Set client_min_messages to ERROR temporary because the following
+ -- function only throws a WARNING on the supported platform.

Is this still true? From my reading of the code, it doesn't appear
that the function throws a WARNING.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION