Horton

Thread

Commits

  1. Fix documentation about DROP DATABASE FORCE process termination rights.

  2. Ban role pg_signal_backend from more superuser backend types.

  1. TerminateOtherDBBackends code comments inconsistency.

    Kirill Reshke <reshkekirill@gmail.com> — 2024-04-11T13:27:47Z

    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%40mail.gmail.com#4e869bc4b6ad88afe70e4484ffd256e2

  2. Re: TerminateOtherDBBackends code comments inconsistency.

    Amit Kapila <amit.kapila16@gmail.com> — 2024-04-15T05:47:54Z

    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.

  3. Re: TerminateOtherDBBackends code comments inconsistency.

    vignesh C <vignesh21@gmail.com> — 2024-04-15T09:40:12Z

    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

  4. Re: TerminateOtherDBBackends code comments inconsistency.

    Noah Misch <noah@leadboat.com> — 2024-04-22T16:26:39Z

    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?

  5. Re: TerminateOtherDBBackends code comments inconsistency.

    Amit Kapila <amit.kapila16@gmail.com> — 2024-04-29T04:48:35Z

    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.

  6. Re: TerminateOtherDBBackends code comments inconsistency.

    Noah Misch <noah@leadboat.com> — 2024-04-29T21:27:56Z

    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.

  7. Re: TerminateOtherDBBackends code comments inconsistency.

    Amit Kapila <amit.kapila16@gmail.com> — 2024-04-30T03:40:52Z

    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.

  8. Re: TerminateOtherDBBackends code comments inconsistency.

    Noah Misch <noah@leadboat.com> — 2024-04-30T17:06:18Z

    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.

  9. Re: TerminateOtherDBBackends code comments inconsistency.

    Amit Kapila <amit.kapila16@gmail.com> — 2024-05-06T09:23:57Z

    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.

  10. Re: TerminateOtherDBBackends code comments inconsistency.

    Noah Misch <noah@leadboat.com> — 2024-05-16T21:18:01Z

    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.