On Thu, Dec 24, 2020 at 7:21 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
> On 2020/12/23 23:40, Bharath Rupireddy wrote:
> > On Wed, Dec 23, 2020 at 7:31 PM Fujii Masao <masao.fu...@oss.nttdata.com>
> > wrote:
> >> 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?
> >
> > We could do that way as well. It seems okay to me. Now the disconnect
> > code is spread in pgfdw_inval_callback() and pgfdw_xact_callback().
> > There's also less burden on pgfdw_xact_callback() to close a lot of
> > connections on a single commit. The behaviour is like this - If
> > entry->xact_depth == 0, then the entries wouldn't have got any
> > connection in the current txn so they can be immediately closed in
> > pgfdw_inval_callback() and pgfdw_xact_callback() can exit immediately
> > as xact_got_connection is false. If entry->xact_depth > 0 which means
> > that probably pgfdw_inval_callback() came from a sub txn, we would
> > have got a connection in the txn i.e. xact_got_connection becomes true
> > due to which it can get invalidated in pgfdw_inval_callback() and get
> > closed in pgfdw_xact_callback() at the end of the txn.
> >
> > And there's no chance of entry->xact_depth > 0 and xact_got_connection
> > false.
> >
> > I think we need to change the comment before pgfdw_inval_callback() a bit:
> >
> > * After a change to a pg_foreign_server or pg_user_mapping catalog entry,
> > * mark connections depending on that entry as needing to be remade.
> > * We can't immediately destroy them, since they might be in the midst of
> > * a transaction, but we'll remake them at the next opportunity.
> >
> > to
> >
> > * After a change to a pg_foreign_server or pg_user_mapping catalog entry,
> > * try to close the cached connections associated with them if they
> > are not in the
> > * midst of a transaction otherwise mark them as invalidated. We will
> > destroy the
> > * invalidated connections in pgfdw_xact_callback() at the end of the main
> > xact.
> > * Closed connections will be remade at the next opportunity.
>
> Yes, I agree that we need to update that comment.
>
> >
> > Any thoughts on the earlier point in [1] related to a test case(which
> > becomes unnecessary with this patch) coverage?
> >
>
> ISTM that we can leave that test as it is because it's still useful to test
> the case where the cached connection is discarded in pgfdw_inval_callback().
> Thought?
Yes, that test case covers the code this patch adds i.e. closing the
connection in pgfdw_inval_callback() while committing alter foreign
server stmt.
> By applying the patch, probably we can get rid of the code to discard
> the invalidated cached connection in GetConnection(). But at least for
> the back branches, I'd like to leave the code as it is so that we can make
> sure that the invalidated cached connection doesn't exist when getting
> new connection. Maybe we can improve that in the master, but I'm not
> sure if it's really worth doing that against the gain. Thought?
+1 to keep that code as is even after this patch is applied(at least
it works as an assertion that we don't have any leftover invalid
connections). I'm not quite sure, we may need that in some cases, say
if we don't hit disconnect_pg_server() code in pgfdw_xact_callback()
and pgfdw_inval_callback() due to some errors in between. I can not
think of an exact use case though.
Attaching v2 patch that adds the comments and the other test case that
covers disconnecting code in pgfdw_xact_callback. Please review it.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 20f605a7586a6d6bc3cb671092c5c1e08c383c4f Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 24 Dec 2020 11:53:30 +0530
Subject: [PATCH v2] postgres_fdw - cached connection leaks if the associated
user mapping is dropped
In postgres_fdw the cached connections to remote servers can stay
until the lifetime of the local session, if the underlying user
mapping is dropped in another session.
To solve the above connection leak problem, it looks like the right
place to close the invalid connections is either in pgfdw_inval_callback()
if they are not in midst any xact, or in pgfdw_xact_callback(), which gets
called at the end of every act once registered, in the
current session(by then all the sub xacts also would have been finished).
Note that if there are too many invalidated entries, then the
following xact has to bear running this extra disconnect code, but
that's okay than having leaked connections.
---
contrib/postgres_fdw/connection.c | 32 ++++++++++++++-----
.../postgres_fdw/expected/postgres_fdw.out | 18 +++++++++++
contrib/postgres_fdw/sql/postgres_fdw.sql | 14 ++++++++
3 files changed, 56 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 66581e5414..c0d4f43f17 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);
@@ -1068,10 +1070,11 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
/*
* Connection invalidation callback function
*
- * After a change to a pg_foreign_server or pg_user_mapping catalog entry,
- * mark connections depending on that entry as needing to be remade.
- * We can't immediately destroy them, since they might be in the midst of
- * a transaction, but we'll remake them at the next opportunity.
+ * After a change to a pg_foreign_server or pg_user_mapping catalog entry, try
+ * to close the cached connections associated with them if they are not in the
+ * midst of a transaction otherwise mark them as invalid. We will destroy the
+ * invalidated connections in pgfdw_xact_callback() at the end of the main
+ * xact. Closed connections will be remade at the next opportunity.
*
* Although most cache invalidation callbacks blow away all the related stuff
* regardless of the given hashvalue, connections are expensive enough that
@@ -1102,7 +1105,20 @@ pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue)
entry->server_hashvalue == hashvalue) ||
(cacheid == USERMAPPINGOID &&
entry->mapping_hashvalue == hashvalue))
- entry->invalidated = true;
+ {
+ /*
+ * Close the connection if it's not in midst of a xact. Otherwise
+ * mark it as invalid so that it can be disconnected at the end of
+ * main xact in pgfdw_xact_callback().
+ */
+ if (entry->xact_depth == 0)
+ {
+ elog(DEBUG3, "discarding connection %p", entry->conn);
+ disconnect_pg_server(entry);
+ }
+ else
+ entry->invalidated = true;
+ }
}
}
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 2d88d06358..c11092f8cc 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9035,3 +9035,21 @@ ERROR: 08006
COMMIT;
-- Clean up
DROP PROCEDURE terminate_backend_and_wait(text);
+-- ===================================================================
+-- test connection invalidation cases
+-- ===================================================================
+-- This test case is for closing the connection in pgfdw_xact_callback
+BEGIN;
+-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
+SELECT 1 FROM ft1 LIMIT 1;
+ ?column?
+----------
+ 1
+(1 row)
+
+-- Connection is not closed at the end of the alter statement in
+-- pgfdw_inval_callback. That's because the connection is in midst of this
+-- xact, it is just marked as invalid.
+ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
+-- The invalid connection gets closed in pgfdw_xact_callback during commit.
+COMMIT;
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 7581c5417b..25dbc08b98 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2697,3 +2697,17 @@ COMMIT;
-- Clean up
DROP PROCEDURE terminate_backend_and_wait(text);
+
+-- ===================================================================
+-- test connection invalidation cases
+-- ===================================================================
+-- This test case is for closing the connection in pgfdw_xact_callback
+BEGIN;
+-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
+SELECT 1 FROM ft1 LIMIT 1;
+-- Connection is not closed at the end of the alter statement in
+-- pgfdw_inval_callback. That's because the connection is in midst of this
+-- xact, it is just marked as invalid.
+ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
+-- The invalid connection gets closed in pgfdw_xact_callback during commit.
+COMMIT;
--
2.25.1
