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

Dear Fujii-san,

> 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.

But this might cause a test failure by cfbot.... See below comment.

> > - 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?

Initially I felt that user might not be able to distinguish whether 1) the
connection has not been established yet or 2) the checking cannot be done on this
platform. But after considering more, such servers are not listed in the function.
So modified like that.

> > - 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.

I also prefer the style, so changed accordingly.

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

This was because the user_name is quite depends on the environment.
I think it looks not so good, but one approach is to print `user_name = CURRENT_USER`
to test the feature. For loopback3, the user_name is set to NULL so that the column
will be NULL as well. How do you think?  Do you have better idea?

> +			 * 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?

I confirmed the meaning of `invalidated` attribute. IIUC:

- It is set to true when the server or user mapping is altered, but
- This connection has already been opened within the transaction.

If the entry is invalided, the server_name of postgres_fdw_get_connections is set
set to NULL (because the entry may be modified). Also, the connection is discarded
when the transaction ends.
Based on the unserstanding, yes, the connection should be also checked. One concern
is that user may not recognize which connection is lost (because the column may be blank).

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

Good finding, removed.

Attached patches contain above fixes and comment improvements per request from GPT-4o.

Best regards,
Hayato Kuroda
FUJITSU LIMITED