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