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". > > 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. Thoughts?
