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

On 2021/01/20 19:17, Bharath Rupireddy wrote:
> On Wed, Jan 20, 2021 at 3:24 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>> Keeping above in mind, I feel we can do hash_seq_search(), as we do
>>> currently, even when the server name is given as input. This way, we
>>> don't need to bother much on the above points.
>>>
>>> Thoughts?
>>
>> Thanks for explaining this! You're right. I'd withdraw my suggestion.
>
> Attaching v14 patch set with review comments addressed. Please review
> it further.

Thanks for updating the patch!

+ * It checks if the cache has a connection for the given foreign server that's
+ * not being used within current transaction, then returns true. If the
+ * connection is in use, then it emits a warning and returns false.

The comment also should mention the case where no open connection
for the given server is found? What about rewriting this to the following?

---------------------
If the cached connection for the given foreign server is found and has not
been used within current transaction yet, close the connection and return
true. Even when it's found, if it's already used, keep the connection, emit
a warning and return false. If it's not found, return false.
---------------------

+ * It returns true, if it closes at least one connection, otherwise false.
+ *
+ * It returns false, if the cache doesn't exit.

The above second comment looks redundant.

+ if (ConnectionHash)
+ result = disconnect_cached_connections(0, true);

Isn't it smarter to make disconnect_cached_connections() check
ConnectionHash and return false if it's NULL? If we do that, we can
simplify the code of postgres_fdw_disconnect() and _all().

+ * current transaction are disconnected. Otherwise, the unused entries with the
+ * given hashvalue are disconnected.

In the above second comment, a singular form should be used instead?
Because there must be no multiple entries with the given hashvalue.

+ server = GetForeignServer(entry->serverid);

This seems to cause an error "cache lookup failed"
if postgres_fdw_disconnect_all() is called when there is
a connection in use but its server is dropped. To avoid this error,
GetForeignServerExtended() with FSV_MISSING_OK should be used
instead, like postgres_fdw_get_connections() does?

+ if (entry->server_hashvalue == hashvalue &&
+ (entry->xact_depth > 0 || result))
+ {
+ hash_seq_term(&scan);
+ break;

entry->server_hashvalue can be 0? If yes, since postgres_fdw_disconnect_all()
specifies 0 as hashvalue, ISTM that the above condition can be true
unexpectedly. Can we replace this condition with just "if (!all)"?

+-- Closes loopback connection, returns true and issues a warning as loopback2
+-- connection is still in use and can not be closed.
+SELECT * FROM postgres_fdw_disconnect_all();
+WARNING: cannot close connection for server "loopback2" because it is still in use
+ postgres_fdw_disconnect_all
+-----------------------------
+ t
+(1 row)

After the above test, isn't it better to call postgres_fdw_get_connections()
to check that loopback is not output?

+WARNING: cannot close connection for server "loopback" because it is still in use
+WARNING: cannot close connection for server "loopback2" because it is still in use

Just in the case please let me confirm that the order of these warning
messages is always stable?

+ <varlistentry>
+ <term><function>postgres_fdw_disconnect(IN servername text) returns boolean</function></term>

I think that "IN" of "IN servername text" is not necessary.

I'd like to replace "servername" with "server_name" because
postgres_fdw_get_connections() uses "server_name" as the output
column name.

+ <listitem>
+ <para>
+ When called in local session with foreign server name as input, it
+ discards the unused open connection previously made to the foreign server
+ and returns <literal>true</literal>.

"unused open connection" sounds confusing to me. What about the following?

---------------------
This function discards the open connection that postgres_fdw established
from the local session to the foriegn server with the given name if it's not
used in the current local transaction yet, and then returns true. If it's
already used, the function doesn't discard the connection, emits
a warning and then returns false. If there is no open connection to
the given foreign server, false is returned. If no foreign server with
the given name is found, an error is emitted. Example usage of the function:
---------------------

+postgres=# SELECT * FROM postgres_fdw_disconnect('loopback1');

"SELECT postgres_fdw_disconnect('loopback1')" is more common?

Regards,

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