Re: Granting SET and ALTER SYSTE privileges for GUCs

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Generally I think this is now in fairly good shape, I've played with it
> and it seems to do what I expect in every case, and the things I found
> surprising are gone.

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.

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 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 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. 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.

regards, tom lane