Re: [Proposal] Add foreign-server health checks infrastructure - Mailing list pgsql-hackers

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
unnecessarycode, 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