Re: tests against running server occasionally fail, postgres_fdw & tenk1

Andres Freund <andres@anarazel.de> writes:
> Hm, yea, that should work. It's indeed the entirety of the diff
>
https://api.cirrus-ci.com/v1/artifact/task/4718859714822144/testrun/build/testrun/postgres_fdw-running/regress/regression.diffs

> If we go that way we can remove the debug_discard muckery as well, I think?

Okay, so that seems to work for the "reestablish new connection" test:
as coded here, it passes with or without debug_discard_caches enabled,
and I believe it's testing what it intends to either way.  So that's
good.

However, the other stanza with debug_discard_caches muckery is the
one about "test postgres_fdw.application_name GUC", and in that case
ignoring the number of terminated connections would destroy the
point of the test entirely; because without that, you're proving
nothing about what the remote's application_name actually looks like.

I'm inclined to think we should indeed just nuke that test.  It's
overcomplicated and it expends a lot of test cycles on a pretty
marginal feature.

So I propose the attached.

            regards, tom lane

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index d5fc61446a..d0ba40aca3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9926,11 +9926,6 @@ WARNING:  there is no transaction in progress
 -- 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_discard_caches 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_discard_caches = 0;
 -- Make sure we have a remote connection.
 SELECT 1 FROM ft1 LIMIT 1;
  ?column?
@@ -9939,13 +9934,12 @@ SELECT 1 FROM ft1 LIMIT 1;
 (1 row)

 -- Terminate the remote connection and wait for the termination to complete.
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
+-- (If a cache flush happens, the remote connection might have already been
+-- dropped; so code this step in a way that doesn't fail if no connection.)
+DO $$ BEGIN
+PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity
     WHERE application_name = 'fdw_retry_check';
- pg_terminate_backend
-----------------------
- t
-(1 row)
-
+END $$;
 -- This query should detect the broken connection when starting new remote
 -- transaction, reestablish new connection, and then succeed.
 BEGIN;
@@ -9958,13 +9952,10 @@ 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.
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
+DO $$ BEGIN
+PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity
     WHERE application_name = 'fdw_retry_check';
- pg_terminate_backend
-----------------------
- t
-(1 row)
-
+END $$;
 SAVEPOINT s;
 -- The text of the error might vary across platforms, so only show SQLSTATE.
 \set VERBOSITY sqlstate
@@ -9972,7 +9963,6 @@ SELECT 1 FROM ft1 LIMIT 1;    -- should fail
 ERROR:  08006
 \set VERBOSITY default
 COMMIT;
-RESET debug_discard_caches;
 -- =============================================================================
 -- test connection invalidation cases and postgres_fdw_get_connections function
 -- =============================================================================
@@ -11627,77 +11617,6 @@ ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');
 ERROR:  invalid option "nonexistent"
 HINT:  There are no valid options in this context.
 -- ===================================================================
--- test postgres_fdw.application_name GUC
--- ===================================================================
---- Turn debug_discard_caches off for this test to make sure that
---- the remote connection is alive when checking its application_name.
-SET debug_discard_caches = 0;
--- Specify escape sequences in application_name option of a server
--- object so as to test that they are replaced with status information
--- expectedly.
---
--- Since pg_stat_activity.application_name may be truncated to less than
--- NAMEDATALEN characters, note that substring() needs to be used
--- at the condition of test query to make sure that the string consisting
--- of database name and process ID is also less than that.
-ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
-SELECT 1 FROM ft6 LIMIT 1;
- ?column?
-----------
-        1
-(1 row)
-
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_database() || pg_backend_pid() for
-      current_setting('max_identifier_length')::int);
- pg_terminate_backend
-----------------------
- t
-(1 row)
-
--- postgres_fdw.application_name overrides application_name option
--- of a server object if both settings are present.
-SET postgres_fdw.application_name TO 'fdw_%a%u%%';
-SELECT 1 FROM ft6 LIMIT 1;
- ?column?
-----------
-        1
-(1 row)
-
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_setting('application_name') ||
-      CURRENT_USER || '%' for current_setting('max_identifier_length')::int);
- pg_terminate_backend
-----------------------
- t
-(1 row)
-
--- Test %c (session ID) and %C (cluster name) escape sequences.
-SET postgres_fdw.application_name TO 'fdw_%C%c';
-SELECT 1 FROM ft6 LIMIT 1;
- ?column?
-----------
-        1
-(1 row)
-
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_setting('cluster_name') ||
-      to_hex(trunc(EXTRACT(EPOCH FROM (SELECT backend_start FROM
-      pg_stat_get_activity(pg_backend_pid()))))::integer) || '.' ||
-      to_hex(pg_backend_pid())
-      for current_setting('max_identifier_length')::int);
- pg_terminate_backend
-----------------------
- t
-(1 row)
-
---Clean up
-RESET postgres_fdw.application_name;
-RESET debug_discard_caches;
--- ===================================================================
 -- test parallel commit
 -- ===================================================================
 ALTER SERVER loopback OPTIONS (ADD parallel_commit 'true');
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 1e50be137b..50e0867341 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3100,18 +3100,16 @@ ROLLBACK;
 -- so that we can easily terminate the connection later.
 ALTER SERVER loopback OPTIONS (application_name 'fdw_retry_check');

--- If debug_discard_caches 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_discard_caches = 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
+-- (If a cache flush happens, the remote connection might have already been
+-- dropped; so code this step in a way that doesn't fail if no connection.)
+DO $$ BEGIN
+PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity
     WHERE application_name = 'fdw_retry_check';
+END $$;

 -- This query should detect the broken connection when starting new remote
 -- transaction, reestablish new connection, and then succeed.
@@ -3121,8 +3119,10 @@ 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.
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
+DO $$ BEGIN
+PERFORM pg_terminate_backend(pid, 180000) FROM pg_stat_activity
     WHERE application_name = 'fdw_retry_check';
+END $$;
 SAVEPOINT s;
 -- The text of the error might vary across platforms, so only show SQLSTATE.
 \set VERBOSITY sqlstate
@@ -3130,8 +3130,6 @@ SELECT 1 FROM ft1 LIMIT 1;    -- should fail
 \set VERBOSITY default
 COMMIT;

-RESET debug_discard_caches;
-
 -- =============================================================================
 -- test connection invalidation cases and postgres_fdw_get_connections function
 -- =============================================================================
@@ -3847,52 +3845,6 @@ CREATE FOREIGN TABLE inv_bsz (c1 int )
 -- No option is allowed to be specified at foreign data wrapper level
 ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (nonexistent 'fdw');

--- ===================================================================
--- test postgres_fdw.application_name GUC
--- ===================================================================
---- Turn debug_discard_caches off for this test to make sure that
---- the remote connection is alive when checking its application_name.
-SET debug_discard_caches = 0;
-
--- Specify escape sequences in application_name option of a server
--- object so as to test that they are replaced with status information
--- expectedly.
---
--- Since pg_stat_activity.application_name may be truncated to less than
--- NAMEDATALEN characters, note that substring() needs to be used
--- at the condition of test query to make sure that the string consisting
--- of database name and process ID is also less than that.
-ALTER SERVER loopback2 OPTIONS (application_name 'fdw_%d%p');
-SELECT 1 FROM ft6 LIMIT 1;
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_database() || pg_backend_pid() for
-      current_setting('max_identifier_length')::int);
-
--- postgres_fdw.application_name overrides application_name option
--- of a server object if both settings are present.
-SET postgres_fdw.application_name TO 'fdw_%a%u%%';
-SELECT 1 FROM ft6 LIMIT 1;
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_setting('application_name') ||
-      CURRENT_USER || '%' for current_setting('max_identifier_length')::int);
-
--- Test %c (session ID) and %C (cluster name) escape sequences.
-SET postgres_fdw.application_name TO 'fdw_%C%c';
-SELECT 1 FROM ft6 LIMIT 1;
-SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
-  WHERE application_name =
-    substring('fdw_' || current_setting('cluster_name') ||
-      to_hex(trunc(EXTRACT(EPOCH FROM (SELECT backend_start FROM
-      pg_stat_get_activity(pg_backend_pid()))))::integer) || '.' ||
-      to_hex(pg_backend_pid())
-      for current_setting('max_identifier_length')::int);
-
---Clean up
-RESET postgres_fdw.application_name;
-RESET debug_discard_caches;
-
 -- ===================================================================
 -- test parallel commit
 -- ===================================================================