> 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 tried to add them above the existing code. When I test it locally, a user 
without pg_signal_autovacuum will actually fail at this block because the user 
is not superuser and !OidIsValid(proc->roleId) is also true in the following:

        /*
         * 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.
         */
        if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
                !superuser())
                return SIGNAL_BACKEND_NOSUPERUSER;

This is what Im planning to do - If the backend is autovacuum worker and the 
user is not superuser or has pg_signal_autovacuum role, we return the new value 
and provide the relevant error message     

              /*
         * If the backend is autovacuum worker, allow user with privileges of 
the 
               * pg_signal_autovacuum role to signal the backend.
         */
        if (pgstat_get_backend_type(proc->backendId) == B_AUTOVAC_WORKER)
        {
                if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM) 
|| !superuser())
                        return SIGNAL_BACKEND_NOAUTOVACUUM;
        }
        /*
         * 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.
        */
        else if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) &&
                         !superuser())
        {
                return SIGNAL_BACKEND_NOSUPERUSER;
        }
        /* Users can signal backends they have role membership in. */
        else if (!has_privs_of_role(GetUserId(), proc->roleId) &&
                         !has_privs_of_role(GetUserId(), 
ROLE_PG_SIGNAL_BACKEND))
        {
                return SIGNAL_BACKEND_NOPERMISSION;
        }


> We can add tests just like [0] with injection points.
> I mean replace that "sleep 1" with something like 
> "$node->wait_for_event('autovacuum worker', 'autocauum-runing');".
> Currently we have no infrastructure to wait for autovacuum of particular 
> table, but I think it's doable.
> Also I do not like that test is changing system-wide autovac settings, AFAIR 
> these settings can be set for particular table.

Thanks for the suggestion. I will take a look at this. Let me also separate the 
test into a different patch file.

--
Anthony Leung
Amazon Web Services: https://aws.amazon.com




Reply via email to