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