On Sun, 2021-10-24 at 19:58 +0530, Bharath Rupireddy wrote:
> 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.
Note that some functions have additional checks that can't be expressed
with GRANT -- see pg_cancel_backend(), for example. But I agree in
general that GRANT is the way to go most of the time.
> 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?
It seems like there's agreement on the direction, but I don't know that
there's a good place to write it down. Probably better to just fix as
many of the functions as we can, and then when people add new ones,
they'll copy the GRANT pattern rather than the explicit superuser
check.
> Comments on the patch:
> 1) testrole1 and testrole2 look generic, how about
Michael had a similar comment. Renamed, thank you.
> 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.
If we add all the function privilege checks there, I think it will
overwhelm the other interesting tests happening in that file.
> 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.
Missed this comment. I'll tweak this before commit.
> 4) Isn't the +#define CATALOG_VERSION_NO 202110230 going to be set to
> the date on which the patch gets committed?
I just put it in there so that I wouldn't forget, but I'll update it at
commit time.
> 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());
What would you like me to mention?
Regards,
Jeff Davis
