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
v3-0001-Extend-postgres_fdw_get_connections-to-return-use.patch
Description: v3-0001-Extend-postgres_fdw_get_connections-to-return-use.patch
