Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: Granting SET and ALTER SYSTE privileges for GUCs
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <664799.1647456444@sss.pgh.pa.us>
Date: Wed, 16 Mar 2022 11:59:06 -0700
Cc: Andrew Dunstan <andrew@dunslane.net>,
 Joshua Brindle <joshua.brindle@crunchydata.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Jeff Davis <pgsql@j-davis.com>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Joe Conway <joe@crunchydata.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <EEE65FF4-BFC8-472B-A81E-EE4ED39A848E@enterprisedb.com>
References: <3D691E20-C1D5-4B80-8BA5-6BEB63AF3029@enterprisedb.com>
 <AAE7FBCE-7C7D-4E92-A57F-D4496B306DDF@enterprisedb.com>
 <CA+TgmobUt_tSF-9tz-eRvWzT1pbe_ssvxfnPniGGEgLzems40Q@mail.gmail.com>
 <9DB8DABC-0E1E-4AB0-BB11-9BC48E4C71BE@enterprisedb.com>
 <df224438-bcb5-5543-8ff6-841ce91053fb@dunslane.net>
 <BA6C2DC4-A169-4239-B7E2-B4F73EA908E3@enterprisedb.com>
 <A9802425-8920-4814-9BE1-5C7250E2C386@enterprisedb.com>
 <E6D03A76-4D79-43BE-B896-4571E9FE323B@enterprisedb.com>
 <CAGB+Vh4_UJsYHCWborm-y4CFt8gVvY_KbcNrpWcpXAk-GcwOtA@mail.gmail.com>
 <2D6C1081-DB7D-4260-8987-5B4912E95917@enterprisedb.com>
 <1A6DA47B-2D5F-427E-AD72-1D8BD23BF94C@enterprisedb.com>
 <C8DF19D8-C15D-4C2D-91CA-391390F1E421@enterprisedb.com>
 <79685.1646604824@sss.pgh.pa.us>
 <A22EA9EA-94AC-409F-BEB8-BEF72096D97E@enterprisedb.com>
 <83814.1646607430@sss.pgh.pa.us>
 <78889A65-CA7A-4015-866D-33460967071D@enterprisedb.com>
 <92485.1646609263@sss.pgh.pa.us>
 <ADC4EBF7-87B2-438F-8BA1-8EF57F0E61CF@enterprisedb.com> <43857434-3f9b-3!
 66f-0401-7aea558827e1@dunslane.net> <664799.1647456444@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/EEE65FF4-BFC8-472B-A81E-EE4ED39A848E%40enterprisedb.com>
Precedence: bulk


> On Mar 16, 2022, at 11:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> 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.
>=20
> 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 =3D 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.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company