Re: Allow non-superuser to cancel superuser tasks.

On Mon, Apr 01, 2024 at 02:29:29PM +0000, Leung, Anthony wrote:
> I've attached the updated patch with some improvements.

Thanks!

+      <row>
+       <entry>pg_signal_autovacuum</entry>
+       <entry>Allow terminating backend running autovacuum</entry>
+      </row>

I think we should be more precise here by calling out the exact types of
workers:

    "Allow signaling autovacuum worker processes to..."

-    if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
-        !superuser())
-        return SIGNAL_BACKEND_NOSUPERUSER;
-
-    /* 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))
-        return SIGNAL_BACKEND_NOPERMISSION;
+    if (!superuser())
+    {
+        if (!OidIsValid(proc->roleId))
+        {
+            /*
+             * We only allow user with pg_signal_autovacuum role to terminate
+             * autovacuum worker as an exception. 
+             */
+            if (!(pg_stat_is_backend_autovac_worker(proc->backendId) &&
+                has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM)))
+                return SIGNAL_BACKEND_NOSUPERUSER;
+        }
+        else
+        {
+            if (superuser_arg(proc->roleId))
+                return SIGNAL_BACKEND_NOSUPERUSER;
+
+            /* 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))
+                return SIGNAL_BACKEND_NOPERMISSION;
+        }
+    }

I don't think we should rely on !OidIsValid(proc->roleId) for signaling
autovacuum workers.  That might not always be true, and I don't see any
need to rely on that otherwise.  IMHO we should just add a few lines before
the existing code, which doesn't need to be changed at all:

    if (pg_stat_is_backend_autovac_worker(proc->backendId) &&
        !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM))
        return SIGNAL_BACKEND_NOAUTOVACUUM;

I also think we need to return something besides SIGNAL_BACKEND_NOSUPERUSER
in this case.  Specifically, we probably need to introduce a new value and
provide the relevant error messages in pg_cancel_backend() and
pg_terminate_backend().

+/* ----------
+ * pg_stat_is_backend_autovac_worker() -
+ *
+ * Return whether the backend of the given backend id is of type autovacuum worker.
+ */
+bool
+pg_stat_is_backend_autovac_worker(BackendId beid)
+{
+    PgBackendStatus *ret;
+
+    Assert(beid != InvalidBackendId);
+
+    ret = pgstat_get_beentry_by_backend_id(beid);
+
+    if (!ret)
+        return false;
+
+    return ret->st_backendType == B_AUTOVAC_WORKER;
+}

Can we have this function return the backend type so that we don't have to
create a new function for every possible type?  That might be handy in the
future.

I haven't looked too closely, but I'm pretty skeptical that the test suite
in your patch would be stable.  Unfortunately, I don't have any better
ideas at the moment besides not adding a test for this new role.

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