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

On 2020/12/04 20:15, Bharath Rupireddy wrote:
> On Fri, Dec 4, 2020 at 1:46 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Fri, Dec 4, 2020 at 11:49 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>>> Attaching the v2 patch set. Please review it further.
>>>
>>> Regarding the 0001 patch, we should add the function that returns
>>> the information of cached connections like dblink_get_connections(),
>>> together with 0001 patch? Otherwise it's not easy for users to
>>> see how many cached connections are and determine whether to
>>> disconnect them or not. Sorry if this was already discussed before.
>>>
>>
>> Thanks for bringing this up. Exactly this is what I was thinking a few
>> days back. Say the new function postgres_fdw_get_connections() which
>> can return an array of server names whose connections exist in the
>> cache. Without this function, the user may not know how many
>> connections this backend has until he checks it manually on the remote
>> server.
>>
>> Thoughts? If okay, I can code the function in the 0001 patch.
>>
> 
> Added a new function postgres_fdw_get_connections() into 0001 patch,

Thanks!


> which returns a list of server names for which there exists an
> existing open and active connection.
> 
> Attaching v3 patch set, please review it further.

I started reviewing 0001 patch.

IMO the 0001 patch should be self-contained so that we can commit it at first. That is, I think that it's better to
movethe documents and tests for the functions 0001 patch adds from 0004 to 0001.
 

Since 0001 introduces new user-visible functions into postgres_fdw, the version of postgres_fdw should be increased?

The similar code to get the server name from cached connection entry exists also in
pgfdw_reject_incomplete_xact_state_change().I'm tempted to make the "common" function for that code and use it both in
postgres_fdw_get_connections()and pgfdw_reject_incomplete_xact_state_change(), to simplify the code.
 

+            /* We only look for active and open remote connections. */
+            if (entry->invalidated || !entry->conn)
+                continue;

We should return even invalidated entry because it has still cached connection?
Also this makes me wonder if we should return both the server name and boolean flag indicating whether it's invalidated
ornot. If so, users can easily find the invalidated connection entry and disconnect it because there is no need to keep
invalidatedconnection.
 

+    if (all)
+    {
+        hash_destroy(ConnectionHash);
+        ConnectionHash = NULL;
+        result = true;
+    }

Could you tell me why ConnectionHash needs to be destroyed?

Regards,

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