On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote: > On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote: > > This patch looks good to me. > > Thanks for looking.
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 <nat...@postgresql.org> Date: Wed, 12 Jun 2024 15:38:14 -0500 Subject: [PATCH v6 1/2] 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 --- src/include/catalog/pg_authid.dat | 5 +++ src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------ doc/src/sgml/func.sgml | 8 +++-- doc/src/sgml/user-manag.sgml | 5 +++ 4 files changed, 53 insertions(+), 11 deletions(-) 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_' }, ] 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/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 2609269610..f73ba439c0 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -28040,7 +28040,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> @@ -28114,7 +28116,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 -- 2.45.2
From 93b7d69585aa4a408749bee6239006d6494df1bb Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
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
signature.asc
Description: PGP signature
