On 2021/01/29 19:45, Bharath Rupireddy wrote:
On Fri, Jan 29, 2021 at 1:24 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
On Fri, Jan 29, 2021 at 1:17 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
But if the issue is only the inconsistency of test results,
we can go with the option (2)? Even with (2), we can make the test
stable by removing "valid" column and executing
postgres_fdw_get_connections() within the transaction?
Hmmm, and we should have the tests at the start of the file
postgres_fdw.sql before even we make any foreign server connections.
We don't need to move the test if we always call postgres_fdw_disconnect_all()
just before starting new transaction and calling postgres_fdw_get_connections()
as follows?
SELECT 1 FROM postgres_fdw_disconnect_all();
BEGIN;
...
SELECT * FROM postgres_fdw_get_connections();
...
Yes, that works, but we cannot show true/false for the
postgres_fdw_disconnect_all output.
I will post the patch soon. Thanks a lot.
Attaching a patch that has following changes: 1) Now,
postgres_fdw_get_connections will only return set of active
connections server names not their valid state 2) The functions
postgres_fdw_get_connections, postgres_fdw_disconnect and
postgres_fdw_disconnect_all are now being tested within an explicit
xact block, this way the tests are more stable even with clobber cache
always builds.
I tested the patch here on my development system with
-DCLOBBER_CACHE_ALWAYS configuration, the tests look consistent.
Please review the patch.
Thanks for the patch!
--- Return false as loopback2 connectin is closed already.
-SELECT postgres_fdw_disconnect('loopback2');
- postgres_fdw_disconnect
--------------------------
- f
-(1 row)
-
--- Return an error as there is no foreign server with given name.
-SELECT postgres_fdw_disconnect('unknownserver');
-ERROR: server "unknownserver" does not exist
Why do we need to remove these? These seem to work fine even in
CLOBBER_CACHE_ALWAYS.
+ /*
+ * It doesn't make sense to show this entry in the
output with a
+ * NULL server_name as it will be closed at the xact
end.
+ */
+ continue;
-1 with this change because I still think that it's more useful to list
all the open connections.
This makes me think that more discussion would be necessary before
changing the interface of postgres_fdw_get_connections(). On the other
hand, we should address the issue ASAP to make the buildfarm member fine.
So at first I'd like to push only the change of regression test.
Patch attached. I tested it both with CLOBBER_CACHE_ALWAYS set and unset,
and the results were stable.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out
b/contrib/postgres_fdw/expected/postgres_fdw.out
index 07e06e5bf7..b09dce63f5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -17,10 +17,6 @@ DO $d$
OPTIONS (dbname '$$||current_database()||$$',
port '$$||current_setting('port')||$$'
)$$;
- EXECUTE $$CREATE SERVER loopback4 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
END;
$d$;
CREATE USER MAPPING FOR public SERVER testserver1
@@ -28,7 +24,6 @@ CREATE USER MAPPING FOR public SERVER testserver1
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2;
CREATE USER MAPPING FOR public SERVER loopback3;
-CREATE USER MAPPING FOR public SERVER loopback4;
-- ===================================================================
-- create objects used through FDW loopback server
-- ===================================================================
@@ -144,11 +139,6 @@ CREATE FOREIGN TABLE ft7 (
c2 int NOT NULL,
c3 text
) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
-CREATE FOREIGN TABLE ft8 (
- c1 int NOT NULL,
- c2 int NOT NULL,
- c3 text
-) SERVER loopback4 OPTIONS (schema_name 'S 1', table_name 'T 4');
-- ===================================================================
-- tests for validator
-- ===================================================================
@@ -220,8 +210,7 @@ ALTER FOREIGN TABLE ft2 ALTER COLUMN c1 OPTIONS
(column_name 'C 1');
public | ft5 | loopback | (schema_name 'S 1', table_name 'T 4') |
public | ft6 | loopback2 | (schema_name 'S 1', table_name 'T 4') |
public | ft7 | loopback3 | (schema_name 'S 1', table_name 'T 4') |
- public | ft8 | loopback4 | (schema_name 'S 1', table_name 'T 4') |
-(7 rows)
+(6 rows)
-- Test that alteration of server options causes reconnection
-- Remote's errors might be non-English, so hide them to ensure stable results
@@ -9066,15 +9055,21 @@ DROP PROCEDURE terminate_backend_and_wait(text);
--
=============================================================================
-- 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
+-------------
+(0 rows)
+
-- This test case is for closing the connection in pgfdw_xact_callback
BEGIN;
--- List all the existing cached connections. Only loopback2 should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback2 | t
-(1 row)
-
-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
SELECT 1 FROM ft1 LIMIT 1;
?column?
@@ -9088,19 +9083,18 @@ SELECT 1 FROM ft7 LIMIT 1;
1
(1 row)
--- List all the existing cached connections. loopback and loopback3
--- also should be output as valid connections.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback | t
- loopback2 | t
- loopback3 | t
-(3 rows)
+-- 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
+(2 rows)
--- 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.
+-- 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.
ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
DROP SERVER loopback3 CASCADE;
NOTICE: drop cascades to 2 other objects
@@ -9113,31 +9107,22 @@ SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
server_name | valid
-------------+-------
loopback | f
- loopback2 | t
| f
-(3 rows)
+(2 rows)
--- The invalid connection gets closed in pgfdw_xact_callback during commit.
+-- The invalid connections get closed in pgfdw_xact_callback during commit.
COMMIT;
--- List all the existing cached connections. loopback and loopback3
--- should not be output because they should be closed at the end of
--- the above transaction.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback2 | t
-(1 row)
+-- 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
+-------------
+(0 rows)
-- =======================================================================
-- test postgres_fdw_disconnect and postgres_fdw_disconnect_all functions
-- =======================================================================
--- Return true as all cached connections are closed.
-SELECT postgres_fdw_disconnect_all();
- postgres_fdw_disconnect_all
------------------------------
- t
-(1 row)
-
+BEGIN;
-- Ensure to cache loopback connection.
SELECT 1 FROM ft1 LIMIT 1;
?column?
@@ -9145,7 +9130,6 @@ SELECT 1 FROM ft1 LIMIT 1;
1
(1 row)
-BEGIN;
-- Ensure to cache loopback2 connection.
SELECT 1 FROM ft6 LIMIT 1;
?column?
@@ -9155,66 +9139,31 @@ SELECT 1 FROM ft6 LIMIT 1;
-- List all the existing cached connections. loopback and loopback2 should be
-- output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback | t
- loopback2 | t
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name
+-------------
+ loopback
+ loopback2
(2 rows)
--- Issue a warning and return false as loopback2 connection is still in use and
+-- Issue a warning and return false as loopback connection is still in use and
-- can not be closed.
-SELECT postgres_fdw_disconnect('loopback2');
-WARNING: cannot close connection for server "loopback2" because it is still
in use
+SELECT postgres_fdw_disconnect('loopback');
+WARNING: cannot close connection for server "loopback" because it is still in
use
postgres_fdw_disconnect
-------------------------
f
(1 row)
--- Close loopback connection, return true and issue a warning as loopback2
--- connection is still in use and can not be closed.
-SELECT postgres_fdw_disconnect_all();
-WARNING: cannot close connection for server "loopback2" because it is still
in use
- postgres_fdw_disconnect_all
------------------------------
- t
-(1 row)
+-- 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
+(2 rows)
--- List all the existing cached connections. loopback2 should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback2 | t
-(1 row)
-
--- Ensure to cache loopback connection.
-SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
- 1
-(1 row)
-
--- Ensure to cache loopback4 connection.
-SELECT 1 FROM ft8 LIMIT 1;
- ?column?
-----------
- 1
-(1 row)
-
--- List all the existing cached connections. loopback, loopback2, loopback4
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback | t
- loopback2 | t
- loopback4 | t
-(3 rows)
-
-DROP SERVER loopback4 CASCADE;
-NOTICE: drop cascades to 2 other objects
-DETAIL: drop cascades to user mapping for public on server loopback4
-drop cascades to foreign table ft8
-- 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';
@@ -9226,21 +9175,19 @@ SELECT postgres_fdw_disconnect_all();
RESET client_min_messages;
COMMIT;
--- Close loopback2 connection and return true.
-SELECT postgres_fdw_disconnect('loopback2');
- postgres_fdw_disconnect
--------------------------
- t
+-- Ensure that loopback2 connection is closed.
+SELECT 1 FROM postgres_fdw_disconnect('loopback2');
+ ?column?
+----------
+ 1
(1 row)
--- List all the existing cached connections. loopback should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback | t
-(1 row)
+SELECT server_name FROM postgres_fdw_get_connections() WHERE server_name =
'loopback2';
+ server_name
+-------------
+(0 rows)
--- Return false as loopback2 connectin is closed already.
+-- Return false as loopback2 connection is closed already.
SELECT postgres_fdw_disconnect('loopback2');
postgres_fdw_disconnect
-------------------------
@@ -9250,18 +9197,17 @@ SELECT postgres_fdw_disconnect('loopback2');
-- Return an error as there is no foreign server with given name.
SELECT postgres_fdw_disconnect('unknownserver');
ERROR: server "unknownserver" does not exist
--- Close loopback connection and return true.
-SELECT postgres_fdw_disconnect_all();
- postgres_fdw_disconnect_all
------------------------------
- t
+-- Let's ensure to close all the existing cached connections.
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+----------
+ 1
(1 row)
--- List all the existing cached connections. No connection exists, so NULL
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
+-- No cached connections, so no records should be output.
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name
+-------------
(0 rows)
--
=============================================================================
@@ -9271,6 +9217,7 @@ CREATE ROLE regress_multi_conn_user1 SUPERUSER;
CREATE ROLE regress_multi_conn_user2 SUPERUSER;
CREATE USER MAPPING FOR regress_multi_conn_user1 SERVER loopback;
CREATE USER MAPPING FOR regress_multi_conn_user2 SERVER loopback;
+BEGIN;
-- Will cache loopback connection with user mapping for
regress_multi_conn_user1
SET ROLE regress_multi_conn_user1;
SELECT 1 FROM ft1 LIMIT 1;
@@ -9290,25 +9237,25 @@ SELECT 1 FROM ft1 LIMIT 1;
RESET ROLE;
-- Should output two connections for loopback server
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
- loopback | t
- loopback | t
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name
+-------------
+ loopback
+ loopback
(2 rows)
--- Close loopback connections and return true.
-SELECT postgres_fdw_disconnect('loopback');
- postgres_fdw_disconnect
--------------------------
- t
+COMMIT;
+-- Let's ensure to close all the existing cached connections.
+SELECT 1 FROM postgres_fdw_disconnect_all();
+ ?column?
+----------
+ 1
(1 row)
--- List all the existing cached connections. No connection exists, so NULL
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
- server_name | valid
--------------+-------
+-- No cached connections, so no records should be output.
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
+ server_name
+-------------
(0 rows)
-- Clean up
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql
b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 647192cf6a..319c15d635 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -19,10 +19,6 @@ DO $d$
OPTIONS (dbname '$$||current_database()||$$',
port '$$||current_setting('port')||$$'
)$$;
- EXECUTE $$CREATE SERVER loopback4 FOREIGN DATA WRAPPER postgres_fdw
- OPTIONS (dbname '$$||current_database()||$$',
- port '$$||current_setting('port')||$$'
- )$$;
END;
$d$;
@@ -31,7 +27,6 @@ CREATE USER MAPPING FOR public SERVER testserver1
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback2;
CREATE USER MAPPING FOR public SERVER loopback3;
-CREATE USER MAPPING FOR public SERVER loopback4;
-- ===================================================================
-- create objects used through FDW loopback server
@@ -158,12 +153,6 @@ CREATE FOREIGN TABLE ft7 (
c3 text
) SERVER loopback3 OPTIONS (schema_name 'S 1', table_name 'T 4');
-CREATE FOREIGN TABLE ft8 (
- c1 int NOT NULL,
- c2 int NOT NULL,
- c3 text
-) SERVER loopback4 OPTIONS (schema_name 'S 1', table_name 'T 4');
-
-- ===================================================================
-- tests for validator
-- ===================================================================
@@ -2723,80 +2712,67 @@ DROP PROCEDURE terminate_backend_and_wait(text);
--
=============================================================================
-- 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;
-- This test case is for closing the connection in pgfdw_xact_callback
BEGIN;
--- List all the existing cached connections. Only loopback2 should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
-- Connection xact depth becomes 1 i.e. the connection is in midst of the xact.
SELECT 1 FROM ft1 LIMIT 1;
SELECT 1 FROM ft7 LIMIT 1;
--- List all the existing cached connections. loopback and loopback3
--- also should be output as valid connections.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 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.
+-- List all the existing cached connections. loopback and loopback3 should be
+-- output.
+SELECT server_name 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.
ALTER SERVER loopback OPTIONS (ADD use_remote_estimate 'off');
DROP SERVER loopback3 CASCADE;
-- List all the existing cached connections. loopback and loopback3
-- should be output as invalid connections. Also the server name for
-- loopback3 should be NULL because the server was dropped.
SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
--- The invalid connection gets closed in pgfdw_xact_callback during commit.
+-- The invalid connections get closed in pgfdw_xact_callback during commit.
COMMIT;
--- List all the existing cached connections. loopback and loopback3
--- should not be output because they should be closed at the end of
--- the above transaction.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- 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;
-- =======================================================================
-- test postgres_fdw_disconnect and postgres_fdw_disconnect_all functions
-- =======================================================================
--- Return true as all cached connections are closed.
-SELECT postgres_fdw_disconnect_all();
+BEGIN;
-- Ensure to cache loopback connection.
SELECT 1 FROM ft1 LIMIT 1;
-BEGIN;
-- Ensure to cache loopback2 connection.
SELECT 1 FROM ft6 LIMIT 1;
-- List all the existing cached connections. loopback and loopback2 should be
-- output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
--- Issue a warning and return false as loopback2 connection is still in use and
+SELECT server_name 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('loopback2');
--- Close loopback connection, return true and issue a warning as loopback2
--- connection is still in use and can not be closed.
-SELECT postgres_fdw_disconnect_all();
--- List all the existing cached connections. loopback2 should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
--- Ensure to cache loopback connection.
-SELECT 1 FROM ft1 LIMIT 1;
--- Ensure to cache loopback4 connection.
-SELECT 1 FROM ft8 LIMIT 1;
--- List all the existing cached connections. loopback, loopback2, loopback4
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
-DROP SERVER loopback4 CASCADE;
+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;
-- 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';
SELECT postgres_fdw_disconnect_all();
RESET client_min_messages;
COMMIT;
--- Close loopback2 connection and return true.
-SELECT postgres_fdw_disconnect('loopback2');
--- List all the existing cached connections. loopback should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
--- Return false as loopback2 connectin is closed already.
+-- 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';
+-- 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');
--- Close loopback connection and return true.
-SELECT postgres_fdw_disconnect_all();
--- List all the existing cached connections. No connection exists, so NULL
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+-- 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;
--
=============================================================================
-- test case for having multiple cached connections for a foreign server
@@ -2806,6 +2782,7 @@ CREATE ROLE regress_multi_conn_user2 SUPERUSER;
CREATE USER MAPPING FOR regress_multi_conn_user1 SERVER loopback;
CREATE USER MAPPING FOR regress_multi_conn_user2 SERVER loopback;
+BEGIN;
-- Will cache loopback connection with user mapping for
regress_multi_conn_user1
SET ROLE regress_multi_conn_user1;
SELECT 1 FROM ft1 LIMIT 1;
@@ -2817,14 +2794,12 @@ SELECT 1 FROM ft1 LIMIT 1;
RESET ROLE;
-- Should output two connections for loopback server
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
-
--- Close loopback connections and return true.
-SELECT postgres_fdw_disconnect('loopback');
-
--- List all the existing cached connections. No connection exists, so NULL
--- should be output.
-SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
+SELECT server_name 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();
+-- No cached connections, so no records should be output.
+SELECT server_name FROM postgres_fdw_get_connections() ORDER BY 1;
-- Clean up
DROP USER MAPPING FOR regress_multi_conn_user1 SERVER loopback;
