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

On 2020/12/10 10:44, Bharath Rupireddy wrote:
> On Wed, Dec 9, 2020 at 4:49 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> I started reviewing 0001 patch.
>>
> 
> Thanks!
> 
>> 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.
 
>>
> 
> +1. I will make each patch self-contained in the next version which I
> plan to submit soon.
> 
>> Since 0001 introduces new user-visible functions into postgres_fdw, the version of postgres_fdw should be
increased?
>>
> 
> Yeah looks like we should do that, dblink has done that when it
> introduced new functions. In case the new functions are not required
> for anyone, they can choose to go back to 1.0.
> 
> Should we make the new version as 1.1 or 2.0? I prefer to make it 1.1
> as we are just adding few functionality over 1.0. I will change the
> default_version from 1.0 to the 1.1 and add a new
> postgres_fdw--1.1.sql file.

+1


> 
> If okay, I will make changes to 0001 patch.
> 
>> 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.
 
>>
> 
> +1. I will move the server name finding code to a new function, say
> char *pgfdw_get_server_name(ConnCacheEntry *entry);
> 
>> +                       /* 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?
>>
> 
> I checked this point earlier, for invalidated connections, the tuple
> returned from the cache is also invalid and the following error will
> be thrown. So, we can not get the server name for that user mapping.
> Cache entries too would have been invalidated after the connection is
> marked as invalid in pgfdw_inval_callback().
> 
> umaptup = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(entry->key));
> if (!HeapTupleIsValid(umaptup))
>        elog(ERROR, "cache lookup failed for user mapping with OID %u",
> entry->key);
> 
> Can we reload the sys cache entries of USERMAPPINGOID (if there is a
> way) for invalid connections in our new function and then do a look
> up? If not, another way could be storing the associated server name or
> oid in the ConnCacheEntry. Currently we store user mapping oid(in
> key), its hash value(in mapping_hashvalue) and foreign server oid's
> hash value (in server_hashvalue). If we have the foreign server oid,
> then we can just look up for the server name, but I'm not quite sure
> whether we get the same issue i.e. invalid tuples when the entry gets
> invalided (via pgfdw_inval_callback) due to some change in foreign
> server options.
> 
> IMHO, we can simply choose to show all the active, valid connections. Thoughts?
> 
>> Also this makes me wonder if we should return both the server name and boolean flag indicating whether it's
invalidatedor not. If so, users can easily find the invalidated connection entry and disconnect it because there is no
needto keep invalidated connection.
 
>>
> 
> Currently we are returning a list of foreing server names with whom
> there exist active connections. If we somehow address the above
> mentioned problem for invalid connections and choose to show them as
> well, then how should our output look like? Is it something like we
> prepare a list of pairs (servername, validflag)?
> 
>> +       if (all)
>> +       {
>> +               hash_destroy(ConnectionHash);
>> +               ConnectionHash = NULL;
>> +               result = true;
>> +       }
>>
>> Could you tell me why ConnectionHash needs to be destroyed?
>>
> 
> Say, in a session there are hundreds of different foreign server
> connections made and if users want to disconnect all of them with the
> new function and don't want any further foreign connections in that
> session, they can do it. But then why keep the cache just lying around
> and holding those many entries? Instead we can destroy the cache and
> if necessary it will be allocated later on next foreign server
> connections.
> 
> IMHO, it is better to destroy the cache in case of disconnect all,
> hoping to save memory, thinking that (next time if required) the cache
> allocation doesn't take much time. Thoughts?

Ok, but why is ConnectionHash destroyed only when "all" is true? Even when "all" is false, for example, the following
querycan disconnect all the cached connections. Even in this case, i.e., whenever there are no cached connections,
ConnectionHashshould be destroyed?
 

     SELECT postgres_fdw_disconnect(srvname) FROM pg_foreign_server ;

Regards,

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