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

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.

[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.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com