On Wed, Oct 13, 2021 at 2:19 PM Stephen Frost <sfrost@snowman.net> wrote:
>
> Greeting,
>
> On Wed, Oct 13, 2021 at 04:14 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Wed, Oct 13, 2021 at 1:24 PM Michael Paquier <michael@paquier.xyz> 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 XXXXX" 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.
