Re: Allow non-superuser to cancel superuser tasks.

On Fri, Apr 05, 2024 at 12:03:05AM +0000, Leung, Anthony wrote:
> Adding tap test for pg_signal_autovacuum using injection points as a
> separate patch. I also made a minor change on the original patch.

+    ret = pgstat_get_beentry_by_proc_number(procNumber);
+
+    if (!ret)
+        return false;

An invalid BackendType is not false, but B_INVALID.

+{ oid => '6312', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM',

OIDs in patches under development should use a value in the range
8000-9999.  Newly-assigned OIDs are renumbered after the feature
freeze.

+    /*
+     * If the backend is autovacuum worker, allow user with the privileges of
+     * pg_signal_autovacuum role to signal the backend.
+     */
+    if (pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER)
+    {
+        if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
+            return SIGNAL_BACKEND_NOAUTOVACUUM;
+    }

I was wondering why this is not done after we've checked that we have
a superuser-owned backend, and this is giving me a pause.  @Nathan,
why do you think we should not rely on the roleId for an autovacuum
worker?  In core, do_autovacuum() is only called in a process without
a role specified, and I've noticed your remark here:
https://www.postgresql.org/message-id/20240401202146.GA2354284@nathanxps13
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.

One thing that we should definitely not do is letting any user calling
pg_signal_backend() know that a given PID maps to an autovacuum
worker.  This information is hidden in pg_stat_activity.  And
actually, doesn't the patch leak this information to all users when
calling pg_signal_backend with random PID numbers because of the fact
that SIGNAL_BACKEND_NOAUTOVACUUM exists?  Any role could guess which
PIDs are used by an autovacuum worker because of the granularity
required for the error related to pg_signal_autovacuum.

+    INJECTION_POINT("autovacuum-start");
Perhaps autovacuum-worker-start is more suited here.  I am not sure
that the beginning of do_autovacuum() is the optimal location, as what
matters is that we've done InitPostgres() to be able to grab the PID
from pg_stat_activity.  This location does the job.

+if ($ENV{enable_injection_points} ne 'yes')
+{
+    plan skip_all => 'Injection points not supported by this build';
+}
[...]
+$node->safe_psql('postgres',
+    "SELECT injection_points_attach('autovacuum-start', 'wait');");
[...]
+# Wait until the autovacuum worker starts
+$node->wait_for_event('autovacuum worker', 'autovacuum-start');

This integration with injection points looks correct to me.

+# 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.  No problems for me with using a new path in
src/test/ for that kind of tests.  There are no similar locations.

+    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.

+# User with signal_backend_role cannot terminate autovacuum worker

Not sure that there is a huge point in checking after a role that
holds pg_signal_backend.  An autovacuum worker is not a backend.  Only
the case of a role not member of pg_signal_autovacuum should be
enough.

+# Test signaling for pg_signal_autovacuum role.

This needs a better documentation: the purpose of the test is to
signal an autovacuum worker, aka it uses an injection point to ensure
that the worker for the whole duration of the test.

It seems to me that it would be a better practice to wakeup the
injection point and detach it before being done with the worker.
That's not mandatory but it would encourage the correct flow if this
code is copy-pasted around to other tests.

+like($psql_err, qr/ERROR:  permission denied to terminate ...

Checking only the ERRROR, and not the DETAIL should be sufficient
here.

+# User with pg_signal_backend can terminate autovacuum worker
+my $terminate_with_pg_signal_av = $node->psql('postgres', qq(
+    SET ROLE signal_autovacuum_role;
+    SELECT pg_terminate_backend($av_pid);
+), stdout => \$psql_out, stderr => \$psql_err);
+
+ok($terminate_with_pg_signal_av == 0, "Terminating autovacuum worker should succeed with pg_signal_autovacuum role");

Is that enough for the validation?  How about checking some pattern in
the server logs from an offset before running this last query?
--
Michael