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

On 2021/01/18 23:14, Bharath Rupireddy wrote:
> On Mon, Jan 18, 2021 at 11:44 AM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>> I will post patches for the other function postgres_fdw_disconnect,
>>> GUC and server level option later.
>>
>> Thanks!
>
> 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);

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

+ /*
+ * 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.

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

Regards,

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