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

On 2023/12/12 11:43, Hayato Kuroda (Fujitsu) wrote:
> Dear Shubham,
>
>>
>> I am failing to apply the latest
>> Patch-"v39-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch"
>> for review. Please find the error I am facing:
>> D:\Project\Postgres>git am
>> D:\Project\Patch\v39-0001-postgres_fdw-add-postgres_fdw_verify_connection
>> -.patch
>> error: patch failed: contrib/postgres_fdw/connection.c:117
>> error: contrib/postgres_fdw/connection.c: patch does not apply
>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>> Applying: postgres_fdw: add postgres_fdw_verify_connection variants
>> Patch failed at 0001 postgres_fdw: add postgres_fdw_verify_connection variants
>> When you have resolved this problem, run "git am --continue".
>> If you prefer to skip this patch, run "git am --skip" instead.
>> To restore the original branch and stop patching, run "git am --abort".
>
> Oh, good catch. Here is a new patch set. There are no new changes from v39.

Thanks for the patches!

I've just started reviewing them.

Here are the review comments for 0001 patch:

Do we really need postgres_fdw_verify_connection_all()? The proposed feature
aims to check if all postgres_fdw connections used within the transaction
are still open. If any of those connections are closed, the transaction
can't be committed successfully, so users can roll back immediately upon
detecting a closed connection.

However, postgres_fdw_verify_connection_all() checks all connections made in
the session, not just those used in the current transaction. This means
users can't determine if they should roll back the transaction based on
its return value. Therefore, I'm concerned that
postgres_fdw_verify_connection_all() might not be very useful. Thoughts?

Considering the purpose of this feature, wouldn't it be better to extend
postgres_fdw_get_connections() to include a "used_in_xact" column
(indicating whether the connection has been used in the current transaction)
and a "closed" column (indicating whether the connection has been closed)?
This approach might be more effective than introducing a new function
like the postgres_fdw_verify_connection family?

If it's too much to check if the connection is closed by default whenever
calling postgres_fdw_get_connections(), we could modify it to accept
an argument indicating whether to perform this check. Thoughts?

Here are the review comments for 0003 patch:

The source comment in postgres_fdw_get_connections() should mention
the return value user_name.

We also need to handle the case where the user mapping used by
the connection cache has been dropped. Otherwise, this could
lead to an error.

-----------------------------
=# BEGIN;
=*# SELECT * FROM postgres_fdw_get_connections();
server_name | user_name | valid
-------------+-----------+-------
loopback | public | t
(1 row)

=*# DROP USER MAPPING FOR public SERVER loopback ;
=*# SELECT * FROM postgres_fdw_get_connections();
ERROR: cache lookup failed for user mapping 16409
-----------------------------

-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT server_name, valid FROM postgres_fdw_get_connections() ORDER BY 1;

Shouldn't this test also check if the returned user_name is valid?

+ server_name | user_name | validvalid
+-------------+------------------------
+ loopback1 | postgres | t
+ loopback2 | | f

The column name "validvalid" should be "valid".

How can we cause the record with server_name != NULL but user_name = NULL?

Regards,

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