Hi, 

I'm new to reviewing postgres patches, but I took an interest in reviewing this 
patch as recommended by Nathan.

I have the following comments:

>       if (!superuser()) {
>               if (!OidIsValid(proc->roleId)) {
>                       LocalPgBackendStatus *local_beentry;
>                       local_beentry = 
> pgstat_get_local_beentry_by_backend_id(proc->backendId);
>
>                       if (!(local_beentry && 
> local_beentry->backendStatus.st_backendType == B_AUTOVAC_WORKER && 
>                               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;
>               }
>       }
>
1. I would suggest not to do nested if blocks since it's becoming harder to 
read. Also, does it make sense to have a utilities function in backend_status.c 
to check if a backend of a given backend id is of a certain backend_type. And 
should we check if proc->backendId is invalid?

>    ALTER SYSTEM SET autovacuum_vacuum_cost_limit TO 1;
>    ALTER SYSTEM SET autovacuum_vacuum_cost_delay TO 100;
>    ALTER SYSTEM SET autovacuum_naptime TO 1;    
2. Could we set these parameters at the beginning of the test before 
$node->start with $node->append_conf ? That way we can avoid restarting the 
node and doing the sleep later on.

> my $res_pid = $node_primary->safe_psql(
>.           'regress',
>       "SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum 
> worker' and datname = 'regress';"
> );
>
> my ($res_reg_psa_1, $stdout_reg_psa_1, $stderr_reg_psa_1)  = 
> $node_primary->psql('regress', qq[
     SET ROLE psa_reg_role_1;
>   SELECT pg_terminate_backend($res_pid);
>  ]);
>
> ok($res_reg_psa_1 != 0, "should fail for non pg_signal_autovacuum");
> like($stderr_reg_psa_1, qr/Only roles with the SUPERUSER attribute may 
> terminate processes of roles with the SUPERUSER attribute./, "matches");
>
> my ($res_reg_psa_2, $stdout_reg_psa_2, $stderr_reg_psa_2) = 
> $node_primary->psql('regress', qq[
>    SET ROLE psa_reg_role_2;
>    SELECT pg_terminate_backend($res_pid);
> ]");
3. Some nits on styling 

4. According to Postgres styles, I believe open brackets should be in a new 
line 

Reply via email to