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

On Sat, Dec 12, 2020 at 12:19 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>
wrote:
> I was thinking that in the case of drop of user mapping or server,
hash_search(ConnnectionHash) in GetConnection() cannot find the cached
connection entry invalidated by that drop. Because "user->umid" used as
hash key is changed. So I was thinking that that invalidated connection
will not be closed nor reconnected.
>

You are right in saying that the connection leaks.

Use case 1:
1) Run foreign query in session1 with server1, user mapping1
2) Drop user mapping1 in another session2, invalidation message gets logged
which will have to be processed by other sessions
3) Run foreign query again in session1, at the start of txn, the cached
entry gets invalidated via pgfdw_inval_callback(). Whatever may be the type
of foreign query (select, update, explain, delete, insert, analyze etc.),
upon next call to GetUserMapping() from postgres_fdw.c, the cache lookup
fails(with ERROR: user mapping not found for "XXXX") since the user
mapping1 has been dropped in session2 and the query will also fail before
reaching GetConnection() where the connections associated with invalidated
entries would have got disconnected.

So, the connection associated with invalidated entry would remain until the
local session exits which is a problem to solve.

Use case 2:
1) Run foreign query in session1 with server1, user mapping1
2) Try to drop foreign server1, then we would not be allowed to do so
because of dependency. If we use CASCADE, then the dependent user mapping1
and foreign tables get dropped too [1].
3) Run foreign query again in session1, at the start of txn, the cached
entry gets invalidated via pgfdw_inval_callback(), it fails because there
is no foreign table and user mapping1.

But, note that the connection remains open in session1, which is again a
problem to solve.

To solve the above connection leak problem, it looks like the right place
to close all the invalid connections is pgfdw_xact_callback(), once
registered, which gets called at the end of every txn in the current
session(by then all the sub txns also would have been finished). Note that
if there are too many invalidated entries, then one of the following txn
has to bear running this extra code, but that's okay than having leaked
connections. Thoughts? If okay, I can code a separate patch.

static void
pgfdw_xact_callback(XactEvent event, void *arg)
{
HASH_SEQ_STATUS scan;
ConnCacheEntry *entry;
* /* HERE WE CAN LOOK FOR ALL INVALIDATED ENTRIES AND DISCONNECT THEM*/*
/* Quick exit if no connections were touched in this transaction. */
if (!xact_got_connection)
return;

And we can also extend postgres_fdw_disconnect() something like.

postgres_fdw_disconnect(bool invalid_only) --> default for invalid_only
false. disconnects all connections. when invalid_only is set to true then
disconnects only invalid connections.
postgres_fdw_disconnect('server_name') --> disconnections connections
associated with the specified foreign server

Having said this, I'm not in favour of invalid_only flag, because if we
choose to change the code in pgfdw_xact_callback to solve connection leak
problem, we may not need this invalid_only flag at all, because at the end
of txn (even for the txns in which the queries fail with error,
pgfdw_xact_callback gets called), all the existing invalid connections get
disconnected. Thoughts?

[1]
postgres=# drop server loopback1 ;
ERROR: cannot drop server loopback1 because other objects depend on it
DETAIL: user mapping for bharath on server loopback1 depends on server
loopback1
foreign table f1 depends on server loopback1
HINT: Use DROP ... CASCADE to drop the dependent objects too.

postgres=# drop server loopback1 CASCADE ;
NOTICE: drop cascades to 2 other objects
DETAIL: drop cascades to user mapping for bharath on server loopback1
drop cascades to foreign table f1
DROP SERVER

> > if (entry->conn != NULL && entry->invalidated && entry->xact_depth
== 0)
> > {
> > elog(DEBUG3, "closing connection %p for option changes to take
effect",
> > entry->conn);
> > disconnect_pg_server(entry);
> > }
> >
> >> If so, this seems like a connection-leak bug, at least for me....
Thought?
> >>
> >
> > It's not a leak. The comment before pgfdw_inval_callback() [1]
> > explains why we can not immediately close/disconnect the connections
> > in pgfdw_inval_callback() after marking them as invalidated.
>
> *If* invalidated connection cannot be close immediately even in the case
of drop of server or user mapping, we can defer it to the subsequent call
to GetConnection(). That is, GetConnection() closes not only the target
invalidated connection but also the other all invalidated connections. Of
course, invalidated connections will remain until subsequent
GetConnection() is called, though.
>

I think my detailed response to the above comment clarifies this.

> > Here is the scenario how in the midst of a txn we get invalidation
> >
messages(AtStart_Cache()-->AcceptInvalidationMessages()-->pgfdw_inval_callback()
> > happens):
> >
> > 1) select from foreign table with server1, usermapping1 in session1
> > 2) begin a top txn in session1, run a few foreign queries that open up
> > sub txns internally. meanwhile alter/drop server1/usermapping1 in
> > session2, then at each start of sub txn also we get to process the
> > invalidation messages via
> > AtStart_Cache()-->AcceptInvalidationMessages()-->pgfdw_inval_callback().
> > So, if we disconnect right after marking invalidated in
> > pgfdw_inval_callback, that's a problem since we are in a sub txn under
> > a top txn.
>
> Maybe. But what is the actual problem here?
>
> OTOH, if cached connection should not be close in the middle of
transaction, postgres_fdw_disconnect() also should be disallowed to be
executed during transaction?

+1. Yeah that makes sense. We can avoid closing the connection if
(entry->xact_depth > 0). I will modify it in
disconnect_cached_connections().

> >> Could you tell me why ConnectionHash needs to be destroyed?
> >
> > Say, in a session there are hundreds of different foreign server
> > connections made and if users want to disconnect all of them with the
> > new function and don't want any further foreign connections in that
> > session, they can do it. But then why keep the cache just lying around
> > and holding those many entries? Instead we can destroy the cache and
> > if necessary it will be allocated later on next foreign server
> > connections.
> >
> > IMHO, it is better to destroy the cache in case of disconnect all,
> > hoping to save memory, thinking that (next time if required) the cache
> > allocation doesn't take much time. Thoughts?
>
> Ok, but why is ConnectionHash destroyed only when "all" is true? Even
when "all" is false, for example, the following query can disconnect all
the cached connections. Even in this case, i.e., whenever there are no
cached connections, ConnectionHash should be destroyed?
>
> SELECT postgres_fdw_disconnect(srvname) FROM pg_foreign_server ;

+1. I can check all the cache entries to see if there are any active
connections, in the same loop where I try to find the cache entry for the
given foreign server, if none exists, then I destroy the cache. Thoughts?

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