Thanks for the review.
On Thu, Dec 3, 2020 at 7:24 AM Hou, Zhijie <houzj.f...@cn.fujitsu.com> wrote:
>
> 1.
> +
> + ereport(WARNING,
> + (errmsg("could not wait for the termination of the
> backend with PID %d within %ld milliseconds",
> + pid, timeout)));
> +
>
> The code use %ld to print int64 type.
> How about use INT64_FORMAT, which looks more appropriate.
>
Changed it to use %lld and typecasting timeout to (long long int) as
suggested by Tom.
>
> 2.
> + if (timeout <= 0)
> + {
> + ereport(WARNING,
> + (errmsg("timeout cannot be negative or zero:
> %ld", timeout)));
> + PG_RETURN_BOOL(r);
> + }
>
> The same as 1.
>
Changed.
>
> 3.
> +pg_terminate_backend_and_wait(PG_FUNCTION_ARGS)
> +{
> + int pid = PG_GETARG_DATUM(0);
>
> +pg_wait_backend(PG_FUNCTION_ARGS)
> +{
> + int pid = PG_GETARG_INT32(0);
>
> The code use different macro to get pid,
> How about use PG_GETARG_INT32(0) for each one.
>
Changed.
> I am Sorry I forgot a possible typo comment.
>
> +{ oid => '16386', descr => 'terminate a backend process and wait for it\'s
> exit or until timeout occurs'
>
> Does the following change looks better?
>
> Wait for it\'s exit => Wait for its exit
>
Changed.
>
> I changed the status to 'wait on anthor'.
> The others of the patch LGTM,
> I think it can be changed to Ready for Committer again, when this comment is
> confirmed.
>
Attaching v4 patch. Please have a look.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From e3f364e8a5a06b3e122b657e668146f220549acb Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Thu, 3 Dec 2020 09:09:13 +0530
Subject: [PATCH v4] pg_terminate_backend with wait, timeout and
pg_wait_backend with timeout
This patch adds two new functions:
1. pg_terminate_backend(pid, wait, timeout) - terminate and
wait or timeout for a given backend.
2. pg_wait_backend(pid, timeout) - check the existence of the
backend with a given PID and wait or timeout until it goes away.
---
doc/src/sgml/func.sgml | 28 +++++-
src/backend/catalog/system_views.sql | 10 ++
src/backend/postmaster/pgstat.c | 3 +
src/backend/storage/ipc/signalfuncs.c | 137 ++++++++++++++++++++++++++
src/include/catalog/pg_proc.dat | 9 ++
src/include/pgstat.h | 3 +-
6 files changed, 187 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index df29af6371..6b77b80336 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -23954,7 +23954,7 @@ SELECT collation for ('foo' COLLATE "de_DE");
<indexterm>
<primary>pg_terminate_backend</primary>
</indexterm>
- <function>pg_terminate_backend</function> ( <parameter>pid</parameter> <type>integer</type> )
+ <function>pg_terminate_backend</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>wait</parameter> <type>boolean</type>, <parameter>timeout</parameter> <type>bigint</type> <literal>DEFAULT</literal> <literal>100</literal> )
<returnvalue>boolean</returnvalue>
</para>
<para>
@@ -23962,7 +23962,31 @@ SELECT collation for ('foo' COLLATE "de_DE");
specified process ID. This is also allowed if the calling role
is a member of the role whose backend is being terminated or the
calling role has been granted <literal>pg_signal_backend</literal>,
- however only superusers can terminate superuser backends.
+ however only superusers can terminate superuser backends. When no
+ <parameter>wait</parameter> and <parameter>timeout</parameter> are
+ provided, only SIGTERM is sent to the backend with the given process
+ ID and <literal>false</literal> is returned immediately. But the
+ backend may still exist. With <parameter>wait</parameter> specified as
+ <literal>true</literal>, the function waits for the backend termination
+ until the given <parameter>timeout</parameter> or the default 100
+ milliseconds. On timeout <literal>false</literal> is returned.
+ </para></entry>
+ </row>
+
+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>pg_wait_backend</primary>
+ </indexterm>
+ <function>pg_wait_backend</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>timeout</parameter> <type>bigint</type> <literal>DEFAULT</literal> <literal>100</literal> )
+ <returnvalue>boolean</returnvalue>
+ </para>
+ <para>
+ Check the existence of the session whose backend process has the
+ specified process ID. On success return <literal>true</literal>. This
+ function waits until the given <parameter>timeout</parameter> or the
+ default 100 milliseconds. On timeout <literal>false</literal> is
+ returned.
</para></entry>
</row>
</tbody>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index b140c210bc..2d1907b4a8 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1246,6 +1246,16 @@ CREATE OR REPLACE FUNCTION
RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_promote'
PARALLEL SAFE;
+CREATE OR REPLACE FUNCTION
+ pg_terminate_backend(pid integer, wait boolean, timeout int8 DEFAULT 100)
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend_and_wait'
+ PARALLEL UNSAFE;
+
+CREATE OR REPLACE FUNCTION
+ pg_wait_backend(pid integer, timeout int8 DEFAULT 100)
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_wait_backend'
+ PARALLEL UNSAFE;
+
-- legacy definition for compatibility with 9.3
CREATE OR REPLACE FUNCTION
json_populate_record(base anyelement, from_json json, use_json_as_text boolean DEFAULT false)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 9bad14981b..cf4d9f8730 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4040,6 +4040,9 @@ pgstat_get_wait_ipc(WaitEventIPC w)
case WAIT_EVENT_XACT_GROUP_UPDATE:
event_name = "XactGroupUpdate";
break;
+ case WAIT_EVENT_BACKEND_TERMINATION:
+ event_name = "BackendTermination";
+ break;
/* no default case, so that compiler will warn */
}
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index d822e82cb9..1d5f001efc 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -18,6 +18,7 @@
#include "catalog/pg_authid.h"
#include "miscadmin.h"
+#include "pgstat.h"
#include "postmaster/syslogger.h"
#include "storage/pmsignal.h"
#include "storage/proc.h"
@@ -149,6 +150,142 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS);
}
+/*
+ * Wait until there is no backend process with the given PID or timeout. On
+ * timeout, a warning is thrown to the user.
+ */
+static bool
+pg_wait_until_termination(int pid, int64 timeout)
+{
+ /*
+ * Wait in steps of waittime milliseconds until this function exits or
+ * timeout.
+ */
+ int64 waittime = 10;
+ /*
+ * Initially remaining time is the entire timeout specified by the user.
+ */
+ int64 remainingtime = timeout;
+
+ /*
+ * Check the backend existence. If doesn't exist, then check for the errno
+ * ESRCH that confirms it and return true. If still exist, then wait for
+ * waittime milliseconds, again check for the existence. Repeat until
+ * timeout or an error occurs or a pending interrupt such as query cancel
+ * is processed.
+ */
+ for (;;)
+ {
+ if (remainingtime < waittime)
+ waittime = remainingtime;
+
+ if (kill(pid, 0) == -1)
+ {
+ if (errno == ESRCH)
+ return true;
+ else
+ {
+ ereport(WARNING,
+ (errmsg("could not check the existence of the backend with PID %d: %m",
+ pid)));
+ return false;
+ }
+ }
+
+ /* Process interrupts, if any, before going for wait. */
+ CHECK_FOR_INTERRUPTS();
+
+ (void) WaitLatch(MyLatch,
+ WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
+ waittime,
+ WAIT_EVENT_BACKEND_TERMINATION);
+
+ remainingtime -= waittime;
+
+ if (remainingtime <= 0)
+ break;
+ }
+
+ ereport(WARNING,
+ (errmsg("could not wait for the termination of the backend with PID %d within %lld milliseconds",
+ pid, (long long int) timeout)));
+
+ return false;
+}
+
+/*
+ * Signal termination of a backend process, wait until timeout or no backend
+ * has the given PID. If the wait input argument is false, this function
+ * behaviour is same as pg_terminate_backend. On timeout, a warning is thrown
+ * to the user. Self termination is allowed but waiting is not, because once
+ * the backend is self terminated there is no point in waiting for it to go
+ * away.
+ */
+Datum
+pg_terminate_backend_and_wait(PG_FUNCTION_ARGS)
+{
+ int pid = PG_GETARG_INT32(0);
+ bool wait = PG_GETARG_BOOL(1);
+ int64 timeout = PG_GETARG_INT64(2);
+ bool r = false;
+
+ if (timeout < 1)
+ {
+ ereport(WARNING,
+ (errmsg("timeout cannot be negative or zero: %lld",
+ (long long int) timeout)));
+ PG_RETURN_BOOL(r);
+ }
+
+ /* Self termination is allowed but waiting is not after that. */
+ if (pid == MyProcPid && wait)
+ wait = false;
+
+ r = DatumGetBool(DirectFunctionCall1(pg_terminate_backend, pid));
+
+ /* Wait only if requested and the termination is successful. */
+ if (wait && r)
+ r = pg_wait_until_termination(pid, timeout);
+
+ PG_RETURN_BOOL(r);
+}
+
+/*
+ * This function waits for a backend with the given PID to be terminated or
+ * until the timeout occurs. If the PID is of the backend which issued this
+ * function, either timeout can occur or the function can succeed if the
+ * backend gets terminated by some other process, say postmaster.
+ */
+Datum
+pg_wait_backend(PG_FUNCTION_ARGS)
+{
+ int pid = PG_GETARG_INT32(0);
+ int64 timeout = PG_GETARG_INT64(1);
+ PGPROC *proc = NULL;
+ bool r = false;
+
+ if (timeout < 1)
+ {
+ ereport(WARNING,
+ (errmsg("timeout cannot be negative or zero: %lld",
+ (long long int) timeout)));
+ PG_RETURN_BOOL(r);
+ }
+
+ proc = BackendPidGetProc(pid);
+
+ if (proc == NULL)
+ {
+ ereport(WARNING,
+ (errmsg("PID %d is not a PostgreSQL server process", pid)));
+ PG_RETURN_BOOL(r);
+ }
+
+ r = pg_wait_until_termination(pid, timeout);
+
+ PG_RETURN_BOOL(r);
+}
+
/*
* Signal to reload the database configuration
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index fc2202b843..f77eadaa54 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6094,6 +6094,15 @@
{ oid => '2096', descr => 'terminate a server process',
proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
proargtypes => 'int4', prosrc => 'pg_terminate_backend' },
+{ oid => '16386', descr => 'terminate a backend process and wait for its exit or until timeout occurs',
+ proname => 'pg_terminate_backend', provolatile => 'v',
+ prorettype => 'bool', proargtypes => 'int4 bool int8',
+ proargnames => '{pid,wait,timeout}',
+ prosrc => 'pg_terminate_backend_and_wait' },
+{ oid => '16387', descr => 'waits for a backend process exit or timeout occurs',
+ proname => 'pg_wait_backend', provolatile => 'v', prorettype => 'bool',
+ proargtypes => 'int4 int8', proargnames => '{pid,timeout}',
+ prosrc => 'pg_wait_backend' },
{ oid => '2172', descr => 'prepare for taking an online backup',
proname => 'pg_start_backup', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => 'text bool bool',
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 5954068dec..c9f5668509 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -964,7 +964,8 @@ typedef enum
WAIT_EVENT_REPLICATION_SLOT_DROP,
WAIT_EVENT_SAFE_SNAPSHOT,
WAIT_EVENT_SYNC_REP,
- WAIT_EVENT_XACT_GROUP_UPDATE
+ WAIT_EVENT_XACT_GROUP_UPDATE,
+ WAIT_EVENT_BACKEND_TERMINATION
} WaitEventIPC;
/* ----------
--
2.25.1
