On 2020/12/15 11:38, Bharath Rupireddy wrote:
Hi,
As discussed in [1], in postgres_fdw the cached connections to remote
servers can stay until the lifetime of the local session without
getting a chance to disconnect (connection leak), if the underlying
user mapping or foreign server is dropped in another session. Here are
few scenarios how this can happen:
Use case 1:
1) Run a foreign query in session 1 with server 1 and user mapping 1
2) Drop user mapping 1 in another session 2, an invalidation message
gets generated which will have to be processed by all the sessions
3) Run the foreign query again in session 1, at the start of txn the
cached entry gets invalidated via pgfdw_inval_callback() (as part of
invalidation message processing). 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
mapping 1 has been dropped in session 2 and the query will also fail
before reaching GetConnection() where the connections associated with
the invalidated entries would have got disconnected.
So, the connection associated with invalidated entry would remain
until the local session exits.
Use case 2:
1) Run a foreign query in session 1 with server 1 and user mapping 1
2) Try to drop foreign server 1, then we would not be allowed because
of dependency. Use CASCADE so that dependent objects i.e. user mapping
1 and foreign tables get dropped along with foreign server 1.
3) Run the foreign query again in session 1, at the start of txn, the
cached entry gets invalidated via pgfdw_inval_callback() and the query
fails because there is no foreign table.
Note that the remote connection remains open in session 1 until the
local session exits.
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
the following txn has to close all of them, but that's okay than
having leaked connections and it's a one time job for the following
one txn.
Attaching a patch for the same.
Thoughts?
Thanks for making the patch!
I agree to make pgfdw_xact_callback() close the connection when
entry->invalidated == true. But I think that it's better to get rid of
have_invalid_connections flag and make pgfdw_inval_callback() close
the connection immediately if entry->xact_depth == 0, to avoid unnecessary
scan of the hashtable during COMMIT of transaction not accessing to
foreign servers. Attached is the POC patch that I'm thinking. Thought?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/connection.c
b/contrib/postgres_fdw/connection.c
index 66581e5414..dcd14fa4b5 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -940,12 +940,14 @@ pgfdw_xact_callback(XactEvent event, void *arg)
entry->xact_depth = 0;
/*
- * If the connection isn't in a good idle state, discard it to
- * recover. Next GetConnection will open a new connection.
+ * If the connection isn't in a good idle state or it is marked
as
+ * invalid, then discard it to recover. Next GetConnection will
open a
+ * new connection.
*/
if (PQstatus(entry->conn) != CONNECTION_OK ||
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
- entry->changing_xact_state)
+ entry->changing_xact_state ||
+ entry->invalidated)
{
elog(DEBUG3, "discarding connection %p", entry->conn);
disconnect_pg_server(entry);
@@ -1102,7 +1104,15 @@ pgfdw_inval_callback(Datum arg, int cacheid, uint32
hashvalue)
entry->server_hashvalue == hashvalue) ||
(cacheid == USERMAPPINGOID &&
entry->mapping_hashvalue == hashvalue))
- entry->invalidated = true;
+ {
+ if (entry->xact_depth == 0)
+ {
+ elog(DEBUG3, "discarding connection %p",
entry->conn);
+ disconnect_pg_server(entry);
+ }
+ else
+ entry->invalidated = true;
+ }
}
}
