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
> <a(dot)kondratov(at)postgrespro(dot)ru> 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