Re: TerminateOtherDBBackends code comments inconsistency.

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.