Thread: [MASSMAIL]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
On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke <reshkekirill@gmail.com> 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 <noah@leadboat.com>
Date: Mon Nov 6 06:14:13 2023 -0800
Ban role pg_signal_backend from more superuser backend types.
--
With Regards,
Amit Kapila.
On Mon, 15 Apr 2024 at 11:18, Amit Kapila <amit.kapila16@gmail.com> wrote: > > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke <reshkekirill@gmail.com> 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
On Mon, Apr 15, 2024 at 11:17:54AM +0530, Amit Kapila wrote: > On Thu, Apr 11, 2024 at 6:58 PM Kirill Reshke <reshkekirill@gmail.com> 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 <noah@leadboat.com> > 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?
On Mon, Apr 22, 2024 at 9:56 PM Noah Misch <noah@leadboat.com> 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 <reshkekirill@gmail.com> 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 <noah@leadboat.com> > > 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.
Attachment
On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote: > On Mon, Apr 22, 2024 at 9:56 PM Noah Misch <noah@leadboat.com> 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 <reshkekirill@gmail.com> 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.
Attachment
On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> 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 <noah@leadboat.com> 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.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote: > On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> 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 <noah@leadboat.com> 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.
On Tue, Apr 30, 2024 at 10:36 PM Noah Misch <noah@leadboat.com> 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.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote: > On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> 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.