Re: Possible regression setting GUCs on \connect

David Steele <david(at)pgmasters(dot)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