Re: TerminateOtherDBBackends code comments inconsistency.

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(at)leadboat(dot)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(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".
> >
>
> 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.