Re: TerminateOtherDBBackends code comments inconsistency.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote: > On Tue, Apr 30, 2024 at 2:58 AM Noah Misch wrote: > > On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote: > > > 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever > > > is mentioned w.r.t permissions in the doc of that function sounds > > > valid for drop database force to me. Do you have any specific proposal > > > in your mind? > > > > Something like the attached. > > LGTM. Pushed as commit 372700c. Thanks for the report and the review.
Re: TerminateOtherDBBackends code comments inconsistency.
On Tue, Apr 30, 2024 at 10:36 PM Noah Misch wrote: > > > > > > One could argue the function should also check > > > isBackgroundWorker and ignore even bgworkers that set proc->roleId, but > > > I've > > > not done that. > > > > What is the argument for ignoring such workers? > > One of the proposed code comments says, "For bgworker authors, it's convenient > to be able to recommend FORCE if a worker is blocking DROP DATABASE > unexpectedly." That argument is debatable, but I do think it applies equally > to bgworkers whether or not they set proc->roleId. > Agreed. -- With Regards, Amit Kapila.
Re: TerminateOtherDBBackends code comments inconsistency.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote: > On Tue, Apr 30, 2024 at 2:58 AM Noah Misch wrote: > > On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote: > > > On Mon, Apr 22, 2024 at 9:56 PM Noah Misch wrote: > > > > > 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever > > > is mentioned w.r.t permissions in the doc of that function sounds > > > valid for drop database force to me. Do you have any specific proposal > > > in your mind? > > > > Something like the attached. > > LGTM. > > > One could argue the function should also check > > isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've > > not done that. > > What is the argument for ignoring such workers? One of the proposed code comments says, "For bgworker authors, it's convenient to be able to recommend FORCE if a worker is blocking DROP DATABASE unexpectedly." That argument is debatable, but I do think it applies equally to bgworkers whether or not they set proc->roleId.
Re: TerminateOtherDBBackends code comments inconsistency.
On Tue, Apr 30, 2024 at 2:58 AM Noah Misch wrote: > > On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote: > > On Mon, Apr 22, 2024 at 9:56 PM Noah Misch wrote: > > > 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever > > is mentioned w.r.t permissions in the doc of that function sounds > > valid for drop database force to me. Do you have any specific proposal > > in your mind? > > Something like the attached. > LGTM. > One could argue the function should also check > isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've > not done that. What is the argument for ignoring such workers? -- With Regards, Amit Kapila.
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote: > On Mon, Apr 22, 2024 at 9:56 PM Noah Misch wrote: > > > > On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke > > > wrote: > > > > > > > > While working on [0] i have noticed this comment in > > > > TerminateOtherDBBackends function: > > > > > > > > /* > > > > * Check whether we have the necessary rights to terminate other > > > > * sessions. We don't terminate any session until we ensure that we > > > > * have rights on all the sessions to be terminated. These checks are > > > > * the same as we do in pg_terminate_backend. > > > > * > > > > * In this case we don't raise some warnings - like "PID %d is not a > > > > * PostgreSQL server process", because for us already finished session > > > > * is not a problem. > > > > */ > > > > > > > > This statement is not true after 3a9b18b. > > > > "These checks are the same as we do in pg_terminate_backend." > > > > The comment mismatch is a problem. Thanks for reporting it. The DROP > > DATABASE doc mimics the comment, using, "permissions are the same as with > > pg_terminate_backend". > > > > How about updating the comments as in the attached? I see that commit I think the rationale for the difference should appear, being non-obvious and debatable in this case. > 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever > is mentioned w.r.t permissions in the doc of that function sounds > valid for drop database force to me. Do you have any specific proposal > in your mind? Something like the attached. One could argue the function should also check isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've not done that. Author: Noah Misch Commit: Noah Misch diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml index ff01450..9215b1a 100644 --- a/doc/src/sgml/ref/drop_database.sgml +++ b/doc/src/sgml/ref/drop_database.sgml @@ -79,12 +79,14 @@ DROP DATABASE [ IF EXISTS ] name [ It doesn't terminate if prepared transactions, active logical replication slots or subscriptions are present in the target database. + - This will fail if the current user has no permissions to terminate other - connections. Required permissions are the same as with - pg_terminate_backend, described in - . This will also fail if we - are not able to terminate connections. + This terminates background worker connections and connections that the + current user has permission to terminate + with pg_terminate_backend, described in + . If connections remain, this + command will fail. diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 1a83c42..ab7651d 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -3808,8 +3808,8 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared) * The current backend is always ignored; it is caller's responsibility to * check whether the current backend uses the given DB, if it's important. * - * It doesn't allow to terminate the connections even if there is a one - * backend with the prepared transaction in the target database. + * If the target database has a prepared transaction or permissions checks + * fail for a connection, this fails without terminating anything. */ void TerminateOtherDBBackends(Oid databaseId) @@ -3854,14 +3854,19 @@ TerminateOtherDBBackends(Oid databaseId) ListCell *lc; /* -* Check whether we have the necessary rights to terminate other -* sessions. We don't terminate any session until we ensure that we -* have rights on all the sessions to be terminated. These checks are -* the same as we do in pg_terminate_backend. +* Permissions checks relax the pg_terminate_backend checks in two +* ways, both by omitting the !OidIsValid(proc->roleId) check: * -* In this case we don't raise some warnings - like "PID %d is not a -* PostgreSQL server process", because for us already finished session -* is not a problem. +* - Accept terminating autovacuum workers, since DROP DATABASE +* without FORCE terminates them. +* +* - Accept terminating bgworkers. For bgworker authors, it's +* convenient to be able to recommend FORCE if a worker is blocking +* DROP DATABASE unexpectedly. +* +* Unlike pg_terminate_backend, we don't raise some warnings - like +* "PID %d is not a PostgreSQL server process", because for us already +* finished session is not a problem.
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, Apr 22, 2024 at 9:56 PM Noah Misch wrote: > > On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke > > wrote: > > > > > > While working on [0] i have noticed this comment in > > > TerminateOtherDBBackends function: > > > > > > /* > > > * Check whether we have the necessary rights to terminate other > > > * sessions. We don't terminate any session until we ensure that we > > > * have rights on all the sessions to be terminated. These checks are > > > * the same as we do in pg_terminate_backend. > > > * > > > * In this case we don't raise some warnings - like "PID %d is not a > > > * PostgreSQL server process", because for us already finished session > > > * is not a problem. > > > */ > > > > > > This statement is not true after 3a9b18b. > > > "These checks are the same as we do in pg_terminate_backend." > > The comment mismatch is a problem. Thanks for reporting it. The DROP > DATABASE doc mimics the comment, using, "permissions are the same as with > pg_terminate_backend". > How about updating the comments as in the attached? I see that commit 3a9b18b309 didn't change the docs of pg_terminate_backend and whatever is mentioned w.r.t permissions in the doc of that function sounds valid for drop database force to me. Do you have any specific proposal in your mind? > > > But the code is still correct, I assume... or not? In fact, we are > > > killing autovacuum workers which are working with a given database > > > (proc->roleId == 0), which is OK in that case. Are there any other > > > cases when proc->roleId == 0 but we should not be able to kill such a > > > process? > > > > > > > Good question. I am not aware of such cases but I wonder if we should > > add a check similar to 3a9b18b [1] for the reason given in the commit > > message. I have added Noah to see if he has any suggestions on this > > matter. > > > > [1] - > > commit 3a9b18b3095366cd0c4305441d426d04572d88c1 > > Author: Noah Misch > > Date: Mon Nov 6 06:14:13 2023 -0800 > > > > Ban role pg_signal_backend from more superuser backend types. > > That commit distinguished several scenarios. Here's how they apply in > TerminateOtherDBBackends(): > > - logical replication launcher, autovacuum launcher: the proc->databaseId test > already skips them, since they don't connect to a database. Vignesh said > this. > > - autovacuum worker: should terminate, since CountOtherDBBackends() would > terminate them in DROP DATABASE without FORCE. > > - background workers that connect to a database: the right thing is less clear > on these, but I lean toward allowing termination and changing the DROP > DATABASE doc. As a bgworker author, I would value being able to recommend > DROP DATABASE FORCE if a worker is sticking around unexpectedly. There's > relatively little chance of a bgworker actually wanting to block DROP > DATABASE FORCE or having an exploitable termination-time bug. > Agreed with this analysis and I don't see the need for the code change. -- With Regards, Amit Kapila. fix_comments_1.patch Description: Binary data
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke wrote: > > > > While working on [0] i have noticed this comment in > > TerminateOtherDBBackends function: > > > > /* > > * Check whether we have the necessary rights to terminate other > > * sessions. We don't terminate any session until we ensure that we > > * have rights on all the sessions to be terminated. These checks are > > * the same as we do in pg_terminate_backend. > > * > > * In this case we don't raise some warnings - like "PID %d is not a > > * PostgreSQL server process", because for us already finished session > > * is not a problem. > > */ > > > > This statement is not true after 3a9b18b. > > "These checks are the same as we do in pg_terminate_backend." The comment mismatch is a problem. Thanks for reporting it. The DROP DATABASE doc mimics the comment, using, "permissions are the same as with pg_terminate_backend". > > But the code is still correct, I assume... or not? In fact, we are > > killing autovacuum workers which are working with a given database > > (proc->roleId == 0), which is OK in that case. Are there any other > > cases when proc->roleId == 0 but we should not be able to kill such a > > process? > > > > Good question. I am not aware of such cases but I wonder if we should > add a check similar to 3a9b18b [1] for the reason given in the commit > message. I have added Noah to see if he has any suggestions on this > matter. > > [1] - > commit 3a9b18b3095366cd0c4305441d426d04572d88c1 > Author: Noah Misch > Date: Mon Nov 6 06:14:13 2023 -0800 > > Ban role pg_signal_backend from more superuser backend types. That commit distinguished several scenarios. Here's how they apply in TerminateOtherDBBackends(): - logical replication launcher, autovacuum launcher: the proc->databaseId test already skips them, since they don't connect to a database. Vignesh said this. - autovacuum worker: should terminate, since CountOtherDBBackends() would terminate them in DROP DATABASE without FORCE. - background workers that connect to a database: the right thing is less clear on these, but I lean toward allowing termination and changing the DROP DATABASE doc. As a bgworker author, I would value being able to recommend DROP DATABASE FORCE if a worker is sticking around unexpectedly. There's relatively little chance of a bgworker actually wanting to block DROP DATABASE FORCE or having an exploitable termination-time bug. Thoughts?
Re: TerminateOtherDBBackends code comments inconsistency.
On Mon, 15 Apr 2024 at 11:18, Amit Kapila wrote: > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke wrote: > > > > While working on [0] i have noticed this comment in > > TerminateOtherDBBackends function: > > > > /* > > * Check whether we have the necessary rights to terminate other > > * sessions. We don't terminate any session until we ensure that we > > * have rights on all the sessions to be terminated. These checks are > > * the same as we do in pg_terminate_backend. > > * > > * In this case we don't raise some warnings - like "PID %d is not a > > * PostgreSQL server process", because for us already finished session > > * is not a problem. > > */ > > > > This statement is not true after 3a9b18b. > > "These checks are the same as we do in pg_terminate_backend." > > > > But the code is still correct, I assume... or not? In fact, we are > > killing autovacuum workers which are working with a given database > > (proc->roleId == 0), which is OK in that case. Are there any other > > cases when proc->roleId == 0 but we should not be able to kill such a > > process? > > > > Good question. I am not aware of such cases but I wonder if we should > add a check similar to 3a9b18b [1] for the reason given in the commit > message. I have added Noah to see if he has any suggestions on this > matter. This function is only for drop database with force option, in case of drop database with force option there will be a valid database id and we will not include "logical replication launcher" and "autovacuum launcher" processes as they will not have any database id. For "autovacuum workers" that are associated with this database, we will send the termination signal and then drop the database. As we are not causing any denial of service attack in this case I felt that could be the reason the check was not added in this case. Regards, Vignesh
Re: TerminateOtherDBBackends code comments inconsistency.
On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke wrote: > > While working on [0] i have noticed this comment in > TerminateOtherDBBackends function: > > /* > * Check whether we have the necessary rights to terminate other > * sessions. We don't terminate any session until we ensure that we > * have rights on all the sessions to be terminated. These checks are > * the same as we do in pg_terminate_backend. > * > * In this case we don't raise some warnings - like "PID %d is not a > * PostgreSQL server process", because for us already finished session > * is not a problem. > */ > > This statement is not true after 3a9b18b. > "These checks are the same as we do in pg_terminate_backend." > > But the code is still correct, I assume... or not? In fact, we are > killing autovacuum workers which are working with a given database > (proc->roleId == 0), which is OK in that case. Are there any other > cases when proc->roleId == 0 but we should not be able to kill such a > process? > Good question. I am not aware of such cases but I wonder if we should add a check similar to 3a9b18b [1] for the reason given in the commit message. I have added Noah to see if he has any suggestions on this matter. [1] - commit 3a9b18b3095366cd0c4305441d426d04572d88c1 Author: Noah Misch Date: Mon Nov 6 06:14:13 2023 -0800 Ban role pg_signal_backend from more superuser backend types. -- With Regards, Amit Kapila.
TerminateOtherDBBackends code comments inconsistency.
Hi hackers! While working on [0] i have noticed this comment in TerminateOtherDBBackends function: /* * Check whether we have the necessary rights to terminate other * sessions. We don't terminate any session until we ensure that we * have rights on all the sessions to be terminated. These checks are * the same as we do in pg_terminate_backend. * * In this case we don't raise some warnings - like "PID %d is not a * PostgreSQL server process", because for us already finished session * is not a problem. */ This statement is not true after 3a9b18b. "These checks are the same as we do in pg_terminate_backend." But the code is still correct, I assume... or not? In fact, we are killing autovacuum workers which are working with a given database (proc->roleId == 0), which is OK in that case. Are there any other cases when proc->roleId == 0 but we should not be able to kill such a process? [0] https://www.postgresql.org/message-id/flat/CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6%3D3TBKABu9C3_97YGOoMg%40mail.gmail.com#4e869bc4b6ad88afe70e4484ffd256e2
