pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedTerminateOtherDBBackends code comments inconsistency.
10+ messages / 4 participants
[nested] [flat]
* TerminateOtherDBBackends code comments inconsistency.
@ 2024-04-11 13:27 Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Reshke @ 2024-04-11 13:27 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi hackers!
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?
[0] https://www.postgresql.org/message-id/flat/CALdSSPiOLADNe1ZS-1dnQXWVXgGAC6%3D3TBKABu9C3_97YGOoMg%40m...
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
@ 2024-04-15 05:47 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-15 09:40 ` Re: TerminateOtherDBBackends code comments inconsistency. vignesh C <vignesh21@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 10+ messages in thread
From: Amit Kapila @ 2024-04-15 05:47 UTC (permalink / raw)
To: Kirill Reshke <reshkekirill@gmail.com>; Noah Misch <noah@leadboat.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
[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.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
@ 2024-04-15 09:40 ` vignesh C <vignesh21@gmail.com>
1 sibling, 0 replies; 10+ messages in thread
From: vignesh C @ 2024-04-15 09:40 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; Noah Misch <noah@leadboat.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
@ 2024-04-22 16:26 ` Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 10+ messages in thread
From: Noah Misch @ 2024-04-22 16:26 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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?
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
@ 2024-04-29 04:48 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
0 siblings, 1 reply; 10+ messages in thread
From: Amit Kapila @ 2024-04-29 04:48 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
Attachments:
[application/octet-stream] fix_comments_1.patch (868B, ../../CAA4eK1J6OXChcJWV_MDrZSUEwbKs-tfC4Zd_1w7yq06W2t5ZFw@mail.gmail.com/2-fix_comments_1.patch)
download | inline diff:
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
@ 2024-04-29 21:27 ` Noah Misch <noah@leadboat.com>
2024-04-30 03:40 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Noah Misch @ 2024-04-29 21:27 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
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,
Attachments:
[text/plain] TerminateOtherDBBackends-doc-v2nm.patch (4.0K, ../../20240429212756.60.nmisch@google.com/2-TerminateOtherDBBackends-doc-v2nm.patch)
download | inline diff:
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,
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
@ 2024-04-30 03:40 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-30 17:06 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-05-16 21:18 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 10+ messages in thread
From: Amit Kapila @ 2024-04-30 03:40 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-30 03:40 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
@ 2024-04-30 17:06 ` Noah Misch <noah@leadboat.com>
2024-05-06 09:23 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 10+ messages in thread
From: Noah Misch @ 2024-04-30 17:06 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-30 03:40 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-30 17:06 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
@ 2024-05-06 09:23 ` Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Amit Kapila @ 2024-05-06 09:23 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Kirill Reshke <reshkekirill@gmail.com>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: TerminateOtherDBBackends code comments inconsistency.
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-22 16:26 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Re: TerminateOtherDBBackends code comments inconsistency. Noah Misch <noah@leadboat.com>
2024-04-30 03:40 ` Re: TerminateOtherDBBackends code comments inconsistency. Amit Kapila <amit.kapila16@gmail.com>
@ 2024-05-16 21:18 ` Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 10+ messages in thread
From: Noah Misch @ 2024-05-16 21:18 UTC (permalink / raw)
To: Kirill Reshke <reshkekirill@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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.
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-05-16 21:18 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-04-11 13:27 TerminateOtherDBBackends code comments inconsistency. Kirill Reshke <reshkekirill@gmail.com>
2024-04-15 05:47 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-15 09:40 ` vignesh C <vignesh21@gmail.com>
2024-04-22 16:26 ` Noah Misch <noah@leadboat.com>
2024-04-29 04:48 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-29 21:27 ` Noah Misch <noah@leadboat.com>
2024-04-30 03:40 ` Amit Kapila <amit.kapila16@gmail.com>
2024-04-30 17:06 ` Noah Misch <noah@leadboat.com>
2024-05-06 09:23 ` Amit Kapila <amit.kapila16@gmail.com>
2024-05-16 21:18 ` Noah Misch <noah@leadboat.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox