Skip to content

Commit 546f143

Browse files
MasaoFujiiMasaoFujii
committed
postgres_fdw: Fix connection leak.
In postgres_fdw, the cached connections to foreign servers will not be closed until the local session exits if the user mappings or foreign servers that those connections depend on are dropped. Those connections can be leaked. To fix that connection leak issue, after a change to a pg_foreign_server or pg_user_mapping catalog entry, this commit makes postgres_fdw close the connections depending on that entry immediately if current transaction has not used those connections yet. Otherwise, mark those connections as invalid and then close them at the end of current transaction, since they cannot be closed in the midst of the transaction using them. Closed connections will be remade at the next opportunity if necessary. Back-patch to all supported branches. Author: Bharath Rupireddy Reviewed-by: Zhihong Yu, Zhijie Hou, Fujii Masao Discussion: https://postgr.es/m/CALj2ACVNcGH_6qLY-4_tXz8JLvA+4yeBThRfxMz7Oxbk1aHcpQ@mail.gmail.com
1 parent cd7d8cd commit 546f143

File tree

3 files changed

+58
-7
lines changed

3 files changed

+58
-7
lines changed

‎contrib/postgres_fdw/connection.c

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -868,12 +868,14 @@ pgfdw_xact_callback(XactEvent event, void *arg)
868868
entry->xact_depth = 0;
869869

870870
/*
871-
* If the connection isn't in a good idle state, discard it to
872-
* recover. Next GetConnection will open a new connection.
871+
* If the connection isn't in a good idle state or it is marked as
872+
* invalid, then discard it to recover. Next GetConnection will open a
873+
* new connection.
873874
*/
874875
if (PQstatus(entry->conn) != CONNECTION_OK ||
875876
PQtransactionStatus(entry->conn) != PQTRANS_IDLE ||
876-
entry->changing_xact_state)
877+
entry->changing_xact_state ||
878+
entry->invalidated)
877879
{
878880
elog(DEBUG3, "discarding connection %p", entry->conn);
879881
disconnect_pg_server(entry);
@@ -997,9 +999,12 @@ pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
997999
* Connection invalidation callback function
9981000
*
9991001
* After a change to a pg_foreign_server or pg_user_mapping catalog entry,
1000-
* mark connections depending on that entry as needing to be remade.
1001-
* We can't immediately destroy them, since they might be in the midst of
1002-
* a transaction, but we'll remake them at the next opportunity.
1002+
* close connections depending on that entry immediately if current transaction
1003+
* has not used those connections yet. Otherwise, mark those connections as
1004+
* invalid and then make pgfdw_xact_callback() close them at the end of current
1005+
* transaction, since they cannot be closed in the midst of the transaction
1006+
* using them. Closed connections will be remade at the next opportunity if
1007+
* necessary.
10031008
*
10041009
* Although most cache invalidation callbacks blow away all the related stuff
10051010
* regardless of the given hashvalue, connections are expensive enough that
@@ -1030,7 +1035,21 @@ pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue)
10301035
entry->server_hashvalue == hashvalue) ||
10311036
(cacheid == USERMAPPINGOID &&
10321037
entry->mapping_hashvalue == hashvalue))
1033-
entry->invalidated = true;
1038+
{
1039+
/*
1040+
* Close the connection immediately if it's not used yet in this
1041+
* transaction. Otherwise mark it as invalid so that
1042+
* pgfdw_xact_callback() can close it at the end of this
1043+
* transaction.
1044+
*/
1045+
if (entry->xact_depth == 0)
1046+
{
1047+
elog(DEBUG3, "discarding connection %p", entry->conn);
1048+
disconnect_pg_server(entry);
1049+
}
1050+
else
1051+
entry->invalidated = true;
1052+
}
10341053
}
10351054
}
10361055

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8974,3 +8974,21 @@ PREPARE TRANSACTION 'fdw_tpc';
89748974
ERROR: cannot PREPARE a transaction that has operated on postgres_fdw foreign tables
89758975
ROLLBACK;
89768976
WARNING: there is no transaction in progress
8977+
-- ===================================================================
8978+
-- test connection invalidation cases
8979+
-- ===================================================================
8980+
-- This test case is for closing the connection in pgfdw_xact_callback
8981+
BEGIN;
8982+
-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
8983+
SELECT 1 FROM ft1 LIMIT 1;
8984+
?column?
8985+
----------
8986+
1
8987+
(1 row)
8988+
8989+
-- Connection is not closed at the end of the alter statement in
8990+
-- pgfdw_inval_callback. That's because the connection is in midst of this
8991+
-- xact, it is just marked as invalid.
8992+
ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
8993+
-- The invalid connection gets closed in pgfdw_xact_callback during commit.
8994+
COMMIT;

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2634,3 +2634,17 @@ SELECT count(*) FROM ft1;
26342634
-- error here
26352635
PREPARE TRANSACTION 'fdw_tpc';
26362636
ROLLBACK;
2637+
2638+
-- ===================================================================
2639+
-- test connection invalidation cases
2640+
-- ===================================================================
2641+
-- This test case is for closing the connection in pgfdw_xact_callback
2642+
BEGIN;
2643+
-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
2644+
SELECT 1 FROM ft1 LIMIT 1;
2645+
-- Connection is not closed at the end of the alter statement in
2646+
-- pgfdw_inval_callback. That's because the connection is in midst of this
2647+
-- xact, it is just marked as invalid.
2648+
ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
2649+
-- The invalid connection gets closed in pgfdw_xact_callback during commit.
2650+
COMMIT;

0 commit comments

Comments
 (0)