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
utilitiesfunction in backend_status.c to check if a backend of a given backend id is of a certain backend_type. And
shouldwe 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
canavoid 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
SUPERUSERattribute./, "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