TerminateOtherDBBackends code comments inconsistency.
Hi hackers!
While working on [0]/messages/by-id/CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com 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."
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?
[0]: /messages/by-id/CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6=3TBKABu9C3_97YGOoMg@mail.gmail.com
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."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]commit 3a9b18b3095366cd0c4305441d426d04572d88c1 Author: Noah Misch <noah@leadboat.com> Date: Mon Nov 6 06:14:13 2023 -0800 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
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.
--
With Regards,
Amit Kapila.
On Mon, 15 Apr 2024 at 11:18, Amit Kapila <amit.kapila16@gmail.com> 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."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.
This function is only for drop database with force option, in case of
drop database with force option there will be a valid database id and
we will not include "logical replication launcher" and "autovacuum
launcher" processes as they will not have any database id. For
"autovacuum workers" that are associated with this database, we will
send the termination signal and then drop the database. As we are not
causing any denial of service attack in this case I felt that could be
the reason the check was not added in this case.
Regards,
Vignesh
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 -0800Ban 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?
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 -0800Ban 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.
Attachments:
fix_comments_1.patchapplication/octet-stream; name=fix_comments_1.patchDownload
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 1a83c4220b..d385ebd065 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -3857,7 +3857,9 @@ TerminateOtherDBBackends(Oid databaseId)
* 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.
+ * the same as we do in pg_terminate_backend except that we don't treat
+ * the process not advertising a role to have the same importance as of
+ * a superuser-owned 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
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.
Attachments:
TerminateOtherDBBackends-doc-v2nm.patchtext/plain; charset=us-asciiDownload
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,
On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> wrote:
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:
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.
LGTM.
One could argue the function should also check
isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've
not done that.
What is the argument for ignoring such workers?
--
With Regards,
Amit Kapila.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote:
On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> wrote:
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:
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.
LGTM.
One could argue the function should also check
isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've
not done that.What is the argument for ignoring such workers?
One of the proposed code comments says, "For bgworker authors, it's convenient
to be able to recommend FORCE if a worker is blocking DROP DATABASE
unexpectedly." That argument is debatable, but I do think it applies equally
to bgworkers whether or not they set proc->roleId.
On Tue, Apr 30, 2024 at 10:36 PM Noah Misch <noah@leadboat.com> wrote:
One could argue the function should also check
isBackgroundWorker and ignore even bgworkers that set proc->roleId, but I've
not done that.What is the argument for ignoring such workers?
One of the proposed code comments says, "For bgworker authors, it's convenient
to be able to recommend FORCE if a worker is blocking DROP DATABASE
unexpectedly." That argument is debatable, but I do think it applies equally
to bgworkers whether or not they set proc->roleId.
Agreed.
--
With Regards,
Amit Kapila.
On Tue, Apr 30, 2024 at 09:10:52AM +0530, Amit Kapila wrote:
On Tue, Apr 30, 2024 at 2:58 AM Noah Misch <noah@leadboat.com> wrote:
On Mon, Apr 29, 2024 at 10:18:35AM +0530, Amit Kapila wrote:
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.
LGTM.
Pushed as commit 372700c. Thanks for the report and the review.