On Sun, Oct 24, 2021 at 1:27 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
>
> Simple patch to implement $SUBJECT attached.
>
> pg_signal_backend seems like the appropriate predefined role, because
> pg_log_backend_memory_contexts() is implemented by a sending signal.
+1.
It looks like we are better off with removing explicit superuser()
checks from the functions and using normal GRANT based system, see
others agreeing on this at [1]. As we have lots of functions that are
doing explicit superuser() checks, I'm sure someday they all will have
to be moved to GRANT system. The current code is a mix - some
functions do explicit checks (I've seen many of them with the comment
at [2]) and others do it via GRANT system. I'm not saying that we
should be dealing with those here in this thread, all I'm looking for
is that we have a note of it in the postgres todo list in the wiki so
that someone interested can pick that work up. Thoughts?
Comments on the patch:
1) testrole1 and testrole2 look generic, how about
regress_mcxt_role1/2? There's no problem as they are
misc_functions.sql local, but still role names can be more readable.
+CREATE ROLE testrole1 IN ROLE pg_signal_backend;
+CREATE ROLE testrole2;
2) It seems like the privileges.sql is the right place to place the
test cases, but I'm fine with keeping all the test cases of the
function together.
3) It might be enough to do has_function_privilege, just a thought -
isn't it better if we execute the function with the test roles set in.
This will help us catch the permission denied error message in the
test output files.
4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to
the date on which the patch gets committed?
5) The following change is being handled in the patch at [3], I know
it is appropriate to have it in this patch, but please mention it in
the commit message on why we do this change. I will remove this change
from my patch at [3].
-SELECT * FROM pg_log_backend_memory_contexts(pg_backend_pid());
+SELECT pg_log_backend_memory_contexts(pg_backend_pid());
[1] - https://www.postgresql.org/message-id/CAOuzzgpp0dmOFjWC4JDvk57ZQGm8umCrFdR1at4b80xuF0XChw%40mail.gmail.com
[2] -
* Permission checking for this function is managed through the normal
* GRANT system.
*/
[3] - https://www.postgresql.org/message-id/CALj2ACVXk1roswqFpiCOMHrsB%2BxxW7HG536krGAzF%3DmWXh3eWQ%40mail.gmail.com
Regards,
Bharath Rupireddy.
