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

On 2020-11-19 07:11, Bharath Rupireddy wrote:
> On Wed, Nov 18, 2020 at 10:32 PM Alexey Kondratov
>  wrote:
> 
> Thanks! I will make separate patches and post them soon.
> 
>> 
>> >> Attached is a small POC patch, which implements this contrib-level
>> >> postgres_fdw.keep_connections GUC. What do you think?
>  >
>> > I see two problems with your patch: 1) It just disconnects the remote
>> > connection at the end of txn if the GUC is set to false, but it
>> > doesn't remove the connection cache entry from ConnectionHash.
>> 
>> Yes, and this looks like a valid state for postgres_fdw and it can get
>> into the same state even without my patch. Next time GetConnection()
>> will find this cache entry, figure out that entry->conn is NULL and
>> establish a fresh connection. It is not clear for me right now, what
>> benefits we will get from clearing also this cache entry, except just
>> doing this for sanity.
>> 
> 
> By clearing the cache entry we will have 2 advantages: 1) we could
> save a(small) bit of memory 2) we could allow new connections to be
> cached, currently ConnectionHash can have only 8 entries. IMHO, along
> with disconnecting, we can also clear off the cache entry. Thoughts?
> 

IIUC, 8 is not a hard limit, it is just a starting size. ConnectionHash 
is not a shared-memory hash table, so dynahash can expand it on-the-fly 
as follow, for example, from the comment before hash_create():

  * Note: for a shared-memory hashtable, nelem needs to be a pretty good
  * estimate, since we can't expand the table on the fly.  But an 
unshared
  * hashtable can be expanded on-the-fly, so it's better for nelem to be
  * on the small side and let the table grow if it's exceeded.  An overly
  * large nelem will penalize hash_seq_search speed without buying much.

Also I am not sure that by doing just a HASH_REMOVE you will free any 
memory, since hash table is already allocated (or expanded) to some 
size. So HASH_REMOVE will only add removed entry to the freeList, I 
guess.

Anyway, I can hardly imagine bloating of ConnectionHash to be a problem 
even in the case, when one has thousands of foreign servers all being 
accessed during a single backend life span.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company