Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, 2021-10-13 at 11:43 +0530, Bharath Rupireddy wrote: > Here comes the v2 patch. Note that I've retained superuser() check in > the pg_log_backend_memory_contexts(). Please review it. FYI: I submitted a separate patch here to allow pg_signal_backend to execute pg_log_backend_memory_contexts(): https://www.postgresql.org/message-id/flat/e5cf6684d17c8d1ef4904ae248605ccd6da03e72.ca...@j-davis.com So we can consider that patch separately. Regards, Jeff Davis
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, Oct 13, 2021 at 2:19 PM Stephen Frost wrote: > > Greeting, > > On Wed, Oct 13, 2021 at 04:14 Bharath Rupireddy > wrote: >> >> On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier wrote: >> > >> > On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote: >> > > IMO, we can just retain the "if (!superuser())" check in the >> > > pg_log_backend_memory_contexts as is. This would be more meaningful as >> > > the error "must be superuser to use raw page functions" explicitly >> > > says that a superuser is allowed. Whereas if we revoke the permissions >> > > in system_views.sql, then the error we get is not meaningful as the >> > > error "permission denied for function pg_log_backend_memory_contexts" >> > > says that permissions denied and the user will have to look at the >> > > documentation for what permissions this function requires. >> > >> > I don't really buy this argument with the "superuser" error message. >> > When removing hardcoded superuser(), we just close the gap by adding >> > in the documentation that the function execution can be granted >> > afterwards. And nobody has complained about the difference in error >> > message AFAIK. That's about extensibility. >> >> I'm not against removing superuser() check in the >> pg_log_backend_memory_contexts. However, there are a lot of functions >> with the "must be superuser to X" kind of error [1]. I'm worried >> if someone proposes to change these as well with what we do for >> pg_log_backend_memory_contexts. >> >> brin_page_type >> brin_page_items >> brin_metapage_info >> brin_revmap_data >> bt_page_stats_internal >> bt_page_items_internal >> bt_page_items_bytea >> bt_metap >> fsm_page_contents >> gin_metapage_info >> gin_page_opaque_info >> and the list goes on. > > > Yes, would generally be good to change at least some of those also, perhaps > all of them. Hm. Let's deal with it separately, if required. > Not sure I see what the argument here is. We should really be trying to move > away from explicit superuser checks. I will remove the superuser() for pg_log_backend_memory_context alone here in the next version of patch. Regards, Bharath Rupireddy.
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
Greeting, On Wed, Oct 13, 2021 at 04:14 Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier > wrote: > > > > On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote: > > > IMO, we can just retain the "if (!superuser())" check in the > > > pg_log_backend_memory_contexts as is. This would be more meaningful as > > > the error "must be superuser to use raw page functions" explicitly > > > says that a superuser is allowed. Whereas if we revoke the permissions > > > in system_views.sql, then the error we get is not meaningful as the > > > error "permission denied for function pg_log_backend_memory_contexts" > > > says that permissions denied and the user will have to look at the > > > documentation for what permissions this function requires. > > > > I don't really buy this argument with the "superuser" error message. > > When removing hardcoded superuser(), we just close the gap by adding > > in the documentation that the function execution can be granted > > afterwards. And nobody has complained about the difference in error > > message AFAIK. That's about extensibility. > > I'm not against removing superuser() check in the > pg_log_backend_memory_contexts. However, there are a lot of functions > with the "must be superuser to X" kind of error [1]. I'm worried > if someone proposes to change these as well with what we do for > pg_log_backend_memory_contexts. > > brin_page_type > brin_page_items > brin_metapage_info > brin_revmap_data > bt_page_stats_internal > bt_page_items_internal > bt_page_items_bytea > bt_metap > fsm_page_contents > gin_metapage_info > gin_page_opaque_info > and the list goes on. Yes, would generally be good to change at least some of those also, perhaps all of them. Not sure I see what the argument here is. We should really be trying to move away from explicit superuser checks. Thanks. Stephen >
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier wrote: > > On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote: > > IMO, we can just retain the "if (!superuser())" check in the > > pg_log_backend_memory_contexts as is. This would be more meaningful as > > the error "must be superuser to use raw page functions" explicitly > > says that a superuser is allowed. Whereas if we revoke the permissions > > in system_views.sql, then the error we get is not meaningful as the > > error "permission denied for function pg_log_backend_memory_contexts" > > says that permissions denied and the user will have to look at the > > documentation for what permissions this function requires. > > I don't really buy this argument with the "superuser" error message. > When removing hardcoded superuser(), we just close the gap by adding > in the documentation that the function execution can be granted > afterwards. And nobody has complained about the difference in error > message AFAIK. That's about extensibility. I'm not against removing superuser() check in the pg_log_backend_memory_contexts. However, there are a lot of functions with the "must be superuser to X" kind of error [1]. I'm worried if someone proposes to change these as well with what we do for pg_log_backend_memory_contexts. brin_page_type brin_page_items brin_metapage_info brin_revmap_data bt_page_stats_internal bt_page_items_internal bt_page_items_bytea bt_metap fsm_page_contents gin_metapage_info gin_page_opaque_info and the list goes on. Regards, Bharath Rupireddy.
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
Greetings, On Wed, Oct 13, 2021 at 03:54 Michael Paquier wrote: > On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote: > > IMO, we can just retain the "if (!superuser())" check in the > > pg_log_backend_memory_contexts as is. This would be more meaningful as > > the error "must be superuser to use raw page functions" explicitly > > says that a superuser is allowed. Whereas if we revoke the permissions > > in system_views.sql, then the error we get is not meaningful as the > > error "permission denied for function pg_log_backend_memory_contexts" > > says that permissions denied and the user will have to look at the > > documentation for what permissions this function requires. > > I don't really buy this argument with the "superuser" error message. > When removing hardcoded superuser(), we just close the gap by adding > in the documentation that the function execution can be granted > afterwards. And nobody has complained about the difference in error > message AFAIK. That's about extensibility. Agreed. Thanks, Stephen >
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, Oct 13, 2021 at 11:15:16AM +0530, Bharath Rupireddy wrote: > IMO, we can just retain the "if (!superuser())" check in the > pg_log_backend_memory_contexts as is. This would be more meaningful as > the error "must be superuser to use raw page functions" explicitly > says that a superuser is allowed. Whereas if we revoke the permissions > in system_views.sql, then the error we get is not meaningful as the > error "permission denied for function pg_log_backend_memory_contexts" > says that permissions denied and the user will have to look at the > documentation for what permissions this function requires. I don't really buy this argument with the "superuser" error message. When removing hardcoded superuser(), we just close the gap by adding in the documentation that the function execution can be granted afterwards. And nobody has complained about the difference in error message AFAIK. That's about extensibility. -- Michael signature.asc Description: PGP signature
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, Oct 13, 2021 at 7:48 AM Bossart, Nathan wrote: > > On 10/12/21, 6:26 PM, "Michael Paquier" wrote: > > On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote: > >> I would think we would do both…. That is- move to using GRANT/REVOKE, and > >> then just include a GRANT to pg_read_all_stats. > >> > >> Or not. I can see the argument that, because it just goes into the log, > >> that it doesn’t make sense to grant to a predefined role, since that role > >> wouldn’t be able to see the results even if it had access. > > > > I don't think that this is a bad thing to remove the superuser() check > > and replace it with a REVOKE FROM PUBLIC in this case, but linking the > > logging of memory contexts with pg_read_all_stats does not seem right > > to me. > > +1 Here comes the v2 patch. Note that I've retained superuser() check in the pg_log_backend_memory_contexts(). Please review it. Regards, Bharath Rupireddy. v2-0001-change-privileges-of-pg_backend_memory_contexts-a.patch Description: Binary data
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Wed, Oct 13, 2021 at 6:55 AM Michael Paquier wrote: > > On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote: > > I would think we would do both…. That is- move to using GRANT/REVOKE, and > > then just include a GRANT to pg_read_all_stats. > > > > Or not. I can see the argument that, because it just goes into the log, > > that it doesn’t make sense to grant to a predefined role, since that role > > wouldn’t be able to see the results even if it had access. > > I don't think that this is a bad thing to remove the superuser() check > and replace it with a REVOKE FROM PUBLIC in this case, IMO, we can just retain the "if (!superuser())" check in the pg_log_backend_memory_contexts as is. This would be more meaningful as the error "must be superuser to use raw page functions" explicitly says that a superuser is allowed. Whereas if we revoke the permissions in system_views.sql, then the error we get is not meaningful as the error "permission denied for function pg_log_backend_memory_contexts" says that permissions denied and the user will have to look at the documentation for what permissions this function requires. And, I see there are a lot of functions in the code base that does "if (!superuser())" check and emit "must be superuser to XXX" sort of error. > but linking the > logging of memory contexts with pg_read_all_stats does not seem right > to me. Agreed. The user with pg_read_all_stats can't see the server logs so it doesn't make sense to make them call the function. I will remove this change from the patch. Regards, Bharath Rupireddy.
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On 10/12/21, 6:26 PM, "Michael Paquier" wrote: > On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote: >> I would think we would do both…. That is- move to using GRANT/REVOKE, and >> then just include a GRANT to pg_read_all_stats. >> >> Or not. I can see the argument that, because it just goes into the log, >> that it doesn’t make sense to grant to a predefined role, since that role >> wouldn’t be able to see the results even if it had access. > > I don't think that this is a bad thing to remove the superuser() check > and replace it with a REVOKE FROM PUBLIC in this case, but linking the > logging of memory contexts with pg_read_all_stats does not seem right > to me. +1 Nathan
Re: should we allow users with a predefined role to access pg_backend_memory_contexts view and pg_log_backend_memory_contexts function?gr
On Tue, Oct 12, 2021 at 08:33:19PM -0400, Stephen Frost wrote: > I would think we would do both…. That is- move to using GRANT/REVOKE, and > then just include a GRANT to pg_read_all_stats. > > Or not. I can see the argument that, because it just goes into the log, > that it doesn’t make sense to grant to a predefined role, since that role > wouldn’t be able to see the results even if it had access. I don't think that this is a bad thing to remove the superuser() check and replace it with a REVOKE FROM PUBLIC in this case, but linking the logging of memory contexts with pg_read_all_stats does not seem right to me. -- Michael signature.asc Description: PGP signature
