Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On 2021/01/18 15:37, Bharath Rupireddy wrote:
> On Mon, Jan 18, 2021 at 11:58 AM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>
>>
>>
>> On 2021/01/18 15:02, Hou, Zhijie wrote:
>>>> We need to create the loopback3 with user mapping public, otherwise the
>>>> test might become unstable as shown below. Note that loopback and
>>>> loopback2 are not dropped in the test, so no problem with them.
>>>>
>>>> ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off'); DROP
>>>> SERVER loopback3 CASCADE;
>>>> NOTICE: drop cascades to 2 other objects
>>>> -DETAIL: drop cascades to user mapping for postgres on server loopback3
>>>> +DETAIL: drop cascades to user mapping for bharath on server loopback3
>>>>
>>>> Attaching v2 patch for postgres_fdw_get_connections. Please have a look.
>>> Hi
>>>
>>> I have a comment for the doc about postgres_fdw_get_connections.
>>>
>>> + <term><function>postgres_fdw_get_connections(OUT server_name text, OUT valid boolean) returns setof record</function></term>
>>> + <listitem>
>>> + <para>
>>> + This function returns the foreign server names of all the open
>>> + connections that <filename>postgres_fdw</filename> established from
>>> + the local session to the foreign servers. It also returns whether
>>> + each connection is valid or not. <literal>false</literal> is returned
>>> + if the foreign server connection is used in the current local
>>> + transaction but its foreign server or user mapping is changed or
>>> + dropped, and then such invalid connection will be closed at
>>> + the end of that transaction. <literal>true</literal> is returned
>>> + otherwise. If there are no open connections, no record is returned.
>>> + Example usage of the function:
>>>
>>> The doc seems does not memtion the case when the function returns NULL in server_name.
>>> Users may be a little confused about why NULL was returned.
>>
>> Yes, so what about adding
>>
>> (Note that the returned server name of invalid connection is NULL if its server is dropped)
>>
>> into the following (just after "dropped")?
>>
>> + if the foreign server connection is used in the current local
>> + transaction but its foreign server or user mapping is changed or
>> + dropped
>>
>> Or better description?
>
> +1 to add it after "dropped (Note ........)", how about as follows
> with slight changes?
>
> dropped (Note that server name of an invalid connection can be NULL if
> the server is dropped), and then such .....

Yes, I like this one. One question is; "should" or "is" is better than
"can" in this case because the server name of invalid connection is
always NULL when its server is dropped?

Regards,

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