public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v11 5/7] Row pattern recognition patch (docs). 19+ messages / 6 participants [nested] [flat]
* [PATCH v11 5/7] Row pattern recognition patch (docs). @ 2023-11-08 06:57 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Tatsuo Ishii @ 2023-11-08 06:57 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 80 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 170 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..cf18dd887e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,86 @@ WHERE pos < 3; <literal>rank</literal> less than 3. </para> + <para> + Row pattern common syntax can be used to perform row pattern recognition + in a query. Row pattern common syntax includes two sub + clauses: <literal>DEFINE</literal> + and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines + definition variables along with an expression. The expression must be a + logical expression, which means it must + return <literal>TRUE</literal>, <literal>FALSE</literal> + or <literal>NULL</literal>. The expression may comprise column references + and functions. Window functions, aggregate functions and subqueries are + not allowed. An example of <literal>DEFINE</literal> is as follows. + +<programlisting> +DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +</programlisting> + + Note that <function>PREV</function> returns the price column in the + previous row if it's called in a context of row pattern recognition. So in + the second line the definition variable "UP" is <literal>TRUE</literal> + when the price column in the current row is greater than the price column + in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when + the price column in the current row is lower than the price column in the + previous row. + </para> + <para> + Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be + used. <literal>PATTERN</literal> defines a sequence of rows that satisfies + certain conditions. For example following <literal>PATTERN</literal> + defines that a row starts with the condition "LOWPRICE", then one or more + rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that + "+" means one or more matches. Also you can use "*", which means zero or + more matches. If a sequence of rows which satisfies the PATTERN is found, + in the starting row of the sequence of rows all window functions and + aggregates are shown in the target list. Note that aggregations only look + into the matched rows, rather than whole frame. In the second or + subsequent rows all window functions and aggregates are NULL. For rows + that do not match the PATTERN, all window functions and aggregates are + shown AS NULL too, except count which shows 0. This is because the + unmatched rows are in an empty frame. Example of + a <literal>SELECT</literal> using the <literal>DEFINE</literal> + and <literal>PATTERN</literal> clause is as follows. + +<programlisting> +SELECT company, tdate, price, + first_value(price) OVER w, + max(price) OVER w, + count(price) OVER w +FROM stock, + WINDOW w AS ( + PARTITION BY company + ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + AFTER MATCH SKIP PAST LAST ROW + INITIAL + PATTERN (LOWPRICE UP+ DOWN+) + DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +); +</programlisting> +<screen> + company | tdate | price | first_value | max | count +----------+------------+-------+-------------+-----+------- + company1 | 2023-07-01 | 100 | 100 | 200 | 4 + company1 | 2023-07-02 | 200 | | | + company1 | 2023-07-03 | 150 | | | + company1 | 2023-07-04 | 140 | | | + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | + company1 | 2023-07-08 | 130 | | | + company1 | 2023-07-09 | 120 | | | + company1 | 2023-07-10 | 130 | | | 0 +</screen> + </para> + <para> When a query involves multiple window functions, it is possible to write out each one with a separate <literal>OVER</literal> clause, but this is diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index d963f0a0a0..c3a8167c8e 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -21933,6 +21933,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -21972,6 +21973,59 @@ SELECT count(*) FROM sometable; Other frame specifications can be used to obtain other effects. </para> + <para> + Row pattern recognition navigation functions are listed in + <xref linkend="functions-rpr-navigation-table"/>. These functions + can be used to describe DEFINE clause of Row pattern recognition. + </para> + + <table id="functions-rpr-navigation-table"> + <title>Row Pattern Navigation Functions</title> + <tgroup cols="1"> + <thead> + <row> + <entry role="func_table_entry"><para role="func_signature"> + Function + </para> + <para> + Description + </para></entry> + </row> + </thead> + + <tbody> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>prev</primary> + </indexterm> + <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the previous row; + returns NULL if there is no previous row in the window frame. + </para></entry> + </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>next</primary> + </indexterm> + <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the next row; + returns NULL if there is no next row in the window frame. + </para></entry> + </row> + + </tbody> + </tgroup> + </table> + <note> <para> The SQL standard defines a <literal>RESPECT NULLS</literal> or diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 42d78913cf..522ad9dd70 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl The <replaceable class="parameter">frame_clause</replaceable> can be one of <synopsis> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] </synopsis> where <replaceable>frame_start</replaceable> @@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS a given peer group will be in the frame or excluded from it. </para> + <para> + The + optional <replaceable class="parameter">row_pattern_common_syntax</replaceable> + defines the <firstterm>row pattern recognition condition</firstterm> for + this + window. <replaceable class="parameter">row_pattern_common_syntax</replaceable> + includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST + ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls + how to proceed to next row position after a match + found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the + default) next row position is next to the last row of previous match. On + the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next + row position is always next to the last row of previous + match. <literal>DEFINE</literal> defines definition variables along with a + boolean expression. <literal>PATTERN</literal> defines a sequence of rows + that satisfies certain conditions using variables defined + in <literal>DEFINE</literal> clause. If the variable is not defined in + the <literal>DEFINE</literal> clause, it is implicitly assumed + following is defined in the <literal>DEFINE</literal> clause. + +<synopsis> +<literal>variable_name</literal> AS TRUE +</synopsis> + + Note that the maximu number of variables defined + in <literal>DEFINE</literal> clause is 26. + +<synopsis> +[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ] +PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...] +DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...] +</synopsis> + </para> + <para> The purpose of a <literal>WINDOW</literal> clause is to specify the behavior of <firstterm>window functions</firstterm> appearing in the query's -- 2.25.1 ----Next_Part(Wed_Nov__8_16_37_05_2023_872)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v11-0006-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-09 22:58 Michael Paquier <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Michael Paquier @ 2024-04-09 22:58 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Leung, Anthony <[email protected]>; Nathan Bossart <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Wed, Apr 10, 2024 at 12:52:19AM +0300, Kirill Reshke wrote: > On Tue, 9 Apr 2024 at 08:53, Michael Paquier <[email protected]> wrote: >> The thing is that you cannot rely on a lookup of the backend type for >> the error information, or you open yourself to letting the caller of >> pg_cancel_backend or pg_terminate_backend know if a backend is >> controlled by a superuser or if a backend is an autovacuum worker. > > Good catch. Thanks. I think we need to update the error message to not > leak backend type info. Yep, that's necessary I am afraid. >> The choice of pg_signal_autovacuum is a bit inconsistent, as well, >> because autovacuum workers operate like regular backends. This name >> can also be confused with the autovacuum launcher. > > Ok. What would be a good choice? Is `pg_signal_autovacuum_worker` good > enough? Sounds fine to me. Perhaps others have an opinion about that? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-10 15:00 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Nathan Bossart @ 2024-04-10 15:00 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Wed, Apr 10, 2024 at 07:58:39AM +0900, Michael Paquier wrote: > On Wed, Apr 10, 2024 at 12:52:19AM +0300, Kirill Reshke wrote: >> On Tue, 9 Apr 2024 at 08:53, Michael Paquier <[email protected]> wrote: >>> The thing is that you cannot rely on a lookup of the backend type for >>> the error information, or you open yourself to letting the caller of >>> pg_cancel_backend or pg_terminate_backend know if a backend is >>> controlled by a superuser or if a backend is an autovacuum worker. >> >> Good catch. Thanks. I think we need to update the error message to not >> leak backend type info. > > Yep, that's necessary I am afraid. Isn't it relatively easy to discover this same information today via pg_stat_progress_vacuum? That has the following code: /* Value available to all callers */ values[0] = Int32GetDatum(beentry->st_procpid); values[1] = ObjectIdGetDatum(beentry->st_databaseid); I guess I'm not quite following why we are worried about leaking whether a backend is an autovacuum worker. >>> The choice of pg_signal_autovacuum is a bit inconsistent, as well, >>> because autovacuum workers operate like regular backends. This name >>> can also be confused with the autovacuum launcher. >> >> Ok. What would be a good choice? Is `pg_signal_autovacuum_worker` good >> enough? > > Sounds fine to me. Perhaps others have an opinion about that? WFM -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-10 22:21 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Michael Paquier @ 2024-04-10 22:21 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Wed, Apr 10, 2024 at 10:00:34AM -0500, Nathan Bossart wrote: > Isn't it relatively easy to discover this same information today via > pg_stat_progress_vacuum? That has the following code: > > /* Value available to all callers */ > values[0] = Int32GetDatum(beentry->st_procpid); > values[1] = ObjectIdGetDatum(beentry->st_databaseid); > > I guess I'm not quite following why we are worried about leaking whether a > backend is an autovacuum worker. Good point. I've missed that we make no effort currently to hide any PID information from the progress tables. And we can guess more context data because of the per-table split of the progress tables. This choice comes down to b6fb6471f6af that has introduced the progress report facility, so this ship has long sailed it seems. And it makes my argument kind of moot. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-11 11:55 Kirill Reshke <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 4 replies; 19+ messages in thread From: Kirill Reshke @ 2024-04-11 11:55 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers Posting updated version of this patch with comments above addressed. 1) pg_signal_autovacuum -> pg_signal_autovacuum_worker, as there seems to be no objections to that. 2) There are comments on how to write if statement: > In core, do_autovacuum() is only called in a process without > a role specified > It's feeling more natural here to check that we have a superuser-owned > backend first, and then do a lookup of the process type. > I figured since there's no reason to rely on that behavior, we might as > well do a bit of future-proofing in case autovacuum workers are ever not > run as InvalidOid. I have combined them into this: if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER) This is both future-proofing and natural, I suppose. Downside of this is double checking condition (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)), but i think that is ok for the sake of simplicity. 3) pg_signal_autovacuum_worker Oid changed to random one: 8916 4) > An invalid BackendType is not false, but B_INVALID. fixed, thanks 5) >>>> There is pg_read_all_stats as well, so I don't see a big issue in >>>> requiring to be a member of this role as well for the sake of what's >>>> proposing here. >>> >>> Well, that tells you quite a bit more than just which PIDs correspond to >>> autovacuum workers, but maybe that's good enough for now. >> >> That may be a good initial compromise, for now. >Sounds good to me. I will update the documentation. @Anthony if you feel that documentation update adds much value here, please do. Given that we know autovacuum worker PIDs from pg_stat_progress_vacuum, I don't know how to reflect something about pg_stat_autovac_worker in doc, and if it is worth it. 6) > + INJECTION_POINT("autovacuum-start"); > Perhaps autovacuum-worker-start is more suited here fixed, thanks 7) > +# Copyright (c) 2022-2024, PostgreSQL Global Development Group > [...] > +# Copyright (c) 2024-2024, PostgreSQL Global Development Group > These need to be cleaned up. > +# Makefile for src/test/recovery > +# > +# src/test/recovery/Makefile > This is incorrect, twice. Cleaned up, thanks! 8) > Not sure that there is a huge point in checking after a role that > holds pg_signal_backend. Ok. Removed. Then: > +like($psql_err, qr/ERROR: permission denied to terminate ... > Checking only the ERRROR, and not the DETAIL should be sufficient > here. After removing the pg_signal_backend test case we have only one place where errors check is done. So, I think we should keep DETAIL here to ensure detail is correct (it differs from regular backend case). 9) > +# Test signaling for pg_signal_autovacuum role. > This needs a better documentation: Updated. Hope now the test documentation helps to understand it. 10) > +ok($terminate_with_pg_signal_av == 0, "Terminating autovacuum worker should succeed with pg_signal_autovacuum role"); > Is that enough for the validation? Added: ok($node->log_contains(qr/FATAL: terminating autovacuum process due to administrator command/, $offset), "Autovacuum terminates when role is granted with pg_signal_autovacuum_worker"); 11) references to `passcheck` extension removed. errors messages rephrased. 12) injection_point_detach added. 13) > + INSERT INTO tab_int SELECT * FROM generate_series(1, 1000000); > A good chunk of the test would be spent on that, but you don't need > that many tuples to trigger an autovacuum worker as the short naptime > is able to do it. I would recommend to reduce that to a minimum. +1 Single tuple works. 14) v3 suffers from segfault: 2024-04-11 11:28:31.116 UTC [147437] 001_signal_autovacuum.pl LOG: statement: SELECT pg_terminate_backend(147427); 2024-04-11 11:28:31.116 UTC [147427] FATAL: terminating autovacuum process due to administrator command 2024-04-11 11:28:31.116 UTC [147410] LOG: server process (PID 147427) was terminated by signal 11: Segmentation fault 2024-04-11 11:28:31.116 UTC [147410] LOG: terminating any other active server processes 2024-04-11 11:28:31.117 UTC [147410] LOG: shutting down because restart_after_crash is off 2024-04-11 11:28:31.121 UTC [147410] LOG: database system is shut down The test doesn't fail because pg_terminate_backend actually meets his point: autovac is killed. But while dying, autovac also receives segfault. Thats because of injections points: (gdb) bt #0 0x000056361c3379ea in tas (lock=0x7fbcb9632224 <error: Cannot access memory at address 0x7fbcb9632224>) at ../../../../src/include/storage/s_lock.h:228 #1 ConditionVariableCancelSleep () at condition_variable.c:238 #2 0x000056361c337e4b in ConditionVariableBroadcast (cv=0x7fbcb66f498c) at condition_variable.c:310 #3 0x000056361c330a40 in CleanupProcSignalState (status=<optimized out>, arg=<optimized out>) at procsignal.c:240 #4 0x000056361c328801 in shmem_exit (code=code@entry=1) at ipc.c:276 #5 0x000056361c3288fc in proc_exit_prepare (code=code@entry=1) at ipc.c:198 #6 0x000056361c3289bf in proc_exit (code=code@entry=1) at ipc.c:111 #7 0x000056361c49ffa8 in errfinish (filename=<optimized out>, lineno=<optimized out>, funcname=0x56361c654370 <__func__.16> "ProcessInterrupts") at elog.c:592 #8 0x000056361bf7191e in ProcessInterrupts () at postgres.c:3264 #9 0x000056361c3378d7 in ConditionVariableTimedSleep (cv=0x7fbcb9632224, timeout=timeout@entry=-1, wait_event_info=117440513) at condition_variable.c:196 #10 0x000056361c337d0b in ConditionVariableTimedSleep (wait_event_info=<optimized out>, timeout=-1, cv=<optimized out>) at condition_variable.c:135 #11 ConditionVariableSleep (cv=<optimized out>, wait_event_info=<optimized out>) at condition_variable.c:98 #12 0x00000000b96347d0 in ?? () #13 0x3a3f1d9baa4f5500 in ?? () #14 0x000056361cc6cbd0 in ?? () #15 0x000056361ccac300 in ?? () #16 0x000056361c62be63 in ?? () #17 0x00007fbcb96347d0 in ?? () at injection_points.c:201 from /home/reshke/postgres/tmp_install/home/reshke/postgres/pgbin/lib/injection_points.so #18 0x00007fffe4122b10 in ?? () #19 0x00007fffe4122b70 in ?? () #20 0x0000000000000000 in ?? () discovered because of # Release injection point. $node->safe_psql('postgres', "SELECT injection_point_detach('autovacuum-worker-start');"); added v4 also suffers from that. i will try to fix that. Attachments: [application/octet-stream] v4-0001-Introduce-pg_signal_autovacuum-role-to-allow-non-.patch (6.7K, ../../CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com/2-v4-0001-Introduce-pg_signal_autovacuum-role-to-allow-non-.patch) download | inline diff: From 591fffa188eaecf59280ff32fe377fc782dd6457 Mon Sep 17 00:00:00 2001 From: Anthony Leung <[email protected]> Date: Thu, 4 Apr 2024 20:24:33 +0000 Subject: [PATCH v4 1/2] Introduce pg_signal_autovacuum role to allow non-superuser to signal autovacuum worker Non-superusers are currently not allowed to signal autovacuum workers which can create friction in some scenarios when performing maintenance. This commit introduce a new pre-defined role 'pg_signal_autovacuum'. Non-superusers granted with this role are allowed to signal backends that are running autovacuum. --- doc/src/sgml/user-manag.sgml | 4 +++ src/backend/storage/ipc/signalfuncs.c | 40 +++++++++++++++++---- src/backend/utils/activity/backend_status.c | 19 ++++++++++ src/include/catalog/pg_authid.dat | 5 +++ src/include/utils/backend_status.h | 2 +- 5 files changed, 63 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index 07a16247d7..2c5b569d4d 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -705,6 +705,10 @@ DROP ROLE doomed_role; database to issue <link linkend="sql-createsubscription"><command>CREATE SUBSCRIPTION</command></link>.</entry> </row> + <row> + <entry>pg_signal_autovacuum_worker</entry> + <entry>Allow signaling autovacuum worker backend to cancel or terminate</entry> + </row> </tbody> </tgroup> </table> diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index 88e9bf8125..12d93ba83f 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -45,6 +45,7 @@ #define SIGNAL_BACKEND_ERROR 1 #define SIGNAL_BACKEND_NOPERMISSION 2 #define SIGNAL_BACKEND_NOSUPERUSER 3 +#define SIGNAL_BACKEND_NOAUTOVACUUM 4 static int pg_signal_backend(int pid, int sig) { @@ -74,19 +75,32 @@ pg_signal_backend(int pid, int sig) return SIGNAL_BACKEND_ERROR; } + /* + * If the backend is autovacuum worker, allow user with the privileges of + * pg_signal_autovacuum_worker role to signal the backend. + */ + if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) + && pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER) + { + if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER)) + 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. - */ - if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && - !superuser()) + */ + else if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && + !superuser()) + { 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)) + else if (!has_privs_of_role(GetUserId(), proc->roleId) && + !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) + { return SIGNAL_BACKEND_NOPERMISSION; + } /* * Can the process we just validated above end, followed by the pid being @@ -137,6 +151,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS) errdetail("Only roles with privileges of the role whose query is being canceled or with privileges of the \"%s\" role may cancel this query.", "pg_signal_backend"))); + if (r == SIGNAL_BACKEND_NOAUTOVACUUM) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to cancel autovacuum worker backend"), + errdetail("Only roles with the %s attribute or with privileges of the \"%s\" role may cancel autovacuum worker backend", + "SUPERUSER", "pg_signal_autovacuum_worker"))); + PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS); } @@ -243,6 +264,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS) errdetail("Only roles with privileges of the role whose process is being terminated or with privileges of the \"%s\" role may terminate this process.", "pg_signal_backend"))); + if (r == SIGNAL_BACKEND_NOAUTOVACUUM) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to terminate autovacuum worker backend"), + errdetail("Only roles with the %s attribute or with privileges of the \"%s\" role may terminate autovacuum worker backend", + "SUPERUSER", "pg_signal_autovacuum_worker"))); + /* Wait only on success and if actually requested */ if (r == SIGNAL_BACKEND_SUCCESS && timeout > 0) PG_RETURN_BOOL(pg_wait_until_termination(pid, timeout)); diff --git a/src/backend/utils/activity/backend_status.c b/src/backend/utils/activity/backend_status.c index 1ccf4c6d83..a6c7899318 100644 --- a/src/backend/utils/activity/backend_status.c +++ b/src/backend/utils/activity/backend_status.c @@ -1038,6 +1038,25 @@ pgstat_get_my_query_id(void) return MyBEEntry->st_query_id; } +/* ---------- + * pgstat_get_backend_type() - + * + * Return the backend type of the backend for the given proc number. + * ---------- + */ +BackendType +pgstat_get_backend_type(ProcNumber procNumber) +{ + PgBackendStatus *ret; + + ret = pgstat_get_beentry_by_proc_number(procNumber); + + if (!ret) + return B_INVALID; + + return ret->st_backendType; +} + /* ---------- * cmp_lbestatus * diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index 55cabdda6f..6156fc55dd 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -99,5 +99,10 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER', + rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] diff --git a/src/include/utils/backend_status.h b/src/include/utils/backend_status.h index 7b7f6f59d0..7fd9cd7167 100644 --- a/src/include/utils/backend_status.h +++ b/src/include/utils/backend_status.h @@ -323,7 +323,7 @@ extern const char *pgstat_get_backend_current_activity(int pid, bool checkUser); extern const char *pgstat_get_crashed_backend_activity(int pid, char *buffer, int buflen); extern uint64 pgstat_get_my_query_id(void); - +extern BackendType pgstat_get_backend_type(ProcNumber procNumber); /* ---------- * Support functions for the SQL-callable functions to -- 2.25.1 [application/octet-stream] v4-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch (7.2K, ../../CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com/3-v4-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch) download | inline diff: From 7661fb08515c2d1f159111e3f11a92d406050cb0 Mon Sep 17 00:00:00 2001 From: Anthony Leung <[email protected]> Date: Thu, 4 Apr 2024 23:33:13 +0000 Subject: [PATCH v4 2/2] Add tap test for pg_signal_autovacuum role Add a tap test to validate the following: 1. Normal user cannot signal autovacuum worker backend 2. User with pg_signal_backend role cannot signal autovacuum worker backend 3. User with pg_signal_autovacuum role can signal autovacuum worker backend --- src/backend/postmaster/autovacuum.c | 3 + src/test/signals/.gitignore | 2 + src/test/signals/Makefile | 27 ++++++ src/test/signals/meson.build | 15 ++++ src/test/signals/t/001_signal_autovacuum.pl | 93 +++++++++++++++++++++ 5 files changed, 140 insertions(+) create mode 100644 src/test/signals/.gitignore create mode 100644 src/test/signals/Makefile create mode 100644 src/test/signals/meson.build create mode 100644 src/test/signals/t/001_signal_autovacuum.pl diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 170b973cc5..f13076f3b3 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -100,6 +100,7 @@ #include "utils/fmgroids.h" #include "utils/fmgrprotos.h" #include "utils/guc_hooks.h" +#include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/ps_status.h" @@ -1889,6 +1890,8 @@ do_autovacuum(void) bool found_concurrent_worker = false; int i; + INJECTION_POINT("autovacuum-worker-start"); + /* * StartTransactionCommand and CommitTransactionCommand will automatically * switch to other contexts. We need this one to keep the list of diff --git a/src/test/signals/.gitignore b/src/test/signals/.gitignore new file mode 100644 index 0000000000..871e943d50 --- /dev/null +++ b/src/test/signals/.gitignore @@ -0,0 +1,2 @@ +# Generated by test suite +/tmp_check/ diff --git a/src/test/signals/Makefile b/src/test/signals/Makefile new file mode 100644 index 0000000000..b5c1d65be4 --- /dev/null +++ b/src/test/signals/Makefile @@ -0,0 +1,27 @@ +#------------------------------------------------------------------------- +# +# Makefile for src/test/signals +# +# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group +# Portions Copyright (c) 1994, Regents of the University of California +# +# src/test/signals/Makefile +# +#------------------------------------------------------------------------- + +EXTRA_INSTALL=src/test/modules/injection_points + +subdir = src/test/signals +top_builddir = ../../.. +include $(top_builddir)/src/Makefile.global + +export enable_injection_points enable_injection_points + +check: + $(prove_check) + +installcheck: + $(prove_installcheck) + +clean distclean maintainer-clean: + rm -rf tmp_check diff --git a/src/test/signals/meson.build b/src/test/signals/meson.build new file mode 100644 index 0000000000..aec7437fb0 --- /dev/null +++ b/src/test/signals/meson.build @@ -0,0 +1,15 @@ +# Copyright (c) 2024-2024, PostgreSQL Global Development Group + +tests += { + 'name': 'signals', + 'sd': meson.current_source_dir(), + 'bd': meson.current_build_dir(), + 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, + 'tests': [ + 't/001_signal_autovacuum.pl', + ], + }, +} diff --git a/src/test/signals/t/001_signal_autovacuum.pl b/src/test/signals/t/001_signal_autovacuum.pl new file mode 100644 index 0000000000..fd5850958c --- /dev/null +++ b/src/test/signals/t/001_signal_autovacuum.pl @@ -0,0 +1,93 @@ +# Copyright (c) 2024-2024, PostgreSQL Global Development Group + +# Test signaling autovacuum worker backend by non-superuser role. +# Regular roles are tested to fail to signals autovacuum worker, roles +# with pg_signal_autovacuum_worker are tested to success this. +# Test uses an injection point to ensure that the worker is present +# for the whole duration of the test. + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Initialize postgres +my $psql_err = ''; +my $psql_out = ''; +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init(); +$node->append_conf( + 'postgresql.conf', 'autovacuum_naptime = 1 +'); +$node->start; +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +# Regular user and user with pg_signal_backend role cannot signal autovacuum worker +# User with pg_signal_backend can signal autovacuum worker +$node->safe_psql('postgres', qq( + CREATE ROLE regular_role; + CREATE ROLE signal_backend_role; + CREATE ROLE signal_autovacuum_worker_role; + GRANT pg_signal_backend TO signal_backend_role; + GRANT pg_signal_autovacuum_worker TO signal_autovacuum_worker_role; +)); + +# From this point, autovacuum worker will wait before doing any vacuum. +$node->safe_psql('postgres', + "SELECT injection_points_attach('autovacuum-worker-start', 'wait');"); + +# Create some content and set autovacuum setting such that it would be triggered. +$node->safe_psql('postgres', qq( + CREATE TABLE tab_int(i int); + ALTER TABLE tab_int SET (autovacuum_vacuum_cost_limit = 1); + ALTER TABLE tab_int SET (autovacuum_vacuum_cost_delay = 100); +)); + +$node->safe_psql('postgres', qq( + INSERT INTO tab_int VALUES(1); +)); + +# Wait until the autovacuum worker starts +$node->wait_for_event('autovacuum worker', 'autovacuum-worker-start'); + +my $av_pid = $node->safe_psql('postgres', qq( + SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum worker'; +)); + +# Regular user cannot terminate autovacuum worker +my $terminate_with_no_pg_signal_av = $node->psql('postgres', qq( + SET ROLE regular_role; + SELECT pg_terminate_backend($av_pid); +), stdout => \$psql_out, stderr => \$psql_err); + +ok($terminate_with_no_pg_signal_av != 0, "Terminating autovacuum worker should not succeed without pg_signal_autovacuum role"); +like($psql_err, qr/ERROR: permission denied to terminate autovacuum worker backend\nDETAIL: Only roles with the SUPERUSER attribute or with privileges of the "pg_signal_autovacuum_worker" role may terminate autovacuum worker backend/, + "Terminating autovacuum errors gracefully when role is not granted with pg_signal_autovacuum_worker"); + +my $offset = -s $node->logfile; + +# User with pg_signal_autovacuum can terminate autovacuum worker +my $terminate_with_pg_signal_av = $node->psql('postgres', qq( + SET ROLE signal_autovacuum_worker_role; + SELECT pg_terminate_backend($av_pid); +), stdout => \$psql_out, stderr => \$psql_err); + + +ok($terminate_with_pg_signal_av == 0, "Terminating autovacuum worker should succeed with pg_signal_autovacuum role"); + +# Check that the primary server logs a FATAL indicating that +# autovacuum is terminated. +ok($node->log_contains(qr/FATAL: terminating autovacuum process due to administrator command/, $offset), + "Autovacuum terminates when role is granted with pg_signal_autovacuum_worker"); + +# Release injection point. +$node->safe_psql('postgres', + "SELECT injection_point_detach('autovacuum-worker-start');"); + +done_testing(); -- 2.25.1 ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-11 14:06 Nathan Bossart <[email protected]> parent: Kirill Reshke <[email protected]> 3 siblings, 1 reply; 19+ messages in thread From: Nathan Bossart @ 2024-04-11 14:06 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Michael Paquier <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote: > Posting updated version of this patch with comments above addressed. I look for a commitfest entry for this one, but couldn't find it. Would you mind either creating one or, if I've somehow missed it, pointing me to the existing entry? https://commitfest.postgresql.org/48/ -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-11 14:38 Nathan Bossart <[email protected]> parent: Kirill Reshke <[email protected]> 3 siblings, 0 replies; 19+ messages in thread From: Nathan Bossart @ 2024-04-11 14:38 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Michael Paquier <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote: >> It's feeling more natural here to check that we have a superuser-owned >> backend first, and then do a lookup of the process type. > >> I figured since there's no reason to rely on that behavior, we might as >> well do a bit of future-proofing in case autovacuum workers are ever not >> run as InvalidOid. > > I have combined them into this: > > if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) > && pgstat_get_backend_type(GetNumberFromPGProc(proc)) == B_AUTOVAC_WORKER) > > This is both future-proofing and natural, I suppose. Downside of this > is double checking condition (!OidIsValid(proc->roleId) || > superuser_arg(proc->roleId)), but i think that is ok for the sake of > simplicity. If we want to retain the check, IMO we might as well combine the first two blocks like Anthony proposed: if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) { ProcNumber procNumber = GetNumberFromPGProc(proc); PGBackendStatus procStatus = pgstat_get_beentry_by_proc_number(procNumber); if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER && !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVAC_WORKER)) return SIGNAL_BACKEND_NOAUTOVAC; else if (!superuser()) return SIGNAL_BACKEND_NOSUPERUSER; } + <row> + <entry>pg_signal_autovacuum_worker</entry> + <entry>Allow signaling autovacuum worker backend to cancel or terminate</entry> + </row> I think we need to be more specific about what pg_cancel_backend() and pg_terminate_backend() do for autovacuum workers. The code offers some clues: /* * SIGINT is used to signal canceling the current table's vacuum; SIGTERM * means abort and exit cleanly, and SIGQUIT means abandon ship. */ pqsignal(SIGINT, StatementCancelHandler); pqsignal(SIGTERM, die); +/* ---------- + * pgstat_get_backend_type() - + * + * Return the backend type of the backend for the given proc number. + * ---------- + */ +BackendType +pgstat_get_backend_type(ProcNumber procNumber) +{ + PgBackendStatus *ret; + + ret = pgstat_get_beentry_by_proc_number(procNumber); + + if (!ret) + return B_INVALID; + + return ret->st_backendType; +} I'm not sure we really need to introduce a new function for this. I avoided using it in my example snippet above. But, maybe it'll come in handy down the road... -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-11 15:20 Kirill Reshke <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Kirill Reshke @ 2024-04-11 15:20 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Thu, 11 Apr 2024 at 19:07, Nathan Bossart <[email protected]> wrote: > > On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote: > > Posting updated version of this patch with comments above addressed. > > I look for a commitfest entry for this one, but couldn't find it. Would > you mind either creating one Done: https://commitfest.postgresql.org/48/4922/ ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-12 00:10 Michael Paquier <[email protected]> parent: Kirill Reshke <[email protected]> 3 siblings, 1 reply; 19+ messages in thread From: Michael Paquier @ 2024-04-12 00:10 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote: > The test doesn't fail because pg_terminate_backend actually meets his > point: autovac is killed. But while dying, autovac also receives > segfault. Thats because of injections points: > > (gdb) bt > #0 0x000056361c3379ea in tas (lock=0x7fbcb9632224 <error: Cannot > access memory at address 0x7fbcb9632224>) at > ../../../../src/include/storage/s_lock.h:228 > #1 ConditionVariableCancelSleep () at condition_variable.c:238 > #2 0x000056361c337e4b in ConditionVariableBroadcast > (cv=0x7fbcb66f498c) at condition_variable.c:310 > #3 0x000056361c330a40 in CleanupProcSignalState (status=<optimized > out>, arg=<optimized out>) at procsignal.c:240 > #4 0x000056361c328801 in shmem_exit (code=code@entry=1) at ipc.c:276 > #5 0x000056361c3288fc in proc_exit_prepare (code=code@entry=1) at ipc.c:198 > #6 0x000056361c3289bf in proc_exit (code=code@entry=1) at ipc.c:111 > #7 0x000056361c49ffa8 in errfinish (filename=<optimized out>, > lineno=<optimized out>, funcname=0x56361c654370 <__func__.16> > "ProcessInterrupts") at elog.c:592 > #8 0x000056361bf7191e in ProcessInterrupts () at postgres.c:3264 > #9 0x000056361c3378d7 in ConditionVariableTimedSleep > (cv=0x7fbcb9632224, timeout=timeout@entry=-1, > wait_event_info=117440513) at condition_variable.c:196 > #10 0x000056361c337d0b in ConditionVariableTimedSleep > (wait_event_info=<optimized out>, timeout=-1, cv=<optimized out>) at > condition_variable.c:135 > #11 ConditionVariableSleep (cv=<optimized out>, > wait_event_info=<optimized out>) at condition_variable.c:98 > > discovered because of > # Release injection point. > $node->safe_psql('postgres', > "SELECT injection_point_detach('autovacuum-worker-start');"); > added > > v4 also suffers from that. i will try to fix that. I can see this stack trace as well. Capturing a bit more than your own stack, this is crashing in the autovacuum worker while waiting on a condition variable when processing a ProcessInterrupts(). That may point to a legit bug with condition variables in this context, actually? From what I can see, issuing a signal on a backend process waiting with a condition variable is able to process the interrupt correctly. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-12 02:01 Kyotaro Horiguchi <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Kyotaro Horiguchi @ 2024-04-12 02:01 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; [email protected]; [email protected]; pgsql-hackers At Fri, 12 Apr 2024 09:10:35 +0900, Michael Paquier <[email protected]> wrote in > On Thu, Apr 11, 2024 at 04:55:59PM +0500, Kirill Reshke wrote: > > The test doesn't fail because pg_terminate_backend actually meets his > > point: autovac is killed. But while dying, autovac also receives > > segfault. Thats because of injections points: > > > > (gdb) bt > > #0 0x000056361c3379ea in tas (lock=0x7fbcb9632224 <error: Cannot > > access memory at address 0x7fbcb9632224>) at > > ../../../../src/include/storage/s_lock.h:228 > > #1 ConditionVariableCancelSleep () at condition_variable.c:238 ... > > #3 0x000056361c330a40 in CleanupProcSignalState (status=<optimized out>, arg=<optimized out>) at procsignal.c:240 > > #4 0x000056361c328801 in shmem_exit (code=code@entry=1) at ipc.c:276 > > #9 0x000056361c3378d7 in ConditionVariableTimedSleep > > (cv=0x7fbcb9632224, timeout=timeout@entry=-1, ... > I can see this stack trace as well. Capturing a bit more than your > own stack, this is crashing in the autovacuum worker while waiting on > a condition variable when processing a ProcessInterrupts(). > > That may point to a legit bug with condition variables in this > context, actually? From what I can see, issuing a signal on a backend > process waiting with a condition variable is able to process the > interrupt correctly. ProcSignalInit sets up CleanupProcSignalState to be called via on_shmem_exit. If the CV is allocated in a dsm segment, shmem_exit should have detached the region for the CV. CV cleanup code should be invoked via before_shmem_exit. regards. -- Kyotaro Horiguchi NTT Open Source Software Center ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-12 08:32 Kirill Reshke <[email protected]> parent: Kirill Reshke <[email protected]> 3 siblings, 1 reply; 19+ messages in thread From: Kirill Reshke @ 2024-04-12 08:32 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Thu, 11 Apr 2024 at 16:55, Kirill Reshke <[email protected]> wrote: > 7) > > > +# Copyright (c) 2022-2024, PostgreSQL Global Development Group > > [...] > > +# Copyright (c) 2024-2024, PostgreSQL Global Development Group > > > These need to be cleaned up. > > > +# Makefile for src/test/recovery > > +# > > +# src/test/recovery/Makefile > > > This is incorrect, twice. > > Cleaned up, thanks! Oh, wait, I did this wrong. Should i use +# Copyright (c) 2024-2024, PostgreSQL Global Development Group (Like in src/test/signals/meson.build & src/test/signals/t/001_signal_autovacuum.pl) or +# +# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group +# Portions Copyright (c) 1994, Regents of the University of California +# (Like in src/test/signals/Makefile) at the beginning of each added file? ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-04-15 04:47 Michael Paquier <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Michael Paquier @ 2024-04-15 04:47 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers On Fri, Apr 12, 2024 at 01:32:42PM +0500, Kirill Reshke wrote: > +# > +# Portions Copyright (c) 1996-2024, PostgreSQL Global Development Group > +# Portions Copyright (c) 1994, Regents of the University of California > +# > (Like in src/test/signals/Makefile) > > at the beginning of each added file? Assuming that these files are merged in 2024, you could just use: Copyright (c) 2024, PostgreSQL Global Development Group See for example slotsync.c introduced recently in commit ddd5f4f54a02. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-12 21:04 Nathan Bossart <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Nathan Bossart @ 2024-06-12 21:04 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; Andrey M. Borodin <[email protected]>; pgsql-hackers I adjusted 0001 based on my upthread feedback. -- nathan From 118f95346fcf8099ab28d2f9186185171e3b88af Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 12 Jun 2024 15:38:14 -0500 Subject: [PATCH v5 1/1] Introduce pg_signal_autovacuum_worker. Since commit 3a9b18b309, roles with privileges of pg_signal_backend cannot signal autovacuum workers. Many users treated the ability to signal autovacuum workers as a feature instead of a bug, so we are reintroducing it via a new predefined role. Having privileges of this new role, named pg_signal_autovacuum_worker, only permits signaling autovacuum workers. It does not permit signaling other types of superuser backends. Author: Kirill Reshke Reviewed-by: Anthony Leung, Michael Paquier Discussion: https://postgr.es/m/CALdSSPhC4GGmbnugHfB9G0%3DfAxjCSug_-rmL9oUh0LTxsyBfsg%40mail.gmail.com --- doc/src/sgml/func.sgml | 8 +++-- doc/src/sgml/user-manag.sgml | 5 +++ src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------ src/include/catalog/pg_authid.dat | 5 +++ 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 17c44bc338..7d5b7acb68 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -28037,7 +28037,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Cancels the current query of the session whose backend process has the specified process ID. This is also allowed if the - calling role is a member of the role whose backend is being canceled or + calling role has privileges of the role whose backend is being canceled, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can cancel superuser backends. </para></entry> @@ -28111,7 +28113,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Terminates the session whose backend process has the specified process ID. This is also allowed if the calling role - is a member of the role whose backend is being terminated or the + has privileges of the role whose backend is being terminated, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can terminate superuser backends. </para> diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index 07a16247d7..340cefff70 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -661,6 +661,11 @@ DROP ROLE doomed_role; <entry>pg_signal_backend</entry> <entry>Signal another backend to cancel a query or terminate its session.</entry> </row> + <row> + <entry>pg_signal_autovacuum_worker</entry> + <entry>Signal an autovacuum worker to cancel the current table's vacuum + or terminate its session.</entry> + </row> <row> <entry>pg_read_server_files</entry> <entry>Allow reading files from any location the database can access on the server with COPY and diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index 88e9bf8125..780a1c682a 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -34,8 +34,9 @@ * role as the backend being signaled. For "dangerous" signals, an explicit * check for superuser needs to be done prior to calling this function. * - * Returns 0 on success, 1 on general failure, 2 on normal permission error - * and 3 if the caller needs to be a superuser. + * Returns 0 on success, 1 on general failure, 2 on normal permission error, + * 3 if the caller needs to be a superuser, and 4 if the caller needs to have + * privileges of pg_signal_autovacuum_worker. * * In the event of a general failure (return code 1), a warning message will * be emitted. For permission errors, doing that is the responsibility of @@ -45,6 +46,7 @@ #define SIGNAL_BACKEND_ERROR 1 #define SIGNAL_BACKEND_NOPERMISSION 2 #define SIGNAL_BACKEND_NOSUPERUSER 3 +#define SIGNAL_BACKEND_NOAUTOVAC 4 static int pg_signal_backend(int pid, int sig) { @@ -77,15 +79,27 @@ pg_signal_backend(int pid, int sig) /* * 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. + * backend, so treat it that way. As an exception, we allow roles with + * privileges of pg_signal_autovacuum_worker to signal autovacuum workers + * (which do not advertise a role). + * + * Otherwise, users can signal backends for roles they have privileges of. */ - if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && - !superuser()) - return SIGNAL_BACKEND_NOSUPERUSER; + if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) + { + ProcNumber procNumber = GetNumberFromPGProc(proc); + PgBackendStatus *procStatus = pgstat_get_beentry_by_proc_number(procNumber); - /* 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)) + if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER) + { + if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER)) + return SIGNAL_BACKEND_NOAUTOVAC; + } + else if (!superuser()) + return SIGNAL_BACKEND_NOSUPERUSER; + } + else if (!has_privs_of_role(GetUserId(), proc->roleId) && + !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) return SIGNAL_BACKEND_NOPERMISSION; /* @@ -130,6 +144,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may cancel queries of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to cancel query"), + errdetail("Only roles with privileges of the \"%s\" role may cancel queries of autovacuum workers.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -236,6 +257,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to terminate process"), + errdetail("Only roles with privileges of the \"%s\" role may terminate autovacuum worker processes.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index bf00815c14..0129f67eaa 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -99,5 +99,10 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER', + rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] -- 2.39.3 (Apple Git-146) Attachments: [text/plain] v5-0001-Introduce-pg_signal_autovacuum_worker.patch (7.8K, ../../ZmoNRuO9-QlLUjRu@nathan/2-v5-0001-Introduce-pg_signal_autovacuum_worker.patch) download | inline diff: From 118f95346fcf8099ab28d2f9186185171e3b88af Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 12 Jun 2024 15:38:14 -0500 Subject: [PATCH v5 1/1] Introduce pg_signal_autovacuum_worker. Since commit 3a9b18b309, roles with privileges of pg_signal_backend cannot signal autovacuum workers. Many users treated the ability to signal autovacuum workers as a feature instead of a bug, so we are reintroducing it via a new predefined role. Having privileges of this new role, named pg_signal_autovacuum_worker, only permits signaling autovacuum workers. It does not permit signaling other types of superuser backends. Author: Kirill Reshke Reviewed-by: Anthony Leung, Michael Paquier Discussion: https://postgr.es/m/CALdSSPhC4GGmbnugHfB9G0%3DfAxjCSug_-rmL9oUh0LTxsyBfsg%40mail.gmail.com --- doc/src/sgml/func.sgml | 8 +++-- doc/src/sgml/user-manag.sgml | 5 +++ src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------ src/include/catalog/pg_authid.dat | 5 +++ 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 17c44bc338..7d5b7acb68 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -28037,7 +28037,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Cancels the current query of the session whose backend process has the specified process ID. This is also allowed if the - calling role is a member of the role whose backend is being canceled or + calling role has privileges of the role whose backend is being canceled, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can cancel superuser backends. </para></entry> @@ -28111,7 +28113,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Terminates the session whose backend process has the specified process ID. This is also allowed if the calling role - is a member of the role whose backend is being terminated or the + has privileges of the role whose backend is being terminated, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can terminate superuser backends. </para> diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index 07a16247d7..340cefff70 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -661,6 +661,11 @@ DROP ROLE doomed_role; <entry>pg_signal_backend</entry> <entry>Signal another backend to cancel a query or terminate its session.</entry> </row> + <row> + <entry>pg_signal_autovacuum_worker</entry> + <entry>Signal an autovacuum worker to cancel the current table's vacuum + or terminate its session.</entry> + </row> <row> <entry>pg_read_server_files</entry> <entry>Allow reading files from any location the database can access on the server with COPY and diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index 88e9bf8125..780a1c682a 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -34,8 +34,9 @@ * role as the backend being signaled. For "dangerous" signals, an explicit * check for superuser needs to be done prior to calling this function. * - * Returns 0 on success, 1 on general failure, 2 on normal permission error - * and 3 if the caller needs to be a superuser. + * Returns 0 on success, 1 on general failure, 2 on normal permission error, + * 3 if the caller needs to be a superuser, and 4 if the caller needs to have + * privileges of pg_signal_autovacuum_worker. * * In the event of a general failure (return code 1), a warning message will * be emitted. For permission errors, doing that is the responsibility of @@ -45,6 +46,7 @@ #define SIGNAL_BACKEND_ERROR 1 #define SIGNAL_BACKEND_NOPERMISSION 2 #define SIGNAL_BACKEND_NOSUPERUSER 3 +#define SIGNAL_BACKEND_NOAUTOVAC 4 static int pg_signal_backend(int pid, int sig) { @@ -77,15 +79,27 @@ pg_signal_backend(int pid, int sig) /* * 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. + * backend, so treat it that way. As an exception, we allow roles with + * privileges of pg_signal_autovacuum_worker to signal autovacuum workers + * (which do not advertise a role). + * + * Otherwise, users can signal backends for roles they have privileges of. */ - if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && - !superuser()) - return SIGNAL_BACKEND_NOSUPERUSER; + if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) + { + ProcNumber procNumber = GetNumberFromPGProc(proc); + PgBackendStatus *procStatus = pgstat_get_beentry_by_proc_number(procNumber); - /* 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)) + if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER) + { + if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER)) + return SIGNAL_BACKEND_NOAUTOVAC; + } + else if (!superuser()) + return SIGNAL_BACKEND_NOSUPERUSER; + } + else if (!has_privs_of_role(GetUserId(), proc->roleId) && + !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) return SIGNAL_BACKEND_NOPERMISSION; /* @@ -130,6 +144,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may cancel queries of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to cancel query"), + errdetail("Only roles with privileges of the \"%s\" role may cancel queries of autovacuum workers.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -236,6 +257,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to terminate process"), + errdetail("Only roles with privileges of the \"%s\" role may terminate autovacuum worker processes.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index bf00815c14..0129f67eaa 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -99,5 +99,10 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER', + rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-14 07:06 Andrey M. Borodin <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 2 replies; 19+ messages in thread From: Andrey M. Borodin @ 2024-06-14 07:06 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Michael Paquier <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers > On 13 Jun 2024, at 02:04, Nathan Bossart <[email protected]> wrote: > > I adjusted 0001 based on my upthread feedback. This patch looks good to me. Spellchecker is complaining about “signaling” instead of “signalling”, but ISTM it’s OK. I’ve tried to dig into the test. The problem is CV is allocated in inj_state = GetNamedDSMSegment("injection_points”, which seems to be destroyed in shmem_exit() calling dsm_backend_shutdown() This happens before we broadcast that sleep is over. I think this might happen with any wait on injection point if it is pg_terminate_backend()ed. Is there way to wake up from CV sleep before processing actual termination? Thanks! Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-14 20:12 Nathan Bossart <[email protected]> parent: Andrey M. Borodin <[email protected]> 1 sibling, 1 reply; 19+ messages in thread From: Nathan Bossart @ 2024-06-14 20:12 UTC (permalink / raw) To: Andrey M. Borodin <[email protected]>; +Cc: Michael Paquier <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote: > This patch looks good to me. Thanks for looking. > Spellchecker is complaining about "signaling" instead of "signalling", > but ISTM it´s OK. I think this is an en-US versus en-GB thing. We've standardized on en-US for "cancel" (see commits 8c9da14, 21f1e15, and af26857), so IMO we might as well do so for "signal," too. -- nathan ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-21 04:01 Michael Paquier <[email protected]> parent: Andrey M. Borodin <[email protected]> 1 sibling, 1 reply; 19+ messages in thread From: Michael Paquier @ 2024-06-21 04:01 UTC (permalink / raw) To: Andrey M. Borodin <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote: > I’ve tried to dig into the test. > The problem is CV is allocated in > > inj_state = GetNamedDSMSegment("injection_points”, > > which seems to be destroyed in > > shmem_exit() calling dsm_backend_shutdown() > > This happens before we broadcast that sleep is over. > I think this might happen with any wait on injection point if it is > pg_terminate_backend()ed. Except if I am missing something, this is not a problem for a normal backend, for example with one using a `SELECT injection_points_run()`. > Is there way to wake up from CV sleep before processing actual termination? I am honestly not sure if this is worth complicating the sigjmp path of the autovacuum worker just for the sake of this test. It seems to me that it would be simple enough to move the injection point autovacuum-worker-start within the transaction block a few lines down in do_autovacuum(), no? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-21 05:31 Andrey M. Borodin <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 19+ messages in thread From: Andrey M. Borodin @ 2024-06-21 05:31 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers > On 21 Jun 2024, at 09:01, Michael Paquier <[email protected]> wrote: > > On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote: >> I’ve tried to dig into the test. >> The problem is CV is allocated in >> >> inj_state = GetNamedDSMSegment("injection_points”, >> >> which seems to be destroyed in >> >> shmem_exit() calling dsm_backend_shutdown() >> >> This happens before we broadcast that sleep is over. >> I think this might happen with any wait on injection point if it is >> pg_terminate_backend()ed. > > Except if I am missing something, this is not a problem for a normal > backend, for example with one using a `SELECT injection_points_run()`. Yes, i’ve tried to get similar error in other CV-sleeps and in injection points of normal backend - everything works just fine. The error is specific to just this test. >> Is there way to wake up from CV sleep before processing actual termination? > > I am honestly not sure if this is worth complicating the sigjmp path > of the autovacuum worker just for the sake of this test. It seems to > me that it would be simple enough to move the injection point > autovacuum-worker-start within the transaction block a few lines down > in do_autovacuum(), no? Thanks for the pointer, I’ll try this approach! Best regards, Andrey Borodin, ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-21 05:36 Michael Paquier <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Michael Paquier @ 2024-06-21 05:36 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andrey M. Borodin <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers On Fri, Jun 14, 2024 at 03:12:50PM -0500, Nathan Bossart wrote: > On Fri, Jun 14, 2024 at 12:06:36PM +0500, Andrey M. Borodin wrote: > > This patch looks good to me. > > Thanks for looking. While double-checking the whole, where I don't have much to say about 0001, I have fixed a few issues with the test presented upthread and stabilized it (CI and my stuff are both OK). I'd suggest to move it to test_misc/, because there is no clear category where to put it, and we have another test with injection points there for timeouts so the module dependency with EXTRA_INSTALL is already cleared. What do you think? -- Michael Attachments: [text/x-diff] v6-0001-Introduce-pg_signal_autovacuum_worker.patch (7.8K, ../../[email protected]/2-v6-0001-Introduce-pg_signal_autovacuum_worker.patch) download | inline diff: From 2d3e219149b8dbdbaf2b465f619f898c61969b17 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 12 Jun 2024 15:38:14 -0500 Subject: [PATCH v6 1/2] Introduce pg_signal_autovacuum_worker. Since commit 3a9b18b309, roles with privileges of pg_signal_backend cannot signal autovacuum workers. Many users treated the ability to signal autovacuum workers as a feature instead of a bug, so we are reintroducing it via a new predefined role. Having privileges of this new role, named pg_signal_autovacuum_worker, only permits signaling autovacuum workers. It does not permit signaling other types of superuser backends. Author: Kirill Reshke Reviewed-by: Anthony Leung, Michael Paquier Discussion: https://postgr.es/m/CALdSSPhC4GGmbnugHfB9G0%3DfAxjCSug_-rmL9oUh0LTxsyBfsg%40mail.gmail.com --- src/include/catalog/pg_authid.dat | 5 +++ src/backend/storage/ipc/signalfuncs.c | 46 +++++++++++++++++++++------ doc/src/sgml/func.sgml | 8 +++-- doc/src/sgml/user-manag.sgml | 5 +++ 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat index bf00815c14..0129f67eaa 100644 --- a/src/include/catalog/pg_authid.dat +++ b/src/include/catalog/pg_authid.dat @@ -99,5 +99,10 @@ rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', rolpassword => '_null_', rolvaliduntil => '_null_' }, +{ oid => '8916', oid_symbol => 'ROLE_PG_SIGNAL_AUTOVACUUM_WORKER', + rolname => 'pg_signal_autovacuum_worker', rolsuper => 'f', rolinherit => 't', + rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f', + rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1', + rolpassword => '_null_', rolvaliduntil => '_null_' }, ] diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c index 88e9bf8125..780a1c682a 100644 --- a/src/backend/storage/ipc/signalfuncs.c +++ b/src/backend/storage/ipc/signalfuncs.c @@ -34,8 +34,9 @@ * role as the backend being signaled. For "dangerous" signals, an explicit * check for superuser needs to be done prior to calling this function. * - * Returns 0 on success, 1 on general failure, 2 on normal permission error - * and 3 if the caller needs to be a superuser. + * Returns 0 on success, 1 on general failure, 2 on normal permission error, + * 3 if the caller needs to be a superuser, and 4 if the caller needs to have + * privileges of pg_signal_autovacuum_worker. * * In the event of a general failure (return code 1), a warning message will * be emitted. For permission errors, doing that is the responsibility of @@ -45,6 +46,7 @@ #define SIGNAL_BACKEND_ERROR 1 #define SIGNAL_BACKEND_NOPERMISSION 2 #define SIGNAL_BACKEND_NOSUPERUSER 3 +#define SIGNAL_BACKEND_NOAUTOVAC 4 static int pg_signal_backend(int pid, int sig) { @@ -77,15 +79,27 @@ pg_signal_backend(int pid, int sig) /* * 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. + * backend, so treat it that way. As an exception, we allow roles with + * privileges of pg_signal_autovacuum_worker to signal autovacuum workers + * (which do not advertise a role). + * + * Otherwise, users can signal backends for roles they have privileges of. */ - if ((!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) && - !superuser()) - return SIGNAL_BACKEND_NOSUPERUSER; + if (!OidIsValid(proc->roleId) || superuser_arg(proc->roleId)) + { + ProcNumber procNumber = GetNumberFromPGProc(proc); + PgBackendStatus *procStatus = pgstat_get_beentry_by_proc_number(procNumber); - /* 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)) + if (procStatus && procStatus->st_backendType == B_AUTOVAC_WORKER) + { + if (!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_AUTOVACUUM_WORKER)) + return SIGNAL_BACKEND_NOAUTOVAC; + } + else if (!superuser()) + return SIGNAL_BACKEND_NOSUPERUSER; + } + else if (!has_privs_of_role(GetUserId(), proc->roleId) && + !has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND)) return SIGNAL_BACKEND_NOPERMISSION; /* @@ -130,6 +144,13 @@ pg_cancel_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may cancel queries of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to cancel query"), + errdetail("Only roles with privileges of the \"%s\" role may cancel queries of autovacuum workers.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), @@ -236,6 +257,13 @@ pg_terminate_backend(PG_FUNCTION_ARGS) errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.", "SUPERUSER", "SUPERUSER"))); + if (r == SIGNAL_BACKEND_NOAUTOVAC) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied to terminate process"), + errdetail("Only roles with privileges of the \"%s\" role may terminate autovacuum worker processes.", + "pg_signal_autovacuum_worker"))); + if (r == SIGNAL_BACKEND_NOPERMISSION) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 2609269610..f73ba439c0 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -28040,7 +28040,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Cancels the current query of the session whose backend process has the specified process ID. This is also allowed if the - calling role is a member of the role whose backend is being canceled or + calling role has privileges of the role whose backend is being canceled, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can cancel superuser backends. </para></entry> @@ -28114,7 +28116,9 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id')); <para> Terminates the session whose backend process has the specified process ID. This is also allowed if the calling role - is a member of the role whose backend is being terminated or the + has privileges of the role whose backend is being terminated, + the backend process is an autovacuum worker and the calling role has + privileges of <literal>pg_signal_autovacuum_worker</literal>, or the calling role has privileges of <literal>pg_signal_backend</literal>, however only superusers can terminate superuser backends. </para> diff --git a/doc/src/sgml/user-manag.sgml b/doc/src/sgml/user-manag.sgml index 07a16247d7..340cefff70 100644 --- a/doc/src/sgml/user-manag.sgml +++ b/doc/src/sgml/user-manag.sgml @@ -661,6 +661,11 @@ DROP ROLE doomed_role; <entry>pg_signal_backend</entry> <entry>Signal another backend to cancel a query or terminate its session.</entry> </row> + <row> + <entry>pg_signal_autovacuum_worker</entry> + <entry>Signal an autovacuum worker to cancel the current table's vacuum + or terminate its session.</entry> + </row> <row> <entry>pg_read_server_files</entry> <entry>Allow reading files from any location the database can access on the server with COPY and -- 2.45.2 [text/x-diff] v6-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch (4.6K, ../../[email protected]/3-v6-0002-Add-tap-test-for-pg_signal_autovacuum-role.patch) download | inline diff: From 93b7d69585aa4a408749bee6239006d6494df1bb Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Fri, 21 Jun 2024 14:29:23 +0900 Subject: [PATCH v6 2/2] Add tap test for pg_signal_autovacuum role --- src/backend/postmaster/autovacuum.c | 7 ++ .../test_misc/t/006_signal_autovacuum.pl | 100 ++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 src/test/modules/test_misc/t/006_signal_autovacuum.pl diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 9a925a10cd..0d4e2c5140 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -100,6 +100,7 @@ #include "utils/fmgroids.h" #include "utils/fmgrprotos.h" #include "utils/guc_hooks.h" +#include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/ps_status.h" @@ -1902,6 +1903,12 @@ do_autovacuum(void) /* Start a transaction so our commands have one to play into. */ StartTransactionCommand(); + /* + * This injection point is put in a transaction block to work with a wait + * that uses a condition variable. + */ + INJECTION_POINT("autovacuum-worker-start"); + /* * Compute the multixact age for which freezing is urgent. This is * normally autovacuum_multixact_freeze_max_age, but may be less if we are diff --git a/src/test/modules/test_misc/t/006_signal_autovacuum.pl b/src/test/modules/test_misc/t/006_signal_autovacuum.pl new file mode 100644 index 0000000000..98cbea3744 --- /dev/null +++ b/src/test/modules/test_misc/t/006_signal_autovacuum.pl @@ -0,0 +1,100 @@ +# Copyright (c) 2024, PostgreSQL Global Development Group + +# Test signaling autovacuum worker backend by non-superuser role. +# +# Only non-superuser roles granted pg_signal_autovacuum_worker are allowed +# to signal autovacuum workers. This test uses an injection point located +# at the beginning of the autovacuum worker startup. + +use strict; +use warnings; +use PostgreSQL::Test::Cluster; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Initialize postgres +my $psql_err = ''; +my $psql_out = ''; +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init; + +# This ensures a quick worker spawn. +$node->append_conf( + 'postgresql.conf', 'autovacuum_naptime = 1'); +$node->start; +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +$node->safe_psql( + 'postgres', qq( + CREATE ROLE regular_role; + CREATE ROLE signal_autovacuum_worker_role; + GRANT pg_signal_autovacuum_worker TO signal_autovacuum_worker_role; +)); + +# From this point, autovacuum worker will wait at startup. +$node->safe_psql('postgres', + "SELECT injection_points_attach('autovacuum-worker-start', 'wait');"); + +# Create some content and set an aggressive autovacuum. +$node->safe_psql( + 'postgres', qq( + CREATE TABLE tab_int(i int); + ALTER TABLE tab_int SET (autovacuum_vacuum_cost_limit = 1); + ALTER TABLE tab_int SET (autovacuum_vacuum_cost_delay = 100); +)); + +$node->safe_psql( + 'postgres', qq( + INSERT INTO tab_int VALUES(1); +)); + +# Wait until an autovacuum worker starts. +$node->wait_for_event('autovacuum worker', 'autovacuum-worker-start'); + +my $av_pid = $node->safe_psql( + 'postgres', qq( + SELECT pid FROM pg_stat_activity WHERE backend_type = 'autovacuum worker'; +)); + +# Regular role cannot terminate autovacuum worker. +my $terminate_with_no_pg_signal_av = $node->psql( + 'postgres', qq( + SET ROLE regular_role; + SELECT pg_terminate_backend($av_pid); +), + stdout => \$psql_out, + stderr => \$psql_err); + +like( + $psql_err, + qr/ERROR: permission denied to terminate process\nDETAIL: Only roles with privileges of the "pg_signal_autovacuum_worker" role may terminate autovacuum worker processes./, + "autovacuum worker not signaled with regular role"); + +my $offset = -s $node->logfile; + +# Role with pg_signal_autovacuum can terminate autovacuum worker. +my $terminate_with_pg_signal_av = $node->psql( + 'postgres', qq( + SET ROLE signal_autovacuum_worker_role; + SELECT pg_terminate_backend($av_pid); +), + stdout => \$psql_out, + stderr => \$psql_err); + +# Check that the primary server logs a FATAL indicating that autovacuum +# is terminated. +ok( $node->log_contains( + qr/FATAL: terminating autovacuum process due to administrator command/, + $offset), + "autovacuum worker signaled with pg_signal_autovacuum_worker granted" +); + +# Release injection point. +$node->safe_psql('postgres', + "SELECT injection_points_detach('autovacuum-worker-start');"); + +done_testing(); -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/4-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
* Re: Allow non-superuser to cancel superuser tasks. @ 2024-06-21 05:38 Michael Paquier <[email protected]> parent: Andrey M. Borodin <[email protected]> 0 siblings, 0 replies; 19+ messages in thread From: Michael Paquier @ 2024-06-21 05:38 UTC (permalink / raw) To: Andrey M. Borodin <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Kirill Reshke <[email protected]>; Leung, Anthony <[email protected]>; pgsql-hackers On Fri, Jun 21, 2024 at 10:31:30AM +0500, Andrey M. Borodin wrote: > Thanks for the pointer, I’ll try this approach! Thanks. FWIW, I've put my mind into it, and fixed the thing a few minutes ago: https://www.postgresql.org/message-id/ZnURUaujl39wSoEW%40paquier.xyz -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 19+ messages in thread
end of thread, other threads:[~2024-06-21 05:38 UTC | newest] Thread overview: 19+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-11-08 06:57 [PATCH v11 5/7] Row pattern recognition patch (docs). Tatsuo Ishii <[email protected]> 2024-04-09 22:58 Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-04-10 15:00 ` Re: Allow non-superuser to cancel superuser tasks. Nathan Bossart <[email protected]> 2024-04-10 22:21 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-04-11 11:55 ` Re: Allow non-superuser to cancel superuser tasks. Kirill Reshke <[email protected]> 2024-04-11 14:06 ` Re: Allow non-superuser to cancel superuser tasks. Nathan Bossart <[email protected]> 2024-04-11 15:20 ` Re: Allow non-superuser to cancel superuser tasks. Kirill Reshke <[email protected]> 2024-04-11 14:38 ` Re: Allow non-superuser to cancel superuser tasks. Nathan Bossart <[email protected]> 2024-04-12 00:10 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-04-12 02:01 ` Re: Allow non-superuser to cancel superuser tasks. Kyotaro Horiguchi <[email protected]> 2024-04-12 08:32 ` Re: Allow non-superuser to cancel superuser tasks. Kirill Reshke <[email protected]> 2024-04-15 04:47 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-06-12 21:04 ` Re: Allow non-superuser to cancel superuser tasks. Nathan Bossart <[email protected]> 2024-06-14 07:06 ` Re: Allow non-superuser to cancel superuser tasks. Andrey M. Borodin <[email protected]> 2024-06-14 20:12 ` Re: Allow non-superuser to cancel superuser tasks. Nathan Bossart <[email protected]> 2024-06-21 05:36 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-06-21 04:01 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]> 2024-06-21 05:31 ` Re: Allow non-superuser to cancel superuser tasks. Andrey M. Borodin <[email protected]> 2024-06-21 05:38 ` Re: Allow non-superuser to cancel superuser tasks. Michael Paquier <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox