Message-ID: <b0d192d1-51ec-40f5-acde-0230ef2cc885@oss.nttdata.com>
Date: Thu, 8 Aug 2024 01:20:57 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: 'Katsuragi Yuta' <katsuragiy@oss.nttdata.com>,
 'vignesh C' <vignesh21@gmail.com>, 'Ted Yu' <yuzhihong@gmail.com>,
 'Tom Lane' <tgl@sss.pgh.pa.us>,
 "'pgsql-hackers@lists.postgresql.org'" <pgsql-hackers@lists.postgresql.org>,
 =?UTF-8?B?J8OWbmRlciBLYWxhY8SxJw==?= <onderkalaci@gmail.com>,
 "'Shinya11.Kato@oss.nttdata.com'" <Shinya11.Kato@oss.nttdata.com>,
 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>,
 'Shubham Khanna' <khannashubham1197@gmail.com>
References: 
 <TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <be9382f7-5072-4760-8b3f-31d6dffa8d62@oss.nttdata.com>
 <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com>
 <613adbf9-c86e-41dc-ab97-27dc4e7281c7@oss.nttdata.com>
 <TYCPR01MB56933FCF37A676B868362575F5AA2@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <d04aae8d-05f5-42f4-a263-b962334d9f75@oss.nttdata.com>
 <TYCPR01MB569331643314B5129312C332F5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <TYCPR01MB56934DE73B0AF7A37D8CF7B2F5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <768032ee-fb57-494b-b674-1ccb65b6f969@oss.nttdata.com>
 <TYCPR01MB5693504C160299B2031FA6FFF5B42@TYCPR01MB5693.jpnprd01.prod.outlook.com>
 <1aa89c91-e3f1-4071-9fa9-14bffa600dd2@oss.nttdata.com>
 <TYAPR01MB5692411E16AB98D8CF72C2E7F5B72@TYAPR01MB5692.jpnprd01.prod.outlook.com>
 <4ab30d00-d434-4e82-af5d-260609075ff7@oss.nttdata.com>
 <TYAPR01MB5692DA275D54AC6653604C6BF5B32@TYAPR01MB5692.jpnprd01.prod.outlook.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: 
 <TYAPR01MB5692DA275D54AC6653604C6BF5B32@TYAPR01MB5692.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/b0d192d1-51ec-40f5-acde-0230ef2cc885%40oss.nttdata.com>
Precedence: bulk


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