Date: Mon, 29 Apr 2024 14:27:56 -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: <20240429212756.60.nmisch@google.com>
References: 
 <CALdSSPjrLvua14C9ckAYbY5aQXepA56d9gd=1Pv85SmtsPvkjA@mail.gmail.com>
 <CAA4eK1KwYxfR7CPLc97vNsQLThOaQsA-GoQCCmjz8s+ORV8caQ@mail.gmail.com>
 <20240422162639.68@rfd.leadboat.com>
 <CAA4eK1J6OXChcJWV_MDrZSUEwbKs-tfC4Zd_1w7yq06W2t5ZFw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8rvLfbG1D9Wg8g7g"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAA4eK1J6OXChcJWV_MDrZSUEwbKs-tfC4Zd_1w7yq06W2t5ZFw@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20240429212756.60.nmisch%40google.com>
Precedence: bulk


--8rvLfbG1D9Wg8g7g
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

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

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.

--8rvLfbG1D9Wg8g7g
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="TerminateOtherDBBackends-doc-v2nm.patch"

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    
diff --git a/doc/src/sgml/ref/drop_database.sgml b/doc/src/sgml/ref/drop_database.sgml
index ff01450..9215b1a 100644
--- a/doc/src/sgml/ref/drop_database.sgml
+++ b/doc/src/sgml/ref/drop_database.sgml
@@ -79,12 +79,14 @@ DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> [
       It doesn't terminate if prepared transactions, active logical replication
       slots or subscriptions are present in the target database.
      </para>
+     <!-- not mentioning exception for autovacuum workers, since those are an
+     implementation detail and the exception is not specific to FORCE -->
      <para>
-      This will fail if the current user has no permissions to terminate other
-      connections. Required permissions are the same as with
-      <literal>pg_terminate_backend</literal>, described in
-      <xref linkend="functions-admin-signal"/>.  This will also fail if we
-      are not able to terminate connections.
+      This terminates background worker connections and connections that the
+      current user has permission to terminate
+      with <function>pg_terminate_backend</function>, described in
+      <xref linkend="functions-admin-signal"/>.  If connections remain, this
+      command will fail.
      </para>
     </listitem>
    </varlistentry>
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 1a83c42..ab7651d 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3808,8 +3808,8 @@ CountOtherDBBackends(Oid databaseId, int *nbackends, int *nprepared)
  * The current backend is always ignored; it is caller's responsibility to
  * check whether the current backend uses the given DB, if it's important.
  *
- * It doesn't allow to terminate the connections even if there is a one
- * backend with the prepared transaction in the target database.
+ * If the target database has a prepared transaction or permissions checks
+ * fail for a connection, this fails without terminating anything.
  */
 void
 TerminateOtherDBBackends(Oid databaseId)
@@ -3854,14 +3854,19 @@ TerminateOtherDBBackends(Oid databaseId)
 		ListCell   *lc;
 
 		/*
-		 * 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.
+		 * Permissions checks relax the pg_terminate_backend checks in two
+		 * ways, both by omitting the !OidIsValid(proc->roleId) check:
 		 *
-		 * 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.
+		 * - Accept terminating autovacuum workers, since DROP DATABASE
+		 *   without FORCE terminates them.
+		 *
+		 * - Accept terminating bgworkers.  For bgworker authors, it's
+		 *   convenient to be able to recommend FORCE if a worker is blocking
+		 *   DROP DATABASE unexpectedly.
+		 *
+		 * Unlike pg_terminate_backend, 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.
 		 */
 		foreach(lc, pids)
 		{
@@ -3870,7 +3875,6 @@ TerminateOtherDBBackends(Oid databaseId)
 
 			if (proc != NULL)
 			{
-				/* Only allow superusers to signal superuser-owned backends. */
 				if (superuser_arg(proc->roleId) && !superuser())
 					ereport(ERROR,
 							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -3878,7 +3882,6 @@ TerminateOtherDBBackends(Oid databaseId)
 							 errdetail("Only roles with the %s attribute may terminate processes of roles with the %s attribute.",
 									   "SUPERUSER", "SUPERUSER")));
 
-				/* Users can signal backends they have role membership in. */
 				if (!has_privs_of_role(GetUserId(), proc->roleId) &&
 					!has_privs_of_role(GetUserId(), ROLE_PG_SIGNAL_BACKEND))
 					ereport(ERROR,

--8rvLfbG1D9Wg8g7g--