Horton

Thread

Commits

  1. Revert "Add USER SET parameter values for pg_db_role_setting"

  2. Fix wrong construct_array_builtin() call in GUCArrayDelete()

  3. In hstore_plpython, avoid crashing when return value isn't a mapping.

  4. Allow granting SET and ALTER SYSTEM privileges on GUC parameters.

  1. Possible regression setting GUCs on \connect

    David Steele <david@pgmasters.net> — 2023-04-27T15:22:09Z

    Hackers,

I have been updating pgAudit for PG16 and ran into the following issue 
in the regression tests:

\connect - user1
WARNING:  permission denied to set parameter "pgaudit.log_level"

This happens after switching back and forth a few times between the 
current user when the regression script was executed and user1 which is 
created in the script. Specifically, it happens at [1].

I have tracked the issue down to context == PGC_USERSET for case 
PGC_SUSET in set_config_option_ext(). This GUC is PGC_SUSET so it seems 
like once it is set reloading it should not be an issue.

If the GUC is set again immediately before the \connect then there is no 
error, so it looks like the correct context is being lost somewhere 
along the way.

Before I get into serious debugging on this issue, I thought it would be 
good to bring it up in case the answer is obvious to someone else.

Thanks,
-David

[1] 
https://github.com/pgaudit/pgaudit/compare/dev-pg16-ci#diff-db4bb73982787fa1d07d4c9d80bc54028b8d2a52b80806f1352a42c42f00eaaaR604

  2. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T16:13:21Z

    David Steele <david@pgmasters.net> writes:
> I have been updating pgAudit for PG16 and ran into the following issue 
> in the regression tests:
> \connect - user1
> WARNING:  permission denied to set parameter "pgaudit.log_level"
> This happens after switching back and forth a few times between the 
> current user when the regression script was executed and user1 which is 
> created in the script. Specifically, it happens at [1].

If this is new in v16, I'm inclined to blame 096dd80f3, but that's
just a guess without a test case.

			regards, tom lane

  3. Re: Possible regression setting GUCs on \connect

    David Steele <david@pgmasters.net> — 2023-04-27T16:35:25Z

    On 4/27/23 19:13, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> I have been updating pgAudit for PG16 and ran into the following issue
>> in the regression tests:
>> \connect - user1
>> WARNING:  permission denied to set parameter "pgaudit.log_level"
>> This happens after switching back and forth a few times between the
>> current user when the regression script was executed and user1 which is
>> created in the script. Specifically, it happens at [1].
> 
> If this is new in v16, I'm inclined to blame 096dd80f3, but that's
> just a guess without a test case.

Seems plausible. This can be reproduced by cloning [1] into contrib and 
running `make check`. I can work out another test case but it may not 
end up being simpler.

Thoughts on this Alexander?

[1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci

  4. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T18:16:35Z

    David Steele <david@pgmasters.net> writes:
> Seems plausible. This can be reproduced by cloning [1] into contrib and 
> running `make check`. I can work out another test case but it may not 
> end up being simpler.
> [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci

I tried to replicate this per that recipe, but it works for me:

$ git clone https://github.com/pgaudit/pgaudit.git pgaudit
$ cd pgaudit
$ git checkout dev-pg16-ci
$ make -s check
# +++ regress check in contrib/pgaudit +++
# using temp instance on port 61696 with PID 1191703
ok 1         - pgaudit                                   310 ms
1..1
# All 1 tests passed.

This is at commit 6f879bddbdcfbf9995ecee1db9a587e06027bd13 on
your dev-pg16-ci branch and df38157d94662a64e2f83aa8a0110fd1ee7c4776
on PG master.  Note that I had to add

$ diff -pud Makefile~ Makefile 
--- Makefile~   2023-04-27 14:02:19.041714415 -0400
+++ Makefile    2023-04-27 14:07:10.056909016 -0400
@@ -20,3 +20,5 @@ top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 include $(top_srcdir)/contrib/contrib-global.mk
 endif
+
+EXTRA_INSTALL += contrib/pg_stat_statements

else I got failures about pg_stat_statements not being installed.
But I don't see the failure you're complaining of.

			regards, tom lane

  5. Re: Possible regression setting GUCs on \connect

    David Steele <david@pgmasters.net> — 2023-04-27T18:28:26Z

    On 4/27/23 21:16, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> Seems plausible. This can be reproduced by cloning [1] into contrib and
>> running `make check`. I can work out another test case but it may not
>> end up being simpler.
>> [1] https://github.com/pgaudit/pgaudit/tree/dev-pg16-ci
> 
> I tried to replicate this per that recipe, but it works for me:
> 
> $ git clone https://github.com/pgaudit/pgaudit.git pgaudit
> $ cd pgaudit
> $ git checkout dev-pg16-ci
> $ make -s check
> # +++ regress check in contrib/pgaudit +++
> # using temp instance on port 61696 with PID 1191703
> ok 1         - pgaudit                                   310 ms
> 1..1
> # All 1 tests passed.

I included the errors in the expect log so I could link to them. So test 
success means the error is happening.

> Note that I had to add
> 
> $ diff -pud Makefile~ Makefile
> --- Makefile~   2023-04-27 14:02:19.041714415 -0400
> +++ Makefile    2023-04-27 14:07:10.056909016 -0400
> @@ -20,3 +20,5 @@ top_builddir = ../..
>   include $(top_builddir)/src/Makefile.global
>   include $(top_srcdir)/contrib/contrib-global.mk
>   endif
> +
> +EXTRA_INSTALL += contrib/pg_stat_statements

Yeah, I rarely run tests in-tree, but I'll add this if it does not break 
our regular CI.

-David

  6. Re: Possible regression setting GUCs on \connect

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-27T18:43:08Z

    I suspect the problem is that GUCArrayDelete() is using the wrong Datum:

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9dd624b3ae..ee9f87e7f2 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6496,7 +6496,7 @@ GUCArrayDelete(ArrayType *array, ArrayType **usersetArray, const char *name)
         {
             newarray = construct_array_builtin(&d, 1, TEXTOID);
             if (usersetArray)
-                newUsersetArray = construct_array_builtin(&d, 1, BOOLOID);
+                newUsersetArray = construct_array_builtin(&userSetDatum, 1, BOOLOID);
         }
 
         index++;

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: Possible regression setting GUCs on \connect

    David Steele <david@pgmasters.net> — 2023-04-27T18:47:33Z

    On 4/27/23 21:43, Nathan Bossart wrote:
> I suspect the problem is that GUCArrayDelete() is using the wrong Datum:
> 
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 9dd624b3ae..ee9f87e7f2 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -6496,7 +6496,7 @@ GUCArrayDelete(ArrayType *array, ArrayType **usersetArray, const char *name)
>           {
>               newarray = construct_array_builtin(&d, 1, TEXTOID);
>               if (usersetArray)
> -                newUsersetArray = construct_array_builtin(&d, 1, BOOLOID);
> +                newUsersetArray = construct_array_builtin(&userSetDatum, 1, BOOLOID);
>           }
>   
>           index++;

That seems to work. The errors are now gone.

Thanks!
-David

  8. Re: Possible regression setting GUCs on \connect

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-27T18:51:28Z

    On Thu, Apr 27, 2023 at 09:47:33PM +0300, David Steele wrote:
> That seems to work. The errors are now gone.

Great.  Barring objections, I'll plan on committing this shortly.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  9. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-27T18:53:23Z

    Hi!

On Thu, Apr 27, 2023 at 9:51 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Thu, Apr 27, 2023 at 09:47:33PM +0300, David Steele wrote:
> > That seems to work. The errors are now gone.
>
> Great.  Barring objections, I'll plan on committing this shortly.

Thanks to everybody for catching and investigating this.
Nathan, I'd like to push it myself.  I'm also going to check the code
for similar errors.

------
Regards,
Alexander Korotkov

  10. Re: Possible regression setting GUCs on \connect

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-27T18:55:29Z

    On Thu, Apr 27, 2023 at 09:53:23PM +0300, Alexander Korotkov wrote:
> Thanks to everybody for catching and investigating this.
> Nathan, I'd like to push it myself.  I'm also going to check the code
> for similar errors.

Sounds good!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  11. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-27T19:20:13Z

    .On Thu, Apr 27, 2023 at 9:55 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> On Thu, Apr 27, 2023 at 09:53:23PM +0300, Alexander Korotkov wrote:
> > Thanks to everybody for catching and investigating this.
> > Nathan, I'd like to push it myself.  I'm also going to check the code
> > for similar errors.
>
> Sounds good!

I didn't find similar bugs in 096dd80f3c.  Pushed!

------
Regards,
Alexander Korotkov

  12. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T19:22:04Z

    David Steele <david@pgmasters.net> writes:
> On 4/27/23 21:16, Tom Lane wrote:
>> I tried to replicate this per that recipe, but it works for me:

> I included the errors in the expect log so I could link to them. So test 
> success means the error is happening.

Ah, got it.

I added some debug output to the test, and what I see is that
at the "\connect - user1" commands that work ok, what we have
in pg_db_role_setting is along the lines of


+select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
+ setdatabase | setrole  |                                                                   setconfig                                                                   |    setuser    
+-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
+           0 | user1    | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on}                       | {f,f,f,f,f}
...

while where it's not working:

+select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
+ setdatabase | setrole  |                                                                   setconfig                                                                   |    setuser    
+-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
+           0 | user1    | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_statement=off}                                               | {t,f,f,f}
...

So it is failing because setuser = 't' for that setting; which makes the
failure itself unsurprising, but it seems like the flag should not
have been set that way.  Digging further, it looks like the flag array
is not correctly updated during ALTER USER RESET:

select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
NOTICE:  AUDIT: SESSION,1,1,READ,SELECT,TABLE,pg_catalog.pg_db_role_setting,"select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;",<not logged>
 setdatabase | setrole  |                                                                   setconfig                                                                   |    setuser    
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
           0 | user1    | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor,pgaudit.log_relation=on}                       | {f,f,f,f,f}

... that's fine ...

ALTER ROLE user1 RESET pgaudit.log_relation;
select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
 setdatabase | setrole  |                                                                   setconfig                                                                   |    setuser    
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
           0 | user1    | {"pgaudit.log=read, WRITE",pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor}                                               | {t,f,f,f}

... wrong ...

ALTER ROLE user1 RESET pgaudit.log;
select setdatabase, setrole::regrole, setconfig, setuser from pg_db_role_setting;
 setdatabase | setrole  |                                                                   setconfig                                                                   |    setuser    
-------------+----------+-----------------------------------------------------------------------------------------------------------------------------------------------+---------------
           0 | user1    | {pgaudit.log_level=notice,pgaudit.log_client=on,pgaudit.role=auditor}                                                                         | {t,f,f}

... and wronger.


I had not paid any attention to 096dd80f3 when it went in, but now
that I have looked at it I'm quite distressed, independently of this
probably-minor bug.  It seems to me that this feature is not well
designed and completely ignores the precedents set by commits
a0ffa885e and 13d838815.  The right way to do this was not to add some
poorly-explained option to ALTER ROLE, but to record the role OID that
issued the ALTER ROLE, and then to check when loading the ALTER ROLE
setting whether that role (still) has the right to change the
specified setting.  As implemented, this can't possibly track changes
in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
introducing outright security holes like the one fixed by 13d838815.

I think we ought to revert 096dd80f3 and try again in v17.

			regards, tom lane

  13. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T19:34:55Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> I suspect the problem is that GUCArrayDelete() is using the wrong Datum:

> -                newUsersetArray = construct_array_builtin(&d, 1, BOOLOID);
> +                newUsersetArray = construct_array_builtin(&userSetDatum, 1, BOOLOID);

Ah, should have checked my mail earlier.

However, my concern about whether we even want this feature
still stands.

			regards, tom lane

  14. Re: Possible regression setting GUCs on \connect

    Nathan Bossart <nathandbossart@gmail.com> — 2023-04-27T22:14:43Z

    On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote:
> The right way to do this was not to add some
> poorly-explained option to ALTER ROLE, but to record the role OID that
> issued the ALTER ROLE, and then to check when loading the ALTER ROLE
> setting whether that role (still) has the right to change the
> specified setting.  As implemented, this can't possibly track changes
> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
> introducing outright security holes like the one fixed by 13d838815.

I generally agree.  At least, I think it would be nice to avoid adding a
new option if possible.  It's not clear to me why we'd need to also check
privileges at login time as opposed to only checking them at ALTER ROLE SET
time.  ISTM that the former approach would introduce some interesting
problems around dropping roles or changing roles' privileges.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  15. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T22:38:03Z

    Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote:
>> The right way to do this was not to add some
>> poorly-explained option to ALTER ROLE, but to record the role OID that
>> issued the ALTER ROLE, and then to check when loading the ALTER ROLE
>> setting whether that role (still) has the right to change the
>> specified setting.  As implemented, this can't possibly track changes
>> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
>> introducing outright security holes like the one fixed by 13d838815.

> I generally agree.  At least, I think it would be nice to avoid adding a
> new option if possible.  It's not clear to me why we'd need to also check
> privileges at login time as opposed to only checking them at ALTER ROLE SET
> time.

Perhaps there's room to argue about that.  But ISTM that if someone
does ALTER ROLE SET on the strength of some privilege you granted
them, and then you regret that and revoke the privilege, then the
ALTER ROLE setting should not continue to work.  So I would regard
the session-start-time check as the primary one.  Checking when
ALTER ROLE is done is just a user-friendliness detail.

Also, in the case of an extension-defined GUC, we don't necessarily
know its privilege level at either ALTER ROLE time or session start,
since the extension might not yet be loaded at either point.
I've forgotten exactly what restrictive hack we use to get around
that, but the *only* way to do that fully correctly is to record
the role that did the ALTER and then check its privileges at extension
load time.  I think that 096dd80f3 has made it harder not easier
to get to the point of doing that correctly, because it's added
a feature that we'll have to figure out how to make interoperate
with a correct implementation.

			regards, tom lane

  16. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-27T23:30:57Z

    On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
> > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote:
> >> The right way to do this was not to add some
> >> poorly-explained option to ALTER ROLE, but to record the role OID that
> >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE
> >> setting whether that role (still) has the right to change the
> >> specified setting.  As implemented, this can't possibly track changes
> >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
> >> introducing outright security holes like the one fixed by 13d838815.
>
> > I generally agree.  At least, I think it would be nice to avoid adding a
> > new option if possible.  It's not clear to me why we'd need to also check
> > privileges at login time as opposed to only checking them at ALTER ROLE SET
> > time.
>
> Perhaps there's room to argue about that.  But ISTM that if someone
> does ALTER ROLE SET on the strength of some privilege you granted
> them, and then you regret that and revoke the privilege, then the
> ALTER ROLE setting should not continue to work.  So I would regard
> the session-start-time check as the primary one.  Checking when
> ALTER ROLE is done is just a user-friendliness detail.

From my point of view that is much different from what we're doing
with other database objects.  If some role gets revoked from
privilege, that doesn't affect the actions done with that privilege
before.  The law is not retroactive.  If one has created some tables,
those tables still work if the creator gets revoked privilege or even
gets deleted.  Why should the setting behave differently?

Additionally, I think if we start recording role OID, then we need a
full set of management clauses for each individual option ownership.
Otherwise, we would leave this new role OID without necessarily
management facilities.  But with them, the whole stuff will look like
awful overengineering.

> Also, in the case of an extension-defined GUC, we don't necessarily
> know its privilege level at either ALTER ROLE time or session start,
> since the extension might not yet be loaded at either point.

Yes, that's it.

> I've forgotten exactly what restrictive hack we use to get around
> that,

As I understand the restrictive hack is to assume that the role is at
least SUSET.

> but the *only* way to do that fully correctly is to record
> the role that did the ALTER and then check its privileges at extension
> load time.

This depends on the understanding of correctness.   Recording role OID
would mean altering that role privileges or deleting the role would
affect the settings.  Even if that is correct, this is very very far
from the behavior we had for decades.

> I think that 096dd80f3 has made it harder not easier
> to get to the point of doing that correctly, because it's added
> a feature that we'll have to figure out how to make interoperate
> with a correct implementation.

With 096dd80f3, we still may revoke the setting of USERSET options
from the public.  Even if the option is not currently loaded at ALTER
time, we still may find an explicit revoke recorder in the system
catalog.  That behavior will be current if we understand the default
options as separate material things (as it is today), but not part of
the setter role.

------
Regards,
Alexander Korotkov

  17. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-28T00:04:01Z

    On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Additionally, I think if we start recording role OID, then we need a
> full set of management clauses for each individual option ownership.
> Otherwise, we would leave this new role OID without necessarily
> management facilities.  But with them, the whole stuff will look like
> awful overengineering.

I can also predict a lot of ambiguous cases.  For instance, we
existing setting can be overridden with a different role OID.  If it
has been overridden can the overwriter turn it back?

------
Regards,
Alexander Korotkov

  18. Re: Possible regression setting GUCs on \connect

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-04-28T13:42:03Z

    On 4/27/23 8:04 PM, Alexander Korotkov wrote:
> On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>> Additionally, I think if we start recording role OID, then we need a
>> full set of management clauses for each individual option ownership.
>> Otherwise, we would leave this new role OID without necessarily
>> management facilities.  But with them, the whole stuff will look like
>> awful overengineering.
> 
> I can also predict a lot of ambiguous cases.  For instance, we
> existing setting can be overridden with a different role OID.  If it
> has been overridden can the overwriter turn it back?

[RMT hat]

While the initial bug has been fixed, given there is discussion on 
reverting 096dd80f3, I've added this as an open item.

I want to study this a bit more before providing my own opinion on revert.

Thanks,

Jonathan

  19. Re: Possible regression setting GUCs on \connect

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-28T16:29:21Z

    Hi!

On Fri, 28 Apr 2023 at 17:42, Jonathan S. Katz <jkatz@postgresql.org> wrote:
>
> On 4/27/23 8:04 PM, Alexander Korotkov wrote:
> > On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >> Additionally, I think if we start recording role OID, then we need a
> >> full set of management clauses for each individual option ownership.
> >> Otherwise, we would leave this new role OID without necessarily
> >> management facilities.  But with them, the whole stuff will look like
> >> awful overengineering.
> >
> > I can also predict a lot of ambiguous cases.  For instance, we
> > existing setting can be overridden with a different role OID.  If it
> > has been overridden can the overwriter turn it back?
>
> [RMT hat]
>
> While the initial bug has been fixed, given there is discussion on
> reverting 096dd80f3, I've added this as an open item.
>
> I want to study this a bit more before providing my own opinion on revert.

I see that 096dd80f3 is a lot simpler in implementation than
a0ffa885e, so I agree Alexander's opinion that it's good not to
overengineer what could be done simple. If we patched corner cases of
a0ffa885e before (by 13d838815), why not patch minor things in
096dd80f3 instead of reverting?

As I see in [1] there is some demand from users regarding this option.

Regards,
Pavel Borisov,
Supabase.

  20. Re: Possible regression setting GUCs on \connect

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-04-30T16:25:20Z

    On 4/28/23 12:29 PM, Pavel Borisov wrote:
> Hi!
> 
> On Fri, 28 Apr 2023 at 17:42, Jonathan S. Katz <jkatz@postgresql.org> wrote:
>>
>> On 4/27/23 8:04 PM, Alexander Korotkov wrote:
>>> On Fri, Apr 28, 2023 at 2:30 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>>>> Additionally, I think if we start recording role OID, then we need a
>>>> full set of management clauses for each individual option ownership.
>>>> Otherwise, we would leave this new role OID without necessarily
>>>> management facilities.  But with them, the whole stuff will look like
>>>> awful overengineering.
>>>
>>> I can also predict a lot of ambiguous cases.  For instance, we
>>> existing setting can be overridden with a different role OID.  If it
>>> has been overridden can the overwriter turn it back?
>>
>> [RMT hat]
>>
>> While the initial bug has been fixed, given there is discussion on
>> reverting 096dd80f3, I've added this as an open item.
>>
>> I want to study this a bit more before providing my own opinion on revert.
> 
> I see that 096dd80f3 is a lot simpler in implementation than
> a0ffa885e, so I agree Alexander's opinion that it's good not to
> overengineer what could be done simple. If we patched corner cases of
> a0ffa885e before (by 13d838815), why not patch minor things in
> 096dd80f3 instead of reverting?
> 
> As I see in [1] there is some demand from users regarding this option.

[RMT hat]

I read through the original thread[1] to understand the use case and 
also the concerns, but I need to study [1] and this thread a bit more 
before I can form an opinion.

The argument that there is "demand from users" is certainly one I relate 
to, but there have been high-demand features in the past (e.g. MERGE, 
SQL/JSON) that have been reverted and released later due to various 
concerns around implementation, etc. The main job of the RMT is to 
ensure a major release is on time and is as stable as possible, which 
will be a major factor into any decisions if there is lack of community 
consensus on an open item.

Thanks,

Jonathan

[1] 
https://postgr.es/m/CAGRrpzawQSbuEedicOLRjQRCmSh6nC3HeMNvnQdBVmPMg7AvQw%40mail.gmail.com

  21. Re: Possible regression setting GUCs on \connect

    Michael Paquier <michael@paquier.xyz> — 2023-05-17T06:17:13Z

    On Sun, Apr 30, 2023 at 12:25:20PM -0400, Jonathan S. Katz wrote:
> [RMT hat]
> 
> I read through the original thread[1] to understand the use case and also
> the concerns, but I need to study [1] and this thread a bit more before I
> can form an opinion.
> 
> The argument that there is "demand from users" is certainly one I relate to,
> but there have been high-demand features in the past (e.g. MERGE, SQL/JSON)
> that have been reverted and released later due to various concerns around
> implementation, etc. The main job of the RMT is to ensure a major release is
> on time and is as stable as possible, which will be a major factor into any
> decisions if there is lack of community consensus on an open item.

(note: Not RMT this year)

This thread had no replies for the last two weeks, and beta1 is
planned for next week.  Alexander, what are your plans here?
--
Michael

  22. Re: Possible regression setting GUCs on \connect

    Amit Kapila <amit.kapila16@gmail.com> — 2023-05-17T06:47:47Z

    On Fri, Apr 28, 2023 at 5:01 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Fri, Apr 28, 2023 at 1:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Nathan Bossart <nathandbossart@gmail.com> writes:
> > > On Thu, Apr 27, 2023 at 03:22:04PM -0400, Tom Lane wrote:
> > >> The right way to do this was not to add some
> > >> poorly-explained option to ALTER ROLE, but to record the role OID that
> > >> issued the ALTER ROLE, and then to check when loading the ALTER ROLE
> > >> setting whether that role (still) has the right to change the
> > >> specified setting.  As implemented, this can't possibly track changes
> > >> in GRANT/REVOKE SET privileges correctly, and I wonder if it's not
> > >> introducing outright security holes like the one fixed by 13d838815.
> >
> > > I generally agree.  At least, I think it would be nice to avoid adding a
> > > new option if possible.  It's not clear to me why we'd need to also check
> > > privileges at login time as opposed to only checking them at ALTER ROLE SET
> > > time.
> >
> > Perhaps there's room to argue about that.  But ISTM that if someone
> > does ALTER ROLE SET on the strength of some privilege you granted
> > them, and then you regret that and revoke the privilege, then the
> > ALTER ROLE setting should not continue to work.  So I would regard
> > the session-start-time check as the primary one.  Checking when
> > ALTER ROLE is done is just a user-friendliness detail.
>
> From my point of view that is much different from what we're doing
> with other database objects.  If some role gets revoked from
> privilege, that doesn't affect the actions done with that privilege
> before.  The law is not retroactive.  If one has created some tables,
> those tables still work if the creator gets revoked privilege or even
> gets deleted.  Why should the setting behave differently?
>

I see there are mainly three concerns (a) Avoid adding the new option
USER SET, (b) The behavior of this feature varies from the precedents
set by a0ffa885e and 13d838815, (c)  As per discussion, not following
13d838815 could lead to a similar security hole in this feature.

Now, I don't know whether Tom and or Nathan share your viewpoint and
feel that nothing should be done. It would have been better if such a
discussion happens during development but I can understand that mostly
the other senior people are sometimes busy enough to pay attention to
all the work going on. I see that when Alexander proposed this new
option and behavior in the original thread [1], there were no
objections, so the commit followed the normal community rules but we
have seen various times that post-commit reviews also lead to changing
or reverting the feature.

I see that you seem to think it would be over-engineering to follow
the suggestions shared here but without the patch or some further
discussion, it won't be easy to conclude that.

Tom/Nathan, do you have any further suggestions here?

[1] - https://www.postgresql.org/message-id/CAPpHfdsLd6E--epnGqXENqLP6dLwuNZrPMcNYb3wJ87WR7UBOQ@mail.gmail.com

-- 
With Regards,
Amit Kapila.

  23. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-05-17T11:56:54Z

    Hi, Amit.

On Wed, May 17, 2023 at 9:47 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I see there are mainly three concerns (a) Avoid adding the new option
> USER SET, (b) The behavior of this feature varies from the precedents
> set by a0ffa885e and 13d838815, (c)  As per discussion, not following
> 13d838815 could lead to a similar security hole in this feature.
>
> Now, I don't know whether Tom and or Nathan share your viewpoint and
> feel that nothing should be done. It would have been better if such a
> discussion happens during development but I can understand that mostly
> the other senior people are sometimes busy enough to pay attention to
> all the work going on. I see that when Alexander proposed this new
> option and behavior in the original thread [1], there were no
> objections, so the commit followed the normal community rules but we
> have seen various times that post-commit reviews also lead to changing
> or reverting the feature.
>
> I see that you seem to think it would be over-engineering to follow
> the suggestions shared here but without the patch or some further
> discussion, it won't be easy to conclude that.
>
> Tom/Nathan, do you have any further suggestions here?

I think the main question regarding the USER SET option is its
contradiction with Tom's plans to track the setter role OID per
setting.  If we do track role OID then it makes USER SET both
unnecessary for users and undesired complications for development.
However, I've expressed my doubts about the tracking setter role OID
[1], [2].  I think these plans look good in the big picture, but
implementation will have so many caveats that implementation will
stall for a long time (probably forever).  If we accept this view, the
USER SET option might seem a good practical solution for real-world
issues.

I think if we would elaborate more on tracking setter role OID, come
to at least sketchy design then it could be easily to come to an
agreement on future directions.

Links.
1. https://www.postgresql.org/message-id/CAPpHfdsy-jxhgR0bWk1Fv63c6txwMAkzxFMGMf29jqa9uU_CdQ%40mail.gmail.com
2. https://www.postgresql.org/message-id/CAPpHfdu6roOVEUsV9TWNdQ%3DTZCrNEEwJM62EQiKULUyjpERhtg%40mail.gmail.com

------
Regards,
Alexander Korotkov

  24. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-17T12:08:36Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> Tom/Nathan, do you have any further suggestions here?

My recommendation is to revert this feature.  I do not see any
way that we won't regret it as a poor design.

			regards, tom lane

  25. Re: Possible regression setting GUCs on \connect

    Nathan Bossart <nathandbossart@gmail.com> — 2023-05-17T16:47:04Z

    On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
>> Tom/Nathan, do you have any further suggestions here?
> 
> My recommendation is to revert this feature.  I do not see any
> way that we won't regret it as a poor design.

I agree.  The problem seems worth solving, but I think we ought to consider
a different approach.  Apologies for not chiming in earlier on the original
thread.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  26. Re: Possible regression setting GUCs on \connect

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-05-17T16:57:45Z

    On 5/17/23 12:47 PM, Nathan Bossart wrote:
> On Wed, May 17, 2023 at 08:08:36AM -0400, Tom Lane wrote:
>> Amit Kapila <amit.kapila16@gmail.com> writes:
>>> Tom/Nathan, do you have any further suggestions here?
>>
>> My recommendation is to revert this feature.  I do not see any
>> way that we won't regret it as a poor design.
> 
> I agree.  The problem seems worth solving, but I think we ought to consider
> a different approach.  Apologies for not chiming in earlier on the original
> thread.

[RMT hat, personal opinion]

I do agree that the feature itself is useful, but given there is 
disagreement over the feature design, particularly from people who have 
spent time working on features and analyzing the security ramifications 
in this area, the safest option is to revert and try again for v17.

I suggest we revert before Beta 1.

Thanks,

Jonathan

  27. Re: Possible regression setting GUCs on \connect

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-05-17T17:30:41Z

    Tom,

On Wed, May 17, 2023 at 3:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > Tom/Nathan, do you have any further suggestions here?
>
> My recommendation is to revert this feature.  I do not see any
> way that we won't regret it as a poor design.

I have carefully noted your concerns regarding the USER SET patch that
I've committed.  It's clear that you have strong convictions about
this, particularly in relation to your plan of storing the setter role
OID in pg_db_role_setting.

I want to take a moment to acknowledge the significance of your
perspective and I respect that you have a different view on this
matter.  Although I have not yet had the opportunity to see the
feasibility of your approach, I am open to understanding it further.

Anyway, I don't want to do anything counter-productive.  So, I've
taken the decision to revert the USER SET patch for the time being.

I'm looking forward to continuing working with you on this subject for v17.

------
Regards,
Alexander Korotkov

  28. Re: Possible regression setting GUCs on \connect

    Jonathan S. Katz <jkatz@postgresql.org> — 2023-05-17T19:23:13Z

    On 5/17/23 1:30 PM, Alexander Korotkov wrote:
> Tom,
> 
> On Wed, May 17, 2023 at 3:08 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Amit Kapila <amit.kapila16@gmail.com> writes:
>>> Tom/Nathan, do you have any further suggestions here?
>>
>> My recommendation is to revert this feature.  I do not see any
>> way that we won't regret it as a poor design.
> 
> I have carefully noted your concerns regarding the USER SET patch that
> I've committed.  It's clear that you have strong convictions about
> this, particularly in relation to your plan of storing the setter role
> OID in pg_db_role_setting.
> 
> I want to take a moment to acknowledge the significance of your
> perspective and I respect that you have a different view on this
> matter.  Although I have not yet had the opportunity to see the
> feasibility of your approach, I am open to understanding it further.
> 
> Anyway, I don't want to do anything counter-productive.  So, I've
> taken the decision to revert the USER SET patch for the time being.

Thanks Alexander. I know reverting a feature is not easy and appreciate 
you taking the time to work through this discussion.

> I'm looking forward to continuing working with you on this subject for v17.

+1; I think everyone agrees there is a feature here that will be helpful 
to our users.

Thanks,

Jonathan

  29. Re: Possible regression setting GUCs on \connect

    Robert Haas <robertmhaas@gmail.com> — 2023-05-18T15:47:17Z

    On Wed, May 17, 2023 at 1:31 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> I have carefully noted your concerns regarding the USER SET patch that
> I've committed.  It's clear that you have strong convictions about
> this, particularly in relation to your plan of storing the setter role
> OID in pg_db_role_setting.
>
> I want to take a moment to acknowledge the significance of your
> perspective and I respect that you have a different view on this
> matter.  Although I have not yet had the opportunity to see the
> feasibility of your approach, I am open to understanding it further.
>
> Anyway, I don't want to do anything counter-productive.  So, I've
> taken the decision to revert the USER SET patch for the time being.

This discussion made me go back and look at the commit in question. My
opinion is that the feature as it was committed is quite hard to
understand. The documentation for it said this: "Specifies that
variable should be set on behalf of ordinary role." But what does that
even mean? What's an "ordinary role"? What does "on behalf of" mean? I
think these are not terms we use elsewhere in the documentation, and I
think it wouldn't be easy for users to understand how to use the
feature properly. I'm not sure whether Tom's idea about what the
design should be is good or bad, but I think that whatever we end up
with, we should try to explain more clearly and thoroughly what
problem the feature solves and how it does so.

Imagine a paragraph someplace that says something like "You might want
to do X. But if you try to do it, you'll find that it doesn't work
because Y: [SQL example] We can work around this problem using the Z
feature. That lets us tell the system that it should Q, which fixes Y:
[SQL example].". It sounds like Tom might be proposing that we solve
this problem in some way that doesn't actually require any new SQL
syntax, and if we do that, then this might not be needed. But if we do
add syntax, then I think something like this would be really good to
have.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  30. Re: Possible regression setting GUCs on \connect

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-18T18:33:16Z

    Robert Haas <robertmhaas@gmail.com> writes:
> This discussion made me go back and look at the commit in question. My
> opinion is that the feature as it was committed is quite hard to
> understand. The documentation for it said this: "Specifies that
> variable should be set on behalf of ordinary role." But what does that
> even mean? What's an "ordinary role"? What does "on behalf of" mean?

Yeah.  And even more to the point: how would the feature interact with
per-user grants of SET privilege?  It seems like it would have to ignore
or override that, which is not a conclusion I like at all.

I think that commit a0ffa885e pretty much nailed down the user interface
we want, and what remains is to work out how granting SET privilege
interacts with the time-delayed nature of ALTER USER/DATABASE SET.
But the answer to that does not seem difficult to me: remember who
issued the ALTER and see if they still have SET privilege at the time
we activate a particular entry.

			regards, tom lane