From: | "Leung, Anthony" <antholeu(at)amazon(dot)com> |
---|---|
To: | Nathan Bossart <nathandbossart(at)gmail(dot)com>, Kirill Reshke <reshkekirill(at)gmail(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Allow non-superuser to cancel superuser tasks. |
Date: | 2024-03-10 04:13:50 |
Message-ID: | A962CC9C-05EC-4332-8E1E-1B2FC9A78D2E@amazon.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Leung, Anthony | 2024-03-10 04:38:59 | Re: Allow non-superuser to cancel superuser tasks. |
Previous Message | Thomas Munro | 2024-03-10 04:02:39 | Re: Failures in constraints regression test, "read only 0 of 8192 bytes" |