On Thu, Apr 8, 2021 at 6:27 PM Bharath Rupireddy
<bharath.rupireddyforpostg...@gmail.com> wrote:
>
> On Thu, Apr 8, 2021 at 5:28 PM Michael Paquier <mich...@paquier.xyz> wrote:
> >
> > On Thu, Apr 08, 2021 at 04:55:22PM +0530, Bharath Rupireddy wrote:
> > > With the recent commit aaf0432572 which introduced a waiting/timeout
> > > capability for pg_teriminate_backend function, I would like to do
> > > $subject. Attaching a patch, please have a look.
> >
> > +-- Terminate the remote backend having the specified application_name and
> > wait
> > +-- for the termination to complete. 10 seconds timeout here is chosen
> > randomly,
> > +-- we will see a warning if the process doesn't go away within that time.
> > +SELECT pg_terminate_backend(pid, 10000) FROM pg_stat_activity
> > + WHERE application_name = 'fdw_retry_check';
> >
> > I think that you are making the tests less stable by doing that. A
> > couple of buildfarm machines are very slow, and 10 seconds would not
> > be enough. So it seems to me that this patch is trading what is a
> > stable solution for a solution that may finish by randomly bite.
>
> Agree. Please see the attached patch, I removed a fixed waiting time.
> Instead of relying on pg_stat_activity, pg_sleep and
> pg_stat_clear_snapshot, now it depends on pg_terminate_backend and
> pg_wait_for_backend_termination. This way we could reduce the
> functions that the procedure terminate_backend_and_wait uses and also
> the new functions pg_terminate_backend and
> pg_wait_for_backend_termination gets a test coverage.
>
> Thoughts?
I realized that the usage like below in v2 patch is completely wrong,
because pg_terminate_backend without timeout will return true and the
loop exits without calling pg_wait_for_backend_terminatioin. Sorry for
not realizing this earlier.
SELECT * INTO is_terminated FROM pg_terminate_backend(pid_v);
LOOP
EXIT WHEN is_terminated;
SELECT * INTO is_terminated FROM
pg_wait_for_backend_termination(pid_v, 1000);
END LOOP;
I feel that we can provide a high timeout value (It can be 1hr on the
similar lines of using pg_sleep(3600) for crash tests in
013_crash_restart.pl with the assumption that the backend running that
command will get killed with SIGQUIT) and make pg_terminate_backend
wait. This unreasonably high timeout looks okay because of the
assumption that the servers in the build farm will not take that much
time to remove the backend from the system processes, so the function
will return much earlier than that. If at all there's a server(which
is impractical IMO) that doesn't remove the backend process even
within 1hr, then that is anyways will fail with the warning.
With the attached patch, we could remove the procedure
terminate_backend_and_wait altogether. Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 89495d7f87132af5128fc3a4bc941a11bb558594 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 9 Apr 2021 16:23:46 +0530
Subject: [PATCH v3] Simplify backend terminate and wait logic in postgres_fdw
test
With the recent commit aaf0432572 which introduced a waiting
capability for pg_teriminate_backend function, we can simply
backend terminate and wait logic in postgres_fdw.sql tests.
---
.../postgres_fdw/expected/postgres_fdw.out | 36 +++++++++----------
contrib/postgres_fdw/sql/postgres_fdw.sql | 29 +++++----------
2 files changed, 26 insertions(+), 39 deletions(-)
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 7f69fa0054..e45d1a30f5 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9195,19 +9195,6 @@ WARNING: there is no transaction in progress
-- ===================================================================
-- reestablish new connection
-- ===================================================================
--- Terminate the backend having the specified application_name and wait for
--- the termination to complete.
-CREATE OR REPLACE PROCEDURE terminate_backend_and_wait(appname text) AS $$
-BEGIN
- PERFORM pg_terminate_backend(pid) FROM pg_stat_activity
- WHERE application_name = appname;
- LOOP
- PERFORM * FROM pg_stat_activity WHERE application_name = appname;
- EXIT WHEN NOT FOUND;
- PERFORM pg_sleep(1), pg_stat_clear_snapshot();
- END LOOP;
-END;
-$$ LANGUAGE plpgsql;
-- 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');
@@ -9217,8 +9204,15 @@ SELECT 1 FROM ft1 LIMIT 1;
1
(1 row)
--- Terminate the remote connection.
-CALL terminate_backend_and_wait('fdw_retry_check');
+-- Terminate the remote connection and wait for its exit from the system
+-- processes.
+SELECT pg_terminate_backend(pid, 3600000) FROM pg_stat_activity
+ WHERE application_name = 'fdw_retry_check';
+ pg_terminate_backend
+----------------------
+ t
+(1 row)
+
-- This query should detect the broken connection when starting new remote
-- transaction, reestablish new connection, and then succeed.
BEGIN;
@@ -9231,15 +9225,21 @@ SELECT 1 FROM ft1 LIMIT 1;
-- If the query detects the broken connection when starting new remote
-- subtransaction, it doesn't reestablish new connection and should fail.
-- The text of the error might vary across platforms, so don't show it.
-CALL terminate_backend_and_wait('fdw_retry_check');
+-- Terminate the remote connection and wait for its exit from the system
+-- processes.
+SELECT pg_terminate_backend(pid, 3600000) FROM pg_stat_activity
+ WHERE application_name = 'fdw_retry_check';
+ pg_terminate_backend
+----------------------
+ t
+(1 row)
+
SAVEPOINT s;
\set VERBOSITY sqlstate
SELECT 1 FROM ft1 LIMIT 1; -- should fail
ERROR: 08006
\set VERBOSITY default
COMMIT;
--- Clean up
-DROP PROCEDURE terminate_backend_and_wait(text);
-- =============================================================================
-- test connection invalidation cases and postgres_fdw_get_connections function
-- =============================================================================
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 7487096eac..c41f434c1f 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2785,28 +2785,15 @@ ROLLBACK;
-- ===================================================================
-- reestablish new connection
-- ===================================================================
-
--- Terminate the backend having the specified application_name and wait for
--- the termination to complete.
-CREATE OR REPLACE PROCEDURE terminate_backend_and_wait(appname text) AS $$
-BEGIN
- PERFORM pg_terminate_backend(pid) FROM pg_stat_activity
- WHERE application_name = appname;
- LOOP
- PERFORM * FROM pg_stat_activity WHERE application_name = appname;
- EXIT WHEN NOT FOUND;
- PERFORM pg_sleep(1), pg_stat_clear_snapshot();
- END LOOP;
-END;
-$$ LANGUAGE plpgsql;
-
-- 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');
SELECT 1 FROM ft1 LIMIT 1;
--- Terminate the remote connection.
-CALL terminate_backend_and_wait('fdw_retry_check');
+-- Terminate the remote connection and wait for its exit from the system
+-- processes.
+SELECT pg_terminate_backend(pid, 3600000) 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.
@@ -2816,16 +2803,16 @@ SELECT 1 FROM ft1 LIMIT 1;
-- If the query detects the broken connection when starting new remote
-- subtransaction, it doesn't reestablish new connection and should fail.
-- The text of the error might vary across platforms, so don't show it.
-CALL terminate_backend_and_wait('fdw_retry_check');
+-- Terminate the remote connection and wait for its exit from the system
+-- processes.
+SELECT pg_terminate_backend(pid, 3600000) FROM pg_stat_activity
+ WHERE application_name = 'fdw_retry_check';
SAVEPOINT s;
\set VERBOSITY sqlstate
SELECT 1 FROM ft1 LIMIT 1; -- should fail
\set VERBOSITY default
COMMIT;
--- Clean up
-DROP PROCEDURE terminate_backend_and_wait(text);
-
-- =============================================================================
-- test connection invalidation cases and postgres_fdw_get_connections function
-- =============================================================================
--
2.25.1
