Date: Mon, 22 Apr 2024 09:26:39 -0700
From: Noah Misch <noah@leadboat.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: TerminateOtherDBBackends code comments inconsistency.
Message-ID: <20240422162639.68@rfd.leadboat.com>
References: 
 <CALdSSPjrLvua14C9ckAYbY5aQXepA56d9gd=1Pv85SmtsPvkjA@mail.gmail.com>
 <CAA4eK1KwYxfR7CPLc97vNsQLThOaQsA-GoQCCmjz8s+ORV8caQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1KwYxfR7CPLc97vNsQLThOaQsA-GoQCCmjz8s+ORV8caQ@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240422162639.68%40rfd.leadboat.com>
Precedence: bulk

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

> > 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.

Thoughts?