From a5ff26edbf54fca4d42833ef4aedf79bec9445bd Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Sat, 8 May 2021 19:51:58 +0530
Subject: [PATCH v1] postgres_fdw-make cached connection functions tests
 meaningful

The new GUC debug_invalidate_system_caches_always which has been
introduced in v14 can be used to switch off cache invalidation in
CLOBBER_CACHE_ALWAYS builds which can make cache sensitive tests
stable. Using this GUC, it is quite possible to make cached
connection management function tests more meaningful by returning
original values(true/false, all the output columns) instead of
SELECT 1. Note that the commit f77717b29 stabilized the tests for
those functions - postgres_fdw_disconnect, postgres_fdw_disconnect_all
and postgres_fdw_get_connections by masking actual return value of
the functions.
---
 .../postgres_fdw/expected/postgres_fdw.out    | 135 ++++++++++--------
 contrib/postgres_fdw/sql/postgres_fdw.sql     |  58 ++++----
 2 files changed, 100 insertions(+), 93 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 6f533c745d..67d993edb8 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9198,17 +9198,26 @@ PREPARE TRANSACTION 'fdw_tpc';
 ERROR:  cannot PREPARE a transaction that has operated on postgres_fdw foreign tables
 ROLLBACK;
 WARNING:  there is no transaction in progress
+-- Let's ensure to close all the existing cached connections so that they don't
+-- make the following tests unstable.
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column? 
+----------
+        1
+(1 row)
+
+-- If debug_invalidate_system_caches_always is active, it results in dropping
+-- remote connections after every transaction, making it impossible to test
+-- connection retry use case meaningfully. And also, the new functions
+-- introduced for cached connections management will be unstable with the flag
+-- on. So turn it off for these tests.
+SET debug_invalidate_system_caches_always = 0;
 -- ===================================================================
 -- reestablish new connection
 -- ===================================================================
 -- Change application_name of remote connection to special one
 -- so that we can easily terminate the connection later.
 ALTER SERVER loopback OPTIONS (application_name 'fdw_retry_check');
--- If debug_invalidate_system_caches_always is active, it results in
--- dropping remote connections after every transaction, making it
--- impossible to test termination meaningfully.  So turn that off
--- for this test.
-SET debug_invalidate_system_caches_always = 0;
 -- Make sure we have a remote connection.
 SELECT 1 FROM ft1 LIMIT 1;
  ?column? 
@@ -9250,21 +9259,13 @@ SELECT 1 FROM ft1 LIMIT 1;    -- should fail
 ERROR:  08006
 \set VERBOSITY default
 COMMIT;
-RESET debug_invalidate_system_caches_always;
 -- =============================================================================
 -- test connection invalidation cases and postgres_fdw_get_connections function
 -- =============================================================================
--- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
- ?column? 
-----------
-        1
-(1 row)
-
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
 (0 rows)
 
 -- This test case is for closing the connection in pgfdw_xact_callback
@@ -9284,11 +9285,11 @@ SELECT 1 FROM ft7 LIMIT 1;
 
 -- List all the existing cached connections. loopback and loopback3 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
- loopback
- loopback3
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
+ loopback    | t
+ loopback3   | t
 (2 rows)
 
 -- Connections are not closed at the end of the alter and drop statements.
@@ -9313,9 +9314,9 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 COMMIT;
 -- All cached connections were closed while committing above xact, so no
 -- records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
 (0 rows)
 
 -- =======================================================================
@@ -9338,11 +9339,11 @@ SELECT 1 FROM ft6 LIMIT 1;
 
 -- List all the existing cached connections. loopback and loopback2 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
- loopback
- loopback2
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
+ loopback    | t
+ loopback2   | t
 (2 rows)
 
 -- Issue a warning and return false as loopback connection is still in use and
@@ -9356,11 +9357,11 @@ WARNING:  cannot close connection for server "loopback" because it is still in u
 
 -- List all the existing cached connections. loopback and loopback2 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
- loopback
- loopback2
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
+ loopback    | t
+ loopback2   | t
 (2 rows)
 
 -- Return false as connections are still in use, warnings are issued.
@@ -9375,15 +9376,15 @@ SELECT postgres_fdw_disconnect_all();
 RESET client_min_messages;
 COMMIT;
 -- Ensure that loopback2 connection is closed.
-SELECT 1 FROM postgres_fdw_disconnect('loopback2');
- ?column? 
-----------
-        1
+SELECT postgres_fdw_disconnect('loopback2');
+ postgres_fdw_disconnect 
+-------------------------
+ t
 (1 row)
 
-SELECT server_name FROM postgres_fdw_get_connections() WHERE server_name = 'loopback2';
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() WHERE server_name = 'loopback2';
+ server_name | valid 
+-------------+-------
 (0 rows)
 
 -- Return false as loopback2 connection is closed already.
@@ -9397,18 +9398,25 @@ SELECT postgres_fdw_disconnect('loopback2');
 SELECT postgres_fdw_disconnect('unknownserver');
 ERROR:  server "unknownserver" does not exist
 -- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
- ?column? 
-----------
-        1
+SELECT postgres_fdw_disconnect_all();
+ postgres_fdw_disconnect_all 
+-----------------------------
+ t
 (1 row)
 
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
 (0 rows)
 
+-- No cached connections, so false should be output.
+SELECT postgres_fdw_disconnect_all();
+ postgres_fdw_disconnect_all 
+-----------------------------
+ f
+(1 row)
+
 -- =============================================================================
 -- test case for having multiple cached connections for a foreign server
 -- =============================================================================
@@ -9436,25 +9444,25 @@ SELECT 1 FROM ft1 LIMIT 1;
 
 RESET ROLE;
 -- Should output two connections for loopback server
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
- loopback
- loopback
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
+ loopback    | t
+ loopback    | t
 (2 rows)
 
 COMMIT;
 -- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
- ?column? 
-----------
-        1
+SELECT postgres_fdw_disconnect_all();
+ postgres_fdw_disconnect_all 
+-----------------------------
+ t
 (1 row)
 
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
 (0 rows)
 
 -- Clean up
@@ -9477,12 +9485,13 @@ SELECT 1 FROM ft1 LIMIT 1;
 (1 row)
 
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name 
--------------
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name | valid 
+-------------+-------
 (0 rows)
 
 ALTER SERVER loopback OPTIONS (SET keep_connections 'on');
+RESET debug_invalidate_system_caches_always;
 -- ===================================================================
 -- batch insert
 -- ===================================================================
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 000e2534fc..f2a502f992 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2788,32 +2788,32 @@ SELECT count(*) FROM ft1;
 PREPARE TRANSACTION 'fdw_tpc';
 ROLLBACK;
 
+-- Let's ensure to close all the existing cached connections so that they don't
+-- make the following tests unstable.
+SELECT 1 FROM postgres_fdw_disconnect_all();
+
+-- If debug_invalidate_system_caches_always is active, it results in dropping
+-- remote connections after every transaction, making it impossible to test
+-- connection retry use case meaningfully. And also, the new functions
+-- introduced for cached connections management will be unstable with the flag
+-- on. So turn it off for these tests.
+SET debug_invalidate_system_caches_always = 0;
+
 -- ===================================================================
 -- reestablish new connection
 -- ===================================================================
-
 -- Change application_name of remote connection to special one
 -- so that we can easily terminate the connection later.
 ALTER SERVER loopback OPTIONS (application_name 'fdw_retry_check');
-
--- If debug_invalidate_system_caches_always is active, it results in
--- dropping remote connections after every transaction, making it
--- impossible to test termination meaningfully.  So turn that off
--- for this test.
-SET debug_invalidate_system_caches_always = 0;
-
 -- Make sure we have a remote connection.
 SELECT 1 FROM ft1 LIMIT 1;
-
 -- Terminate the remote connection and wait for the termination to complete.
 SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
 	WHERE application_name = 'fdw_retry_check';
-
 -- This query should detect the broken connection when starting new remote
 -- transaction, reestablish new connection, and then succeed.
 BEGIN;
 SELECT 1 FROM ft1 LIMIT 1;
-
 -- If we detect the broken connection when starting a new remote
 -- subtransaction, we should fail instead of establishing a new connection.
 -- Terminate the remote connection and wait for the termination to complete.
@@ -2826,15 +2826,11 @@ SELECT 1 FROM ft1 LIMIT 1;    -- should fail
 \set VERBOSITY default
 COMMIT;
 
-RESET debug_invalidate_system_caches_always;
-
 -- =============================================================================
 -- test connection invalidation cases and postgres_fdw_get_connections function
 -- =============================================================================
--- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 -- 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.
@@ -2842,7 +2838,7 @@ SELECT 1 FROM ft1 LIMIT 1;
 SELECT 1 FROM ft7 LIMIT 1;
 -- List all the existing cached connections. loopback and loopback3 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 -- Connections are not closed at the end of the alter and drop statements.
 -- That's because the connections are in midst of this xact,
 -- they are just marked as invalid in pgfdw_inval_callback.
@@ -2856,7 +2852,7 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 COMMIT;
 -- All cached connections were closed while committing above xact, so no
 -- records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 
 -- =======================================================================
 -- test postgres_fdw_disconnect and postgres_fdw_disconnect_all functions
@@ -2868,13 +2864,13 @@ SELECT 1 FROM ft1 LIMIT 1;
 SELECT 1 FROM ft6 LIMIT 1;
 -- List all the existing cached connections. loopback and loopback2 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 -- Issue a warning and return false as loopback connection is still in use and
 -- can not be closed.
 SELECT postgres_fdw_disconnect('loopback');
 -- List all the existing cached connections. loopback and loopback2 should be
 -- output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 -- Return false as connections are still in use, warnings are issued.
 -- But disable warnings temporarily because the order of them is not stable.
 SET client_min_messages = 'ERROR';
@@ -2882,16 +2878,18 @@ SELECT postgres_fdw_disconnect_all();
 RESET client_min_messages;
 COMMIT;
 -- Ensure that loopback2 connection is closed.
-SELECT 1 FROM postgres_fdw_disconnect('loopback2');
-SELECT server_name FROM postgres_fdw_get_connections() WHERE server_name = 'loopback2';
+SELECT postgres_fdw_disconnect('loopback2');
+SELECT * FROM postgres_fdw_get_connections() WHERE server_name = 'loopback2';
 -- Return false as loopback2 connection is closed already.
 SELECT postgres_fdw_disconnect('loopback2');
 -- Return an error as there is no foreign server with given name.
 SELECT postgres_fdw_disconnect('unknownserver');
 -- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
+SELECT postgres_fdw_disconnect_all();
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- No cached connections, so false should be output.
+SELECT postgres_fdw_disconnect_all();
 
 -- =============================================================================
 -- test case for having multiple cached connections for a foreign server
@@ -2906,19 +2904,17 @@ BEGIN;
 SET ROLE regress_multi_conn_user1;
 SELECT 1 FROM ft1 LIMIT 1;
 RESET ROLE;
-
 -- Will cache loopback connection with user mapping for regress_multi_conn_user2
 SET ROLE regress_multi_conn_user2;
 SELECT 1 FROM ft1 LIMIT 1;
 RESET ROLE;
-
 -- Should output two connections for loopback server
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 COMMIT;
 -- Let's ensure to close all the existing cached connections.
-SELECT 1 FROM postgres_fdw_disconnect_all();
+SELECT postgres_fdw_disconnect_all();
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 
 -- Clean up
 DROP USER MAPPING FOR regress_multi_conn_user1 SERVER loopback;
@@ -2936,9 +2932,11 @@ ALTER SERVER loopback OPTIONS (keep_connections 'off');
 -- as keep_connections was set to off.
 SELECT 1 FROM ft1 LIMIT 1;
 -- No cached connections, so no records should be output.
-SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
 ALTER SERVER loopback OPTIONS (SET keep_connections 'on');
 
+RESET debug_invalidate_system_caches_always;
+
 -- ===================================================================
 -- batch insert
 -- ===================================================================
-- 
2.25.1

