Subject: Re: Allow non-superuser to cancel superuser tasks.

From: Michael Paquier <[email-masked]>
Date: Wed, 10 Apr 2024 07:58:39 +0900

On Wed, Apr 10, 2024 at 12:52:19AM +0300, Kirill Reshke wrote:
> On Tue, 9 Apr 2024 at 08:53, Michael Paquier <[email-masked]> wrote:
>> The thing is that you cannot rely on a lookup of the backend type for
>> the error information, or you open yourself to letting the caller of
>> pg_cancel_backend or pg_terminate_backend know if a backend is
>> controlled by a superuser or if a backend is an autovacuum worker.
> 
> Good catch. Thanks.  I think we need to update the error message to not
> leak backend type info.

Yep, that's necessary I am afraid.

>> The choice of pg_signal_autovacuum is a bit inconsistent, as well,
>> because autovacuum workers operate like regular backends.  This name
>> can also be confused with the autovacuum launcher.
> 
> Ok. What would be a good choice? Is `pg_signal_autovacuum_worker` good
> enough?

Sounds fine to me.  Perhaps others have an opinion about that?
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 10 Apr 2024 10:00:34 -0500

> Yep, that's necessary I am afraid.

Isn't it relatively easy to discover this same information today via
pg_stat_progress_vacuum?  That has the following code:

		/* Value available to all callers */
		values[0] = Int32GetDatum(beentry->st_procpid);
		values[1] = ObjectIdGetDatum(beentry->st_databaseid);

I guess I'm not quite following why we are worried about leaking whether a
backend is an autovacuum worker.

> Sounds fine to me.  Perhaps others have an opinion about that?

WFM

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Thu, 11 Apr 2024 07:21:36 +0900

> backend is an autovacuum worker.

Good point.  I've missed that we make no effort currently to hide any
PID information from the progress tables.  And we can guess more
context data because of the per-table split of the progress tables.

This choice comes down to b6fb6471f6af that has introduced the
progress report facility, so this ship has long sailed it seems.  And
it makes my argument kind of moot.
--
Michael

---

From: Kirill Reshke <[email-masked]>
Date: Thu, 11 Apr 2024 16:55:59 +0500

Posting updated version of this patch with comments above addressed.

1) pg_signal_autovacuum -> pg_signal_autovacuum_worker, as there seems
to be no objections to that.

2)
There are comments on how to write if statement:

> In core, do_autovacuum() is only called in a process without
> a role specified

> It's feeling more natural here to check that we have a superuser-owned
> backend first, and then do a lookup of the process type.

> I figured since there's no reason to rely on that behavior, we might as
> well do a bit of future-proofing in case autovacuum workers are ever not
> run as InvalidOid.

I have combined them into this:

if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
&& pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER)

This is both future-proofing and natural, I suppose. Downside of this
is double checking condition (!OidIsValid(proc->roleId) ||
superuser_arg(proc->roleId)), but i think that is ok for the sake of
simplicity.

3) pg_signal_autovacuum_worker Oid changed to random one: 8916

4)

> An invalid BackendType is not false, but B_INVALID.
fixed, thanks

5)

>>>> There is pg_read_all_stats as well, so I don't see a big issue in
>>>> requiring to be a member of this role as well for the sake of what's
>>>> proposing here.
>>>
>>> Well, that tells you quite a bit more than just which PIDs correspond to
>>> autovacuum workers, but maybe that's good enough for now.
>>
>> That may be a good initial compromise, for now.

>Sounds good to me. I will update the documentation.

@Anthony if you feel that documentation update adds much value here,
please do. Given that we know autovacuum worker PIDs from
pg_stat_progress_vacuum, I don't know how to reflect something about
pg_stat_autovac_worker in doc, and if it is worth it.

6)
> + INJECTION_POINT("autovacuum-start");
> Perhaps autovacuum-worker-start is more suited here

fixed, thanks

7)

> +# Copyright (c) 2022-2024, PostgreSQL Global Development Group
> [...]
> +# Copyright (c) 2024-2024, PostgreSQL Global Development Group

> These need to be cleaned up.

> +# Makefile for src/test/recovery
> +#
> +# src/test/recovery/Makefile

> This is incorrect, twice.

Cleaned up, thanks!

8)

> Not sure that there is a huge point in checking after a role that
> holds pg_signal_backend.
Ok. Removed.

Then:

> +like($psql_err, qr/ERROR: permission denied to terminate ...
> Checking only the ERRROR, and not the DETAIL should be sufficient
> here.

After removing the pg_signal_backend test case we have only one place
where errors check is done. So, I think we should keep DETAIL here to
ensure detail is correct (it differs from regular backend case).

9)
> +# Test signaling for pg_signal_autovacuum role.
> This needs a better documentation:

Updated. Hope now the test documentation helps to understand it.

10)

> +ok($terminate_with_pg_signal_av == 0, "Terminating autovacuum worker should succeed with pg_signal_autovacuum role");
> Is that enough for the validation?

Added:
ok($node->log_contains(qr/FATAL: terminating autovacuum process due to
administrator command/, $offset),
"Autovacuum terminates when role is granted with pg_signal_autovacuum_worker");

11) references to `passcheck` extension removed. errors messages rephrased.

12) injection_point_detach added.

13)
> + INSERT INTO tab_int SELECT * FROM generate_series(1, 1000000);
> A good chunk of the test would be spent on that, but you don't need
> that many tuples to trigger an autovacuum worker as the short naptime
> is able to do it. I would recommend to reduce that to a minimum.

+1
Single tuple works.

14)

v3 suffers from segfault:
2024-04-11 11:28:31.116 UTC [147437] 001_signal_autovacuum.pl LOG:
statement: SELECT pg_terminate_backend(147427);
2024-04-11 11:28:31.116 UTC [147427] FATAL:  terminating autovacuum
process due to administrator command
2024-04-11 11:28:31.116 UTC [147410] LOG:  server process (PID 147427)
was terminated by signal 11: Segmentation fault
2024-04-11 11:28:31.116 UTC [147410] LOG:  terminating any other
active server processes
2024-04-11 11:28:31.117 UTC [147410] LOG:  shutting down because
restart_after_crash is off
2024-04-11 11:28:31.121 UTC [147410] LOG:  database system is shut down

The test doesn't fail because pg_terminate_backend actually meets his
point: autovac is killed. But while dying, autovac also receives
segfault. Thats because of injections points:

(gdb) bt
#0  0x000056361c3379ea in tas (lock=0x7fbcb9632224 <error: Cannot
access memory at address 0x7fbcb9632224>) at
../../../../src/include/storage/s_lock.h:228
#1  ConditionVariableCancelSleep () at condition_variable.c:238
#2  0x000056361c337e4b in ConditionVariableBroadcast
(cv=0x7fbcb66f498c) at condition_variable.c:310
#3  0x000056361c330a40 in CleanupProcSignalState (status=<optimized
out>, arg=<optimized out>) at procsignal.c:240
#4  0x000056361c328801 in shmem_exit (code=code@entry=1) at ipc.c:276
#5  0x000056361c3288fc in proc_exit_prepare (code=code@entry=1) at ipc.c:198
#6  0x000056361c3289bf in proc_exit (code=code@entry=1) at ipc.c:111
#7  0x000056361c49ffa8 in errfinish (filename=<optimized out>,
lineno=<optimized out>, funcname=0x56361c654370 <__func__.16>
"ProcessInterrupts") at elog.c:592
#8  0x000056361bf7191e in ProcessInterrupts () at postgres.c:3264
#9  0x000056361c3378d7 in ConditionVariableTimedSleep
(cv=0x7fbcb9632224, timeout=timeout@entry=-1,
wait_event_info=117440513) at condition_variable.c:196
#10 0x000056361c337d0b in ConditionVariableTimedSleep
(wait_event_info=<optimized out>, timeout=-1, cv=<optimized out>) at
condition_variable.c:135
#11 ConditionVariableSleep (cv=<optimized out>,
wait_event_info=<optimized out>) at condition_variable.c:98
#12 0x00000000b96347d0 in ?? ()
#13 0x3a3f1d9baa4f5500 in ?? ()
#14 0x000056361cc6cbd0 in ?? ()
#15 0x000056361ccac300 in ?? ()
#16 0x000056361c62be63 in ?? ()
#17 0x00007fbcb96347d0 in ?? () at injection_points.c:201 from
/home/reshke/postgres/tmp_install/home/reshke/postgres/pgbin/lib/injection_points.so
#18 0x00007fffe4122b10 in ?? ()
#19 0x00007fffe4122b70 in ?? ()
#20 0x0000000000000000 in ?? ()

discovered because of
# Release injection point.
$node->safe_psql('postgres',
"SELECT injection_point_detach('autovacuum-worker-start');");
added

v4 also suffers from that. i will try to fix that.

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Apr 2024 09:06:59 -0500

> Posting updated version of this patch with comments above addressed.

I look for a commitfest entry for this one, but couldn't find it.  Would
you mind either creating one or, if I've somehow missed it, pointing me to
the existing entry?

	https://commitfest.postgresql.org/48/

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 11 Apr 2024 09:38:07 -0500

> superuser_arg(proc->roleId)), but i think that is ok for the sake of

If we want to retain the check, IMO we might as well combine the first two
blocks like Anthony proposed:

	if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
	{
		ProcNumber procNumber = GetNumberFromPGProc(proc);
		PGBackendStatus procStatus = pgstat_get_beentry_by_proc_number(procNumber);

		if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER &&
			!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVAC_WORKER))
			return SIGNAL_BACKEND_NOAUTOVAC;
		else if (!superuser())
			return SIGNAL_BACKEND_NOSUPERUSER;
	}

+      <row>
+       <entry>pg_signal_autovacuum_worker</entry>
+       <entry>Allow signaling autovacuum worker backend to cancel or terminate</entry>
+      </row>

I think we need to be more specific about what pg_cancel_backend() and
pg_terminate_backend() do for autovacuum workers.  The code offers some
clues:

	/*
	 * SIGINT is used to signal canceling the current table's vacuum; SIGTERM
	 * means abort and exit cleanly, and SIGQUIT means abandon ship.
	 */
	pqsignal(SIGINT, StatementCancelHandler);
	pqsignal(SIGTERM, die);

+/* ----------
+ * pgstat_get_backend_type() -
+ *
+ * Return the backend type of the backend for the given proc number.
+ * ----------
+ */
+BackendType
+pgstat_get_backend_type(ProcNumber procNumber)
+{
+	PgBackendStatus *ret;
+
+	ret = pgstat_get_beentry_by_proc_number(procNumber);
+
+	if (!ret)
+		return B_INVALID;
+
+	return ret->st_backendType;
+}

I'm not sure we really need to introduce a new function for this.  I
avoided using it in my example snippet above.  But, maybe it'll come in
handy down the road...

---

From: Kirill Reshke <[email-masked]>
Date: Thu, 11 Apr 2024 20:20:56 +0500

On Thu, 11 Apr 2024 at 19:07, Nathan Bossart <[email-masked]> wrote:
> you mind either creating one

Done: https://commitfest.postgresql.org/48/4922/

---

From: Michael Paquier <[email-masked]>
Date: Fri, 12 Apr 2024 09:10:35 +0900

> v4 also suffers from that. i will try to fix that.

I can see this stack trace as well.  Capturing a bit more than your
own stack, this is crashing in the autovacuum worker while waiting on
a condition variable when processing a ProcessInterrupts().

That may point to a legit bug with condition variables in this
context, actually?  From what I can see, issuing a signal on a backend
process waiting with a condition variable is able to process the
interrupt correctly.
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 12 Apr 2024 11:01:24 +0900 (JST)

At Fri, 12 Apr 2024 09:10:35 +0900, Michael Paquier <[email-masked]> wrote in 
> > #1  ConditionVariableCancelSleep () at condition_variable.c:238
...
> > (cv=0x7fbcb9632224, timeout=timeout@entry=-1,
...
> process waiting with a condition variable is able to process the

ProcSignalInit sets up CleanupProcSignalState to be called via
on_shmem_exit. If the CV is allocated in a dsm segment, shmem_exit
should have detached the region for the CV.  CV cleanup code should be
invoked via before_shmem_exit.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Kirill Reshke <[email-masked]>
Date: Fri, 12 Apr 2024 13:32:42 +0500

> > This is incorrect, twice.

Oh, wait, I did this wrong.

Should i use

(Like in src/test/signals/meson.build &
src/test/signals/t/001_signal_autovacuum.pl)
or

+#
+# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
(Like in src/test/signals/Makefile)

at the beginning of each added file?

---

From: Michael Paquier <[email-masked]>
Date: Mon, 15 Apr 2024 13:47:08 +0900

> at the beginning of each added file?

Assuming that these files are merged in 2024, you could just use:
Copyright (c) 2024, PostgreSQL Global Development Group

See for example slotsync.c introduced recently in commit ddd5f4f54a02.
--
Michael

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 12 Jun 2024 16:04:06 -0500

I adjusted 0001 based on my upthread feedback.

-- 
nathan
>From 118f95346fcf8099ab28d2f9186185171e3b88af Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Wed, 12 Jun 2024 15:38:14 -0500
Subject: [PATCH v5 1/1] Introduce pg_signal_autovacuum_worker.

Since commit 3a9b18b309, roles with privileges of pg_signal_backend
cannot signal autovacuum workers.  Many users treated the ability
to signal autovacuum workers as a feature instead of a bug, so we
are reintroducing it via a new predefined role.  Having privileges
of this new role, named pg_signal_autovacuum_worker, only permits
signaling autovacuum workers.  It does not permit signaling other
types of superuser backends.

Author: Kirill Reshke
Reviewed-by: Anthony Leung, Michael Paquier
Discussion: https://postgr.es/m/CALdSSPhC4GGmbnugHfB9G0%3DfAxjCSug_-rmL9oUh0LTxsyBfsg%40mail.gmail.com
---
 doc/src/sgml/func.sgml                |  8 +++--
 doc/src/sgml/user-manag.sgml          |  5 +++
 src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------
 src/include/catalog/pg_authid.dat     |  5 +++
 4 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 17c44bc338..7d5b7acb68 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28037,7 +28037,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
        <para>
         Cancels the current query of the session whose backend process has the
         specified process ID.  This is also allowed if the
-        calling role is a member of the role whose backend is being canceled or
+        calling role has privileges of the role whose backend is being canceled,
+        the backend process is an autovacuum worker and the calling role has
+        privileges of <literal>pg_signal_autovacuum_worker</literal>, or
         the calling role has privileges of <literal>pg_signal_backend</literal>,
         however only superusers can cancel superuser backends.
        </para></entry>
@@ -28111,7 +28113,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
        <para>
         Terminates the session whose backend process has the
         specified process ID.  This is also allowed if the calling role
-        is a member of the role whose backend is being terminated or the
+        has privileges of the role whose backend is being terminated,
+        the backend process is an autovacuum worker and the calling role has
+        privileges of <literal>pg_signal_autovacuum_worker</literal>, or the
         calling role has privileges of <literal>pg_signal_backend</literal>,
         however only superusers can terminate superuser backends.
        </para>
diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml
index 07a16247d7..340cefff70 100644
--- a/doc/src/sgml/user-manag.sgml
+++ b/doc/src/sgml/user-manag.sgml
@@ -661,6 +661,11 @@ DROP ROLE doomed_role;
        <entry>pg_signal_backend</entry>
        <entry>Signal another backend to cancel a query or terminate its session.</entry>
       </row>
+      <row>
+       <entry>pg_signal_autovacuum_worker</entry>
+       <entry>Signal an autovacuum worker to cancel the current table's vacuum
+       or terminate its session.</entry>
+      </row>
       <row>
        <entry>pg_read_server_files</entry>
        <entry>Allow reading files from any location the database can access on the server with COPY and
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index 88e9bf8125..780a1c682a 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -34,8 +34,9 @@
  * role as the backend being signaled. For "dangerous" signals, an explicit
  * check for superuser needs to be done prior to calling this function.
  *
- * Returns 0 on success, 1 on general failure, 2 on normal permission error
- * and 3 if the caller needs to be a superuser.
+ * Returns 0 on success, 1 on general failure, 2 on normal permission error,
+ * 3 if the caller needs to be a superuser, and 4 if the caller needs to have
+ * privileges of pg_signal_autovacuum_worker.
  *
  * In the event of a general failure (return code 1), a warning message will
  * be emitted. For permission errors, doing that is the responsibility of
@@ -45,6 +46,7 @@
 #define SIGNAL_BACKEND_ERROR 1
 #define SIGNAL_BACKEND_NOPERMISSION 2
 #define SIGNAL_BACKEND_NOSUPERUSER 3
+#define SIGNAL_BACKEND_NOAUTOVAC 4
 static int
 pg_signal_backend(int pid, int sig)
 {
@@ -77,15 +79,27 @@ pg_signal_backend(int pid, int sig)
 	/*
 	 * Only allow superusers to signal superuser-owned backends.  Any process
 	 * not advertising a role might have the importance of a superuser-owned
-	 * backend, so treat it that way.
+	 * backend, so treat it that way.  As an exception, we allow roles with
+	 * privileges of pg_signal_autovacuum_worker to signal autovacuum workers
+	 * (which do not advertise a role).
+	 *
+	 * Otherwise, users can signal backends for roles they have privileges of.
 	 */
-	if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
-		!superuser())
-		return SIGNAL_BACKEND_NOSUPERUSER;
+	if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId))
+	{
+		ProcNumber	procNumber = GetNumberFromPGProc(proc);
+		PgBackendStatus *procStatus = pgstat_get_beentry_by_proc_number(procNumber);
 
-	/* Users can signal backends they have role membership in. */
-	if (!has_privs_of_role(GetUserId(), proc->roleId) &&
-		!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
+		if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER)
+		{
+			if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER))
+				return SIGNAL_BACKEND_NOAUTOVAC;
+		}
+		else if (!superuser())
+			return SIGNAL_BACKEND_NOSUPERUSER;
+	}
+	else if (!has_privs_of_role(GetUserId(), proc->roleId) &&
+			 !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
 		return SIGNAL_BACKEND_NOPERMISSION;
 
 	/*
@@ -130,6 +144,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
 				 errdetail("Only roles with the %s attribute may cancel queries of roles with the %s attribute.",
 						   "SUPERUSER", "SUPERUSER")));
 
+	if (r == SIGNAL_BACKEND_NOAUTOVAC)
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied to cancel query"),
+				 errdetail("Only roles with privileges of the \"%s\" role may cancel queries of autovacuum workers.",
+						   "pg_signal_autovacuum_worker")));
+
 	if (r == SIGNAL_BACKEND_NOPERMISSION)
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -236,6 +257,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
 				 errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.",
 						   "SUPERUSER", "SUPERUSER")));
 
+	if (r == SIGNAL_BACKEND_NOAUTOVAC)
+		ereport(ERROR,
+				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+				 errmsg("permission denied to terminate process"),
+				 errdetail("Only roles with privileges of the \"%s\" role may terminate autovacuum worker processes.",
+						   "pg_signal_autovacuum_worker")));
+
 	if (r == SIGNAL_BACKEND_NOPERMISSION)
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index bf00815c14..0129f67eaa 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -99,5 +99,10 @@
   rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
   rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
   rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER',
+  rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't',
+  rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+  rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+  rolpassword => '_null_', rolvaliduntil => '_null_' },
 
 ]
-- 
2.39.3 (Apple Git-146)

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 14 Jun 2024 12:06:36 +0500

> On 13 Jun 2024, at 02:04, Nathan Bossart <[email-masked]> wrote:
> 

This patch looks good to me. Spellchecker is complaining about “signaling” instead of “signalling”, but ISTM it’s OK.

I’ve tried to dig into the test.
The problem is CV is allocated in

inj_state = GetNamedDSMSegment("injection_points”,

which seems to be destroyed in

shmem_exit() calling dsm_backend_shutdown()

This happens before we broadcast that sleep is over.
I think this might happen with any wait on injection point if it is pg_terminate_backend()ed.

Is there way to wake up from CV sleep before processing actual termination?

Thanks!

Best regards, Andrey Borodin.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 14 Jun 2024 15:12:50 -0500

On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote:
> This patch looks good to me.

Thanks for looking.

> Spellchecker is complaining about "signaling" instead of "signalling",
> but ISTM it´s OK.

I think this is an en-US versus en-GB thing.  We've standardized on en-US
for "cancel" (see commits 8c9da14, 21f1e15, and af26857), so IMO we might
as well do so for "signal," too.

-- 
nathan

---

From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jun 2024 13:01:41 +0900

> pg_terminate_backend()ed.

Except if I am missing something, this is not a problem for a normal
backend, for example with one using a `SELECT injection_points_run()`.

> Is there way to wake up from CV sleep before processing actual termination?

I am honestly not sure if this is worth complicating the sigjmp path
of the autovacuum worker just for the sake of this test.  It seems to
me that it would be simple enough to move the injection point
autovacuum-worker-start within the transaction block a few lines down
in do_autovacuum(), no?
--
Michael

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Fri, 21 Jun 2024 10:31:30 +0500

> On 21 Jun 2024, at 09:01, Michael Paquier <[email-masked]> wrote:
> 

Yes, i’ve tried to get similar error in other CV-sleeps and in injection points of normal backend - everything works just fine. The error is specific to just this test.

> autovacuum-worker-start within the transaction block a few lines down

Thanks for the pointer, I’ll try this approach!

Best regards, Andrey Borodin,

---

From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jun 2024 14:36:17 +0900

> > This patch looks good to me.

While double-checking the whole, where I don't have much to say about
0001, I have fixed a few issues with the test presented upthread and
stabilized it (CI and my stuff are both OK).  I'd suggest to move it
to test_misc/, because there is no clear category where to put it, and
we have another test with injection points there for timeouts so the
module dependency with EXTRA_INSTALL is already cleared.

What do you think?
--
Michael
From 2d3e219149b8dbdbaf2b465f619f898c61969b17 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Wed, 12 Jun 2024 15:38:14 -0500
Subject: [PATCH v6 1/2] Introduce pg_signal_autovacuum_worker.

Author: Kirill Reshke

 ]

 	/*

index 2609269610..f73ba439c0 100644
@@ -28040,7 +28040,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
@@ -28114,7 +28116,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
2.45.2

From 93b7d69585aa4a408749bee6239006d6494df1bb Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jun 2024 14:29:23 +0900
Subject: [PATCH v6 2/2] Add tap test for pg_signal_autovacuum role

---
 src/backend/postmaster/autovacuum.c           |   7 ++
 .../test_misc/t/006_signal_autovacuum.pl      | 100 ++++++++++++++++++
 2 files changed, 107 insertions(+)
 create mode 100644 src/test/modules/test_misc/t/006_signal_autovacuum.pl

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..0d4e2c5140 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -100,6 +100,7 @@
 #include "utils/fmgroids.h"
 #include "utils/fmgrprotos.h"
 #include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/ps_status.h"
@@ -1902,6 +1903,12 @@ do_autovacuum(void)
 	/* Start a transaction so our commands have one to play into. */
 	StartTransactionCommand();
 
+	/*
+	 * This injection point is put in a transaction block to work with a wait
+	 * that uses a condition variable.
+	 */
+	INJECTION_POINT("autovacuum-worker-start");
+
 	/*
 	 * Compute the multixact age for which freezing is urgent.  This is
 	 * normally autovacuum_multixact_freeze_max_age, but may be less if we are
diff --git a/src/test/modules/test_misc/t/006_signal_autovacuum.pl b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
new file mode 100644
index 0000000000..98cbea3744
--- /dev/null
+++ b/src/test/modules/test_misc/t/006_signal_autovacuum.pl
@@ -0,0 +1,100 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test signaling autovacuum worker backend by non-superuser role.
+#
+# Only non-superuser roles granted pg_signal_autovacuum_worker are allowed
+# to signal autovacuum workers.  This test uses an injection point located
+# at the beginning of the autovacuum worker startup.
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+# Initialize postgres
+my $psql_err = '';
+my $psql_out = '';
+my $node = PostgreSQL::Test::Cluster->new('node');
+$node->init;
+
+# This ensures a quick worker spawn.
+$node->append_conf(
+	'postgresql.conf', 'autovacuum_naptime = 1');
+$node->start;
+$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+$node->safe_psql(
+	'postgres', qq(
+    CREATE ROLE regular_role;
+    CREATE ROLE signal_autovacuum_worker_role;
+    GRANT pg_signal_autovacuum_worker TO signal_autovacuum_worker_role;
+));
+
+# From this point, autovacuum worker will wait at startup.
+$node->safe_psql('postgres',
+	"SELECT injection_points_attach('autovacuum-worker-start', 'wait');");
+
+# Create some content and set an aggressive autovacuum.
+$node->safe_psql(
+	'postgres', qq(
+    CREATE TABLE tab_int(i int);
+    ALTER TABLE tab_int SET (autovacuum_vacuum_cost_limit = 1);
+    ALTER TABLE tab_int SET (autovacuum_vacuum_cost_delay = 100);
+));
+
+$node->safe_psql(
+	'postgres', qq(
+    INSERT INTO tab_int VALUES(1);
+));
+
+# Wait until an autovacuum worker starts.
+$node->wait_for_event('autovacuum worker', 'autovacuum-worker-start');
+
+my $av_pid = $node->safe_psql(
+	'postgres', qq(
+    SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum worker';
+));
+
+# Regular role cannot terminate autovacuum worker.
+my $terminate_with_no_pg_signal_av = $node->psql(
+	'postgres', qq(
+    SET ROLE regular_role;
+    SELECT pg_terminate_backend($av_pid);
+),
+	stdout => \$psql_out,
+	stderr => \$psql_err);
+
+like(
+	$psql_err,
+	qr/ERROR:  permission denied to terminate process\nDETAIL:  Only roles with privileges of the "pg_signal_autovacuum_worker" role may terminate autovacuum worker processes./,
+	"autovacuum worker not signaled with regular role");
+
+my $offset = -s $node->logfile;
+
+# Role with pg_signal_autovacuum can terminate autovacuum worker.
+my $terminate_with_pg_signal_av = $node->psql(
+	'postgres', qq(
+    SET ROLE signal_autovacuum_worker_role;
+    SELECT pg_terminate_backend($av_pid);
+),
+	stdout => \$psql_out,
+	stderr => \$psql_err);
+
+# Check that the primary server logs a FATAL indicating that autovacuum
+# is terminated.
+ok( $node->log_contains(
+		qr/FATAL:  terminating autovacuum process due to administrator command/,
+		$offset),
+	"autovacuum worker signaled with pg_signal_autovacuum_worker granted"
+);
+
+# Release injection point.
+$node->safe_psql('postgres',
+	"SELECT injection_points_detach('autovacuum-worker-start');");
+
+done_testing();
-- 
2.45.2

---

From: Michael Paquier <[email-masked]>
Date: Fri, 21 Jun 2024 14:38:11 +0900

> Thanks for the pointer, I’ll try this approach!

Thanks.  FWIW, I've put my mind into it, and fixed the thing a few
minutes ago:
https://www.postgresql.org/message-id/ZnURUaujl39wSoEW%40paquier.xyz
--
Michael