Re: Granting SET and ALTER SYSTE privileges for GUCs

> On Mar 16, 2022, at 11:47 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Stepping back a bit ... do we really want to institutionalize the
> term "setting" for GUC variables? I realize that the view pg_settings
> exists, but the documentation generally prefers the term "configuration
> parameters". Where config.sgml uses "setting" as a noun, it's usually
> talking about a specific concrete value for a parameter, and you can
> argue that the view's name comports with that meaning. But you can't
> GRANT a parameter's current value.
>
> I don't have a better name to offer offhand --- I surely am not proposing
> that we change the syntax to be "GRANT ... ON CONFIGURATION PARAMETER x",
> because that's way too wordy. But now is the time to bikeshed if we're
> gonna bikeshed, or else admit that we're not interested in precise
> vocabulary.

Informally, we often use "GUC" on this list, but that isn't used formally, leaving "configuration parameter" and "setting" as the two obvious choices. I preferred "configuration parameter" originally and was argued out of it. My take on "setting" was also that it more naturally refers to the choice of setting, not the thing being set, such that "work_mem = 8192" means the configuration parameter "work_mem" has the setting "8192". I'm happy to change the patch along these lines, but I vaguely recall it being Robert who liked the "setting" language. Robert? (Sorry if I misremember that...)

> I'm also fairly allergic to the way that this patch has decided to assign
> multi-word names to privilege types (ie SET VALUE, ALTER SYSTEM). There
> is no existing precedent for that, and I think it's going to break
> client-side code that we don't need to break. It's not coincidental that
> this forces weird changes in rules about whitespace in the has_privilege
> functions, for example; and if you think that isn't going to cause
> problems I think you are wrong. Perhaps we could just use "SET" and
> "ALTER", or "SET" and "SYSTEM"?

I chose those particular multi-word names to avoid needing to promote any keywords. That's been a while ago, and I don't recall exactly where the shift-reduce or reduce-reduce errors were coming from. I'll play with it to see what I can find.

> I also agree with the upthread criticism that this is abusing the
> ObjectPostAlterHook API unreasonably much. In particular, it's
> trying to use pg_setting_acl OIDs as identities for GUCs, which
> they are not, first because they're unstable and second because
> most GUCs won't even have an entry there. I therefore judge the
> hook calls added to ExecSetVariableStmt and AlterSystemSetConfigFile
> to be 100% useless, in fact probably counterproductive because they
> introduce a boatload of worries about whether the right things happen
> if the hook errors out or does something guc.c isn't expecting.

I think Joshua was planning to use these hooks for security purposes. The hooks are supposed to check whether the Oid is valid, and if not, still be able to make choices based on the other information. Joshua, any comment on this?

> I suggest that what might be saner is to consider that the "objects"
> that the hook calls are concerned with are the pg_setting_acl entries,
> not the underlying GUCs, and thus that the hooks need be invoked only
> when creating, destroying or altering those entries.

That's certainly a thing we could do, but I got the impression that Joshua wanted to hook into SET, RESET, and ALTER SYSTEM SET, not merely into GRANT and REVOKE.

> If we do have
> a need for a hook editorializing on GUC value settings, that would
> have to be an independent API --- but I have heard no calls for
> the ability to have such a hook, and I don't think that this patch
> is the place to introduce one.

Well, the call for it was in this thread, but I'm ok with yanking out that part of the patch if it seems half-baked.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company