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

On 2021/01/19 12:09, Bharath Rupireddy wrote:
> On Mon, Jan 18, 2021 at 9:11 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>> Attaching v12 patch set. 0001 is for postgres_fdw_disconnect()
>>> function, 0002 is for keep_connections GUC and 0003 is for
>>> keep_connection server level option.
>>
>> Thanks!
>>
>>>
>>> Please review it further.
>>
>> + server = GetForeignServerByName(servername, true);
>> +
>> + if (!server)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_CONNECTION_DOES_NOT_EXIST),
>> + errmsg("foreign server \"%s\" does not exist", servername)));
>>
>> ISTM we can simplify this code as follows.
>>
>> server = GetForeignServerByName(servername, false);
>
> Done.
>
>> + hash_seq_init(&scan, ConnectionHash);
>> + while ((entry = (ConnCacheEntry *) hash_seq_search(&scan)))
>>
>> When the server name is specified, even if its connection is successfully
>> closed, postgres_fdw_disconnect() scans through all the entries to check
>> whether there are active connections. But if "result" is true and
>> active_conn_exists is true, we can get out of this loop to avoid unnecessary
>> scans.
>
> My initial thought was that it's possible to have two entries with the
> same foreign server name but with different user mappings, looks like
> it's not possible. I tried associating a foreign server with two
> different user mappings [1], then the cache entry is getting
> associated initially with the user mapping that comes first in the
> pg_user_mappings, if this user mapping is dropped then the cache entry
> gets invalidated, so next time the second user mapping is used.
>
> Since there's no way we can have two cache entries with the same
> foreign server name, we can get out of the loop when we find the cache
> entry match with the given server. I made the changes.

So, furthermore, we can use hash_search() to find the target cached
connection, instead of using hash_seq_search(), when the server name
is given. This would simplify the code a bit more? Of course,
hash_seq_search() is necessary when closing all the connections, though.

>
> [1]
> postgres=# select * from pg_user_mappings ;
> umid | srvid | srvname | umuser | usename | umoptions
> -------+-------+-----------+--------+---------+-----------
> 16395 | 16394 | loopback1 | 10 | bharath | -----> cache entry
> is initially made with this user mapping.
> 16399 | 16394 | loopback1 | 0 | public | -----> if the
> above user mapping is dropped, then the cache entry is made with this
> user mapping.
>
>> + /*
>> + * Destroy the cache if we discarded all active connections i.e. if there
>> + * is no single active connection, which we can know while scanning the
>> + * cached entries in the above loop. Destroying the cache is better than to
>> + * keep it in the memory with all inactive entries in it to save some
>> + * memory. Cache can get initialized on the subsequent queries to foreign
>> + * server.
>>
>> How much memory is assumed to be saved by destroying the cache in
>> many cases? I'm not sure if it's really worth destroying the cache to save
>> the memory.
>
> I removed the cache destroying code, if somebody complains in
> future(after the feature commit), we can really revisit then.
>
>> + a warning is issued and <literal>false</literal> is returned. <literal>false</literal>
>> + is returned when there are no open connections. When there are some open
>> + connections, but there is no connection for the given foreign server,
>> + then <literal>false</literal> is returned. When no foreign server exists
>> + with the given name, an error is emitted. Example usage of the function:
>>
>> When a non-existent server name is specified, postgres_fdw_disconnect()
>> emits an error if there is at least one open connection, but just returns
>> false otherwise. At least for me, this behavior looks inconsitent and strange.
>> In that case, IMO the function always should emit an error.
>
> Done.
>
> Attaching v13 patch set, please review it further.

Thanks!

+ * 2) If no input argument is provided, then it tries to disconnect all the
+ * connections.

I'm concerned that users can easily forget to specify the argument and
accidentally discard all the connections. So, IMO, to alleviate this situation,
what about changing the function name (only when closing all the connections)
to something postgres_fdw_disconnect_all(), like we have
pg_advisory_unlock_all() against pg_advisory_unlock()?

+ if (result)
+ {
+ /* We closed at least one connection, others are in use. */
+ ereport(WARNING,
+ (errmsg("cannot close all connections because some of them are still in use")));
+ }

Sorry if this was already discussed upthread. Isn't it more helpful to
emit a warning for every connections that fail to be closed? For example,

WARNING: cannot close connection for server "loopback1" because it is still in use
WARNING: cannot close connection for server "loopback2" because it is still in use
WARNING: cannot close connection for server "loopback3" because it is still in use
...

This enables us to identify easily which server connections cannot be
closed for now.

Regards,

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