Re: Granting SET and ALTER SYSTE privileges for GUCs

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
Cc: Joshua Brindle <joshua(dot)brindle(at)crunchydata(dot)com>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Jeff Davis <pgsql(at)j-davis(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Joe Conway <joe(at)crunchydata(dot)com>
Subject: Re: Granting SET and ALTER SYSTE privileges for GUCs
Date: 2022-04-05 21:50:02
Message-ID: 2850561.1649195402@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Here's a v16, which I have editorialized on rather heavily.
I have not yet really looked at the docs, but I'm satisfied
with the code and tests now.

The main non-cosmetic changes I made:

* I got rid of the mechanisms that tried to make has_parameter_privilege()
act like USERSET GUCs have a forced public SET grant. I think that's
fundamentally unworkable for custom GUCs, and it's just going to create
confusion and inconsistency. As the code stands, it'll report SET
privilege if you're superuser or have been explicitly granted that
privilege, but having that privilege only matters if the GUC is SUSET.
Need to craft some documentation wording to explain that, but I'd rather
explain that than say "it might or might not tell you that SET privileges
exist". In any case I think this is perfectly symmetrical with the
ALTER SYSTEM privilege: you can grant that all you want, but it still
won't let you set variables whose context value forbids it.

* That meant we have only one default ACL setting, so I was able to
get rid of aclparameterdefault() altogether.

* I took out the post-alter hook call in ExecGrant_Parameter. I'm
prepared to listen to arguments why it should be put back in, but
said arguments would have to explain why GRANT ON PARAMETER should
do that when no other form of GRANT does. (Or, possibly, it'd be
sane for all the forms to call that hook; but that would be material
for a different patch IMV.)

* I took out the pg_parameter_privileges view, which was undocumented
anyway, and which I think people would not find useful. The fact that
it's missing entries for most GUCs and might contain bogus entries
for nonexistent GUCs seems like a strong reason why it's not very
useful in this form. (But see below.)

* I thought the tab-completion stuff was completely excessive.
I got rid of the support for tab completion with both privileges
spelled out, because I think people are just going to write ALL
instead. We don't have tab completion support for commands listing
multiple privileges of other object types, and I don't see why
PARAMETER is the place to start. I also merged a bunch of duplicative
rules by relying on MatchAny where possible.

Aside from documentation, there is one loose end that perhaps would
be best done in a follow-up patch: I'm somewhat of the opinion that
we ought to reinvent a \dcp command for psql. I'd define it as being
a compact version of the pg_settings view, showing the name, value,
and unit columns and probably not much else by default. With "+",
we could add on the privileges if any, and perhaps the context since
you need that to interpret the privileges. I think this'd be more
useful than the pg_parameter_privileges view, and it'd be handier
than pg_settings itself since you could do things like
\dcp autovac*
with a lot less typing than it takes to get the same info
out of pg_settings directly.

Anyway, I'm out of patience with this for today, and I'm throwing
it up for the cfbot to have a look at. I'll hit the docs tomorrow.

regards, tom lane

Attachment Content-Type Size
v16-0001-Allow-grant-and-revoke-of-privileges-on-paramete.patch text/x-diff 142.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2022-04-05 21:52:59 Re: should vacuum's first heap pass be read-only?
Previous Message David G. Johnston 2022-04-05 21:43:49 Re: shared-memory based stats collector - v69