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

Dear Fujii-san,

Thanks for reviewing! PSA new version.

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

Right, fixed.

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

Added. I ran Grammarly and it said OK.

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

I did not done that be cause either of server_name or user_name is NULL and
it might be strange. But yes, the example should have more information.
Based on that, I added a tuple so that the example has below. Thought?

loopback1 - user is "postgres", valid
loopback2 - user is "public", valid
loopback3 - user is NULL, invalid

>
>
> +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);
> }
> ----------

Largely agreed, but some comments and Assertion() may be needed. Done.

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

Oh, my fault. I tried to remove GetUserMappingByOid() and the entry was also
Removed at that time. Restored.

Best regards,
Hayato Kuroda
FUJITSU LIMITED