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

On 2024/08/02 14:56, Hayato Kuroda (Fujitsu) wrote:
> I moved the function to connection.c, which uses the SearchSysCache1().
>
> I've tried both ways, and they worked well. One difference is that when we use
> the extended ConnCacheEntry approach and the entry has been invalidated, we cannot
> distinguish the reason. For example, in the below case, the entry is invalidated,
> so the user_name of the output record will be NULL, whereas the user mapping is
> actually still valid. We may be able to add the reason for invalidation, but
> I'm not very motivated to modify the part.

Understood. Also thanks for updating the patch!

<term><function>postgres_fdw_get_connections(
IN check_conn boolean DEFAULT false, OUT server_name text,
OUT valid boolean, OUT used_in_xact boolean, OUT closed boolean)
returns setof record</function></term>

In the documentation, this part should be updated to include the user_name output column.

+ <entry><structfield>user_name</structfield></entry>
+ <entry><type>text</type></entry>
+ <entry>
+ The local user name of this connection. If the user mapping is
+ dropped but the connection remains open (i.e., marked as
+ invalid), this will be <literal>NULL</literal>.

How about changing the first description to "Name of the local user mapped to the foreign server of this connection, or "public" if a public mapping is used." for more precision?

- server_name | valid | used_in_xact | closed
--------------+-------+--------------+--------
- loopback1 | t | t |
- loopback2 | f | t |
+ server_name | user_name | valid | used_in_xact | closed
+-------------+-----------+-------+--------------+--------
+ loopback1 | postgres | t | t |
+ loopback2 | postgres | t | t |

How about displaying the record with loopback2 and valid=false like the previous usage example?

+UserMapping *
+GetUserMappingByOid(Oid umid, bool missing_ok)

postgres_fdw doesn't need a generic function to return UserMapping. How about simplifying the function by removing unnecessary code, e.g., as follows?

----------
tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(umid));
if (!HeapTupleIsValid(tp))
nulls[i++] = true;
else
{
Oid userid = ((Form_pg_user_mapping) GETSTRUCT(tp))->userid;
values[i++] = CStringGetTextDatum(MappingUserName(userid));
ReleaseSysCache(tp);
}
----------

-ForeignTable *
-GetForeignTable(Oid relid);
-</programlisting>
-
- This function returns a <structname>ForeignTable</structname> object for
- the foreign table with the given OID. A
- <structname>ForeignTable</structname> object contains properties of the
- foreign table (see <filename>foreign/foreign.h</filename> for details).
- </para>
-
- <para>
-<programlisting>

Why did you remove these code? Just mistake?

Regards,

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