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

On Wed, Jan 20, 2021 at 6:58 PM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> + * 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.
> ---------------------

Done.

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

Yes. "otherwise false" means it.

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

Done.

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

Rephrased the function comment a bit. Mentioned the _disconnect and
_disconnect_all in comments because we have said enough what each of
those two functions do.

+/*
+ * Workhorse to disconnect cached connections.
+ *
+ * This function disconnects either all unused connections when called from
+ * postgres_fdw_disconnect_all or a given foreign server unused connection when
+ * called from postgres_fdw_disconnect.
+ *
+ * This function returns true if at least one connection is disconnected,
+ * otherwise false.
+ */

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

+1. So, I changed it to GetForeignServerExtended, added an assertion
for invalidation just like postgres_fdw_get_connections. I also added
a test case for this, we now emit a slightly different warning for
this case alone that is (errmsg("cannot close dropped server
connection because it is still in use")));. This warning looks okay as
we cannot show any other server name in the ouput and we know that
this rare case can exist when someone drops the server in an explicit
transaction.

> + 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)"?

I don't think so entry->server_hashvalue can be zero, because
GetSysCacheHashValue1/CatalogCacheComputeHashValue will not return 0
as hash value. I have not seen someone comparing hashvalue with an
expectation that it has 0 value, for instance see if (hashvalue == 0
|| riinfo->oidHashValue == hashvalue).

Having if(!all) something like below there doesn't suffice because we
might call hash_seq_term, when some connection other than the given
foreign server connection is in use. Our intention to call
hash_seq_term is only when a given server is found and either it's in
use or is closed.

if (!all && (entry->xact_depth > 0 || result))
{
hash_seq_term(&scan);
break;
}

Given the above points, the existing check looks good to me.

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

+1.

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

I think the order of the above warnings depends on how the connections
are stored in cache and we emit the warnings. Looks like new cached
connections are stored at the beginning of the cache always and the
warnings also will show up in that order i.e new entries to old
entries. I think it's stable and I didn't see cfbot complaining about
that on v14.

> + <varlistentry>
> + <term><function>postgres_fdw_disconnect(IN servername text) returns boolean</function></term>
>
> I think that "IN" of "IN servername text" is not necessary.

Done.

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

Done.

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

Done.

> +postgres=# SELECT * FROM postgres_fdw_disconnect('loopback1');
>
> "SELECT postgres_fdw_disconnect('loopback1')" is more common?

Done.

Attaching v15 patch set. Please consider it for further review.

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