On Mon, Apr 22, 2024 at 9:56 PM Noah Misch <n...@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 <reshkekir...@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 <n...@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.
fix_comments_1.patch
Description: Binary data