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 <reshkekirill(at)gmail(dot)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(at)leadboat(dot)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?