[PATCH] New default role allowing to change per-role/database settings

On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael.banck@credativ.de>
wrote:

Hi,

in today's world, some DBAs have no superuser rights, but we can
delegate them additional powers like CREATEDB or the pg_monitor default
role etc. Usually, the DBA can also view the database logs, either via
shell access or some web interface.

One thing that I personally find lacking is that it is not possible to
change role-specific log settings (like log_statement = 'mod' for a
security sensitive role) without being SUPERUSER as their GUC context is
"superuser". This makes setup auditing much harder if there is no
SUPERUSER access, also pgaudit then only allows to configure object-
based auditing. Amazon RDS e.g. has patched Postgres to allow the
cluster owner/pseudo-superuser `rds_superuser' to change those log
settings that define what/when we log something, while keeping the
"where to log" entries locked down.

The attached patch introduces a new guc context "administrator" (better
names/bikeshedding for this welcome) that is meant as a middle ground
between "superuser" and "user". It also adds a new default role
"pg_change_role_settings" (better names also welcome) that can be
granted to DBAs so that they can change the "administrator"-context GUCs
on a per-role (or per-database) basis. Whether the latter should be
included is maybe debatable, but I added both on the basis that they are
the same "source".

The initial set of "administrator" GUCs are all current GUCs with
"superuser" context from these categories:

* Reporting and Logging / What to Log
* Reporting and Logging / When to
Log
* Statistics / Query and Index Statistics Collector
* Statistics /
Monitoring

Of course, further categories (or particular GUCs) could be added now or
in the future, e.g. RDS also patches the following GUCs in their v12
offering:

* temp_file_limit
* session_replication_role

RDS does *not* patch log_transaction_sample_rate from "Reporting and
Logging / When to Log", but that might be more of an oversight than a
security consideration, or does anybody see a big problem with that
(compared to the others in that set)?

I initially pondered not introducing a new context but just filtering on
category, but as categories seem to be only descriptive in guc.c and not
used for any policy decisions so far, I have abandoned this pretty
quickly.

Thoughts?

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

The patch (0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_role_.patch)
does
not apply successfully and has some hunks failed.

http://cfbot.cputube.org/patch_32_2918.log

1 out of 23 hunks FAILED -- saving rejects to file
src/backend/utils/misc/guc.c.rej
patching file src/include/catalog/catversion.h
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/catalog/catversion.h.rej
patching file src/include/catalog/pg_authid.dat

Can we get a rebase?

I am marking the patch "Waiting on Author".

--
Ibrar Ahmed

Hi,

Am Montag, den 08.03.2021, 20:54 +0500 schrieb Ibrar Ahmed:

On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael.banck@credativ.de> wrote:

in today's world, some DBAs have no superuser rights, but we can
delegate them additional powers like CREATEDB or the pg_monitor default
role etc. Usually, the DBA can also view the database logs, either via
shell access or some web interface.

One thing that I personally find lacking is that it is not possible to
change role-specific log settings (like log_statement = 'mod' for a
security sensitive role) without being SUPERUSER as their GUC context is
"superuser". This makes setup auditing much harder if there is no
SUPERUSER access, also pgaudit then only allows to configure object-
based auditing. Amazon RDS e.g. has patched Postgres to allow the
cluster owner/pseudo-superuser `rds_superuser' to change those log
settings that define what/when we log something, while keeping the
"where to log" entries locked down.

The attached patch introduces a new guc context "administrator" (better
names/bikeshedding for this welcome) that is meant as a middle ground
between "superuser" and "user". It also adds a new default role
"pg_change_role_settings" (better names also welcome) that can be
granted to DBAs so that they can change the "administrator"-context GUCs
on a per-role (or per-database) basis. Whether the latter should be
included is maybe debatable, but I added both on the basis that they are
the same "source".

The initial set of "administrator" GUCs are all current GUCs with
"superuser" context from these categories:

* Reporting and Logging / What to Log
* Reporting and Logging / When to
Log
* Statistics / Query and Index Statistics Collector
* Statistics /
Monitoring

Of course, further categories (or particular GUCs) could be added now or
in the future, e.g. RDS also patches the following GUCs in their v12
offering:

* temp_file_limit
* session_replication_role

RDS does *not* patch log_transaction_sample_rate from "Reporting and
Logging / When to Log", but that might be more of an oversight than a
security consideration, or does anybody see a big problem with that
(compared to the others in that set)?

I initially pondered not introducing a new context but just filtering on
category, but as categories seem to be only descriptive in guc.c and not
used for any policy decisions so far, I have abandoned this pretty
quickly.

Thoughts?

Michael

The patch (0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_role_.patch) does
not apply successfully and has some hunks failed.

Thanks for letting me know.

http://cfbot.cputube.org/patch_32_2918.log
1 out of 23 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej
patching file src/include/catalog/catversion.h
Hunk #1 FAILED at 53.
1 out of 1 hunk FAILED -- saving rejects to file src/include/catalog/catversion.h.rej
patching file src/include/catalog/pg_authid.dat
Can we get a rebase?

Please find attached the rebase; I've removed the catversion hunk as I
believe it is customary to leave that to committers.

I am marking the patch "Waiting on Author".

I've put that back to "Needs Review".

Cheers,

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Greetings Michael,

* Michael Banck (michael.banck@credativ.de) wrote:

Am Montag, den 08.03.2021, 20:54 +0500 schrieb Ibrar Ahmed:

On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael.banck@credativ.de> wrote:

in today's world, some DBAs have no superuser rights, but we can
delegate them additional powers like CREATEDB or the pg_monitor default
role etc. Usually, the DBA can also view the database logs, either via
shell access or some web interface.

One thing that I personally find lacking is that it is not possible to
change role-specific log settings (like log_statement = 'mod' for a
security sensitive role) without being SUPERUSER as their GUC context is
"superuser". This makes setup auditing much harder if there is no
SUPERUSER access, also pgaudit then only allows to configure object-
based auditing. Amazon RDS e.g. has patched Postgres to allow the
cluster owner/pseudo-superuser `rds_superuser' to change those log
settings that define what/when we log something, while keeping the
"where to log" entries locked down.

The attached patch introduces a new guc context "administrator" (better
names/bikeshedding for this welcome) that is meant as a middle ground
between "superuser" and "user". It also adds a new default role
"pg_change_role_settings" (better names also welcome) that can be
granted to DBAs so that they can change the "administrator"-context GUCs
on a per-role (or per-database) basis. Whether the latter should be
included is maybe debatable, but I added both on the basis that they are
the same "source".

If we're going to make the context be called 'administrator' then it
would seem like we should make the predefined role be
"pg_change_admin_settings"...? Thoughts on that?

The initial set of "administrator" GUCs are all current GUCs with
"superuser" context from these categories:

* Reporting and Logging / What to Log
* Reporting and Logging / When to Log
* Statistics / Query and Index Statistics Collector
* Statistics / Monitoring

Just to be clear- the predefined role being added here actually allows a
user with this role to change both 'admin' and 'user' GUCs across all
databases and across all non-superuser roles, right? (A bit
disappointed at the lack of any documentation included in this patch..
presumably that would have made it clear).

Of course, further categories (or particular GUCs) could be added now or
in the future, e.g. RDS also patches the following GUCs in their v12
offering:

* temp_file_limit

Being able to set temp_file_limit certainly seems appropriate.

* session_replication_role

I'm less sure about session_replication_role ...

RDS does *not* patch log_transaction_sample_rate from "Reporting and
Logging / When to Log", but that might be more of an oversight than a
security consideration, or does anybody see a big problem with that
(compared to the others in that set)?

I tend to agree that it should be included, I don't see any particular
reason why that would be worse than, say, log_statement.

I initially pondered not introducing a new context but just filtering on
category, but as categories seem to be only descriptive in guc.c and not
used for any policy decisions so far, I have abandoned this pretty
quickly.

Yeah, context is how these have been managed before and it seems to make
sense to continue with that general idea.

Please find attached the rebase; I've removed the catversion hunk as I
believe it is customary to leave that to committers.

Yeah, generally no need to include that in the patch.

This commit introduces `administrator' as a middle ground between `superuser'
and `user' for guc contexts.

Those settings initially include all previously `superuser'-context settings
from the 'Reporting and Logging' ('What/When to Log' but not 'Where to Log')
and both 'Statistics' categories. Further settings could be added in follow-up
commits.

Also, a new default role pg_change_role_settings is introduced. This allows
administrators (that are not superusers) that have been granted this role to
change the above PGC_ADMINSET settings on a per-role/database basis like "ALTER
ROLE [...] SET log_statement".

The rationale is to allow certain settings that affect logging to be changed
in setups where the DBA has access to the database log, but is not a full
superuser.

I don't really see an issue with this approach but I do have to admit to
wondering about ALTER SYSTEM. Any thoughts regarding that..?

---
src/backend/commands/dbcommands.c | 7 +++-
src/backend/commands/user.c | 7 ++--
src/backend/utils/misc/guc.c | 56 +++++++++++++++++++------------
src/include/catalog/pg_authid.dat | 5 +++
src/include/utils/guc.h | 1 +
5 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 2b159b60eb..a59b1dbeb8 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1657,7 +1657,12 @@ AlterDatabaseSet(AlterDatabaseSetStmt *stmt)
*/
shdepLockAndCheckObject(DatabaseRelationId, datid);

-	if (!pg_database_ownercheck(datid, GetUserId()))
+	/*
+	 * Only allow the database owner or a member of the
+	 * pg_change_role_settings default role to change database settings.
+	 */
+	if (!pg_database_ownercheck(datid, GetUserId()) &&
+		!is_member_of_role(GetUserId(), DEFAULT_ROLE_CHANGE_ROLE_SETTINGS))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
stmt->dbname);

I have to admit to wondering if we should still require database owner
rights when it comes to this (that is- possibly require both?). With
this approach, the user with the predefined role could change the
settings for any database, even one they don't have rights to connect
to..

diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 87d917ffc3..b86cd1e4f3 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -64,5 +64,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '8801', oid_symbol => 'DEFAULT_ROLE_CHANGE_ROLE_SETTINGS',
+  rolname => 'pg_change_role_settings', rolsuper => 'f', rolinherit => 't',
+  rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+  rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+  rolpassword => '_null_', rolvaliduntil => '_null_' },

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Thanks!

Stephen

Hi,

Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:

* Michael Banck (michael.banck@credativ.de) wrote:

Am Montag, den 08.03.2021, 20:54 +0500 schrieb Ibrar Ahmed:

On Thu, Dec 31, 2020 at 6:16 PM Michael Banck <michael.banck@credativ.de> wrote:

in today's world, some DBAs have no superuser rights, but we can
delegate them additional powers like CREATEDB or the pg_monitor default
role etc. Usually, the DBA can also view the database logs, either via
shell access or some web interface.

One thing that I personally find lacking is that it is not possible to
change role-specific log settings (like log_statement = 'mod' for a
security sensitive role) without being SUPERUSER as their GUC context is
"superuser". This makes setup auditing much harder if there is no
SUPERUSER access, also pgaudit then only allows to configure object-
based auditing. Amazon RDS e.g. has patched Postgres to allow the
cluster owner/pseudo-superuser `rds_superuser' to change those log
settings that define what/when we log something, while keeping the
"where to log" entries locked down.

The attached patch introduces a new guc context "administrator" (better
names/bikeshedding for this welcome) that is meant as a middle ground
between "superuser" and "user". It also adds a new default role
"pg_change_role_settings" (better names also welcome) that can be
granted to DBAs so that they can change the "administrator"-context GUCs
on a per-role (or per-database) basis. Whether the latter should be
included is maybe debatable, but I added both on the basis that they are
the same "source".

If we're going to make the context be called 'administrator' then it
would seem like we should make the predefined role be
"pg_change_admin_settings"...? Thoughts on that?

Well, I think the "administrator" guc context would be much less visible
than a "pg_change_admin_settings" predefined role. As a user, I would
assume such a predefined role would provide something more universal
than what is proposed here. That could still be the case later on, of
course.

Maybe somebody else has another idea on naming things?

The initial set of "administrator" GUCs are all current GUCs with
"superuser" context from these categories:

* Reporting and Logging / What to Log
* Reporting and Logging / When to Log
* Statistics / Query and Index Statistics Collector
* Statistics / Monitoring

Just to be clear- the predefined role being added here actually allows a
user with this role to change both 'admin' and 'user' GUCs across all
databases and across all non-superuser roles, right? (A bit
disappointed at the lack of any documentation included in this patch..
presumably that would have made it clear).

About all databases, see below for your suggestion to limit it to the
intersection of this predefined role and the database owner.

Not exactly sure what you mean with "both 'admin' and 'user' GUCs", but
yeah, one could classify the above categories as such I guess. Other
SUSET-GUCs like the developer options are not touched, though.

I did not
include documentation in the initial patch because I wasn't sure whether
there was any interest in it (and admittedly, the commitfest deadline
came up I think), I can work on that now.

Of course, further categories (or particular GUCs) could be added now or
in the future, e.g. RDS also patches the following GUCs in their v12
offering:

* temp_file_limit

Being able to set temp_file_limit certainly seems appropriate.

* session_replication_role

I'm less sure about session_replication_role ...

I'll keep those two out for now.

RDS does *not* patch log_transaction_sample_rate from "Reporting and
Logging / When to Log", but that might be more of an oversight than a
security consideration, or does anybody see a big problem with that
(compared to the others in that set)?

I tend to agree that it should be included, I don't see any particular
reason why that would be worse than, say, log_statement.

Ok.

[...]

This commit introduces `administrator' as a middle ground between `superuser'
and `user' for guc contexts.

Those settings initially include all previously `superuser'-context settings
from the 'Reporting and Logging' ('What/When to Log' but not 'Where to Log')
and both 'Statistics' categories. Further settings could be added in follow-up
commits.

Also, a new default role pg_change_role_settings is introduced. This allows
administrators (that are not superusers) that have been granted this role to
change the above PGC_ADMINSET settings on a per-role/database basis like "ALTER
ROLE [...] SET log_statement".

The rationale is to allow certain settings that affect logging to be changed
in setups where the DBA has access to the database log, but is not a full
superuser.

I don't really see an issue with this approach but I do have to admit to
wondering about ALTER SYSTEM. Any thoughts regarding that..?

Not sure what you mean here?

---
src/backend/commands/dbcommands.c | 7 +++-
src/backend/commands/user.c | 7 ++--
src/backend/utils/misc/guc.c | 56 +++++++++++++++++++------------
src/include/catalog/pg_authid.dat | 5 +++
src/include/utils/guc.h | 1 +
5 files changed, 52 insertions(+), 24 deletions(-)

diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c
index 2b159b60eb..a59b1dbeb8 100644
--- a/src/backend/commands/dbcommands.c
+++ b/src/backend/commands/dbcommands.c
@@ -1657,7 +1657,12 @@ AlterDatabaseSet(AlterDatabaseSetStmt *stmt)
*/
shdepLockAndCheckObject(DatabaseRelationId, datid);

-	if (!pg_database_ownercheck(datid, GetUserId()))
+	/*
+	 * Only allow the database owner or a member of the
+	 * pg_change_role_settings default role to change database settings.
+	 */
+	if (!pg_database_ownercheck(datid, GetUserId()) &&
+		!is_member_of_role(GetUserId(), DEFAULT_ROLE_CHANGE_ROLE_SETTINGS))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
stmt->dbname);

I have to admit to wondering if we should still require database owner
rights when it comes to this (that is- possibly require both?). With
this approach, the user with the predefined role could change the
settings for any database, even one they don't have rights to connect
to..

That's a good point. I have to admit my itch was mostly about role-based
settings and database-based settings are kinda in there as well.

I will
ponder this some more, but it seems to me it would be better to err on
the more restrictive side of things for now and we could open it up
later?

diff --git a/src/include/catalog/pg_authid.dat b/src/include/catalog/pg_authid.dat
index 87d917ffc3..b86cd1e4f3 100644
--- a/src/include/catalog/pg_authid.dat
+++ b/src/include/catalog/pg_authid.dat
@@ -64,5 +64,10 @@
rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
rolpassword => '_null_', rolvaliduntil => '_null_' },
+{ oid => '8801', oid_symbol => 'DEFAULT_ROLE_CHANGE_ROLE_SETTINGS',
+  rolname => 'pg_change_role_settings', rolsuper => 'f', rolinherit => 't',
+  rolcreaterole => 'f', rolcreatedb => 'f', rolcanlogin => 'f',
+  rolreplication => 'f', rolbypassrls => 'f', rolconnlimit => '-1',
+  rolpassword => '_null_', rolvaliduntil => '_null_' },

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Right, will send a rebased patch ASAP.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email:
michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen:
https://www.credativ.de/datenschutz

Hi,

Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck:

Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Right, will send a rebased patch ASAP.

Here's a rebased version that also removes the ALTER DATABASE SET
capability from this new predefined role and adds some documentation.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email:
michael.banck@credativ.de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen:
https://www.credativ.de/datenschutz

On Wed, Apr 7, 2021 at 5:23 PM Michael Banck <michael.banck@credativ.de> wrote:

Hi,

Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck:

Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Right, will send a rebased patch ASAP.

Here's a rebased version that also removes the ALTER DATABASE SET
capability from this new predefined role and adds some documentation.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Regards,
Vignesh

Hi,

Am Mittwoch, den 14.07.2021, 21:29 +0530 schrieb vignesh C:

On Wed, Apr 7, 2021 at 5:23 PM Michael Banck <michael.banck@credativ.de> wrote:

Hi,

Am Dienstag, den 06.04.2021, 15:37 +0200 schrieb Michael Banck:

Am Montag, den 05.04.2021, 14:33 -0400 schrieb Stephen Frost:

Should drop the 'DEFAULT_' to match the others since the rename to
'predefined' roles went in.

Right, will send a rebased patch ASAP.

Here's a rebased version that also removes the ALTER DATABASE SET
capability from this new predefined role and adds some documentation.

The patch does not apply on Head anymore, could you rebase and post a
patch. I'm changing the status to "Waiting for Author".

Thanks for letting me know, I've attached a rebased v4 of this patch, no
other changes.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
E-Mail: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer, Geoff Richardson, Peter Lilley

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

Thanks for letting me know, I've attached a rebased v4 of this patch, no other
changes.

I tried it, but when I used set command, tab completion did not work properly and an error occurred.

---
postgres=> \conninfo
You are connected to database "postgres" as user "aaa" via socket in "/tmp" at port "5432".
postgres=> \du
List of roles
Role name | Attributes | Member of
-----------+------------------------------------------------------------+---------------------------
aaa | | {pg_change_role_settings}
penguin | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
postgres=> show log
log_autovacuum_min_duration log_executor_stats log_min_error_statement log_replication_commands log_timezone
log_checkpoints log_file_mode log_min_messages log_rotation_age log_transaction_sample_rate
log_connections log_hostname log_parameter_max_length log_rotation_size log_truncate_on_rotation
log_destination log_line_prefix log_parameter_max_length_on_error log_statement logging_collector
log_disconnections log_lock_waits log_parser_stats log_statement_sample_rate logical_decoding_work_mem
log_duration log_min_duration_sample log_planner_stats log_statement_stats
log_error_verbosity log_min_duration_statement log_recovery_conflict_waits log_temp_files
postgres=> show log_duration ;
log_duration
--------------
off
(1 row)

postgres=> set log
log_parameter_max_length_on_error logical_decoding_work_mem
postgres=> set log_duration to on;
2021-09-08 16:23:39.216 JST [533860] ERROR: permission denied to set parameter "log_duration"
2021-09-08 16:23:39.216 JST [533860] STATEMENT: set log_duration to on;
ERROR: permission denied to set parameter "log_duration"
---

Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Hi,

On Wed, Sep 08, 2021 at 07:38:25AM +0000, Shinya11.Kato@nttdata.com wrote:

Thanks for letting me know, I've attached a rebased v4 of this patch, no other
changes.

I tried it, but when I used set command, tab completion did not work properly and an error occurred.

Also, the patch doesn't apply anymore:

http://cfbot.cputube.org/patch_36_2918.log

=== Applying patches on top of PostgreSQL commit ID 5513dc6a304d8bda114004a3b906cc6fde5d6274 ===
=== applying patch ./v4-0001-Add-new-PGC_ADMINSET-guc-context-and-pg_change_ro.patch
[...]
1 out of 23 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej

I'm switching the patch to Waiting on Author.

Hi,

Am Mittwoch, dem 08.09.2021 um 07:38 +0000 schrieb
Shinya11.Kato@nttdata.com:

Thanks for letting me know, I've attached a rebased v4 of this
patch, no other changes.

Please find attached another rebase, sorry it took so long.

I tried it, but when I used set command, tab completion did not work >
properly and an error occurred.

It's been a while, but I believe the patch only changes the ALTER ROLE
command, not the SET/SHOW commands. I thought that was more generally
useful, can you explain the SET use-case?

---
postgres=> \conninfo
You are connected to database "postgres" as user "aaa" via socket in
"/tmp" at port "5432".
postgres=> \du
                                           List of roles
 Role name |                        
Attributes                         |         Member of        
-----------+---------------------------------------------------------
---+---------------------------
 aaa      
|                                                            |
{pg_change_role_settings}
 penguin   | Superuser, Create role, Create DB, Replication, Bypass
RLS | {}
postgres=> show log
log_autovacuum_min_duration        log_executor_stats                
log_min_error_statement            log_replication_commands          
log_timezone
log_checkpoints                    log_file_mode                     
log_min_messages                   log_rotation_age                  
log_transaction_sample_rate
log_connections                    log_hostname                      
log_parameter_max_length           log_rotation_size                 
log_truncate_on_rotation
log_destination                    log_line_prefix                   
log_parameter_max_length_on_error  log_statement                     
logging_collector
log_disconnections                 log_lock_waits                    
log_parser_stats                   log_statement_sample_rate         
logical_decoding_work_mem
log_duration                       log_min_duration_sample           
log_planner_stats                  log_statement_stats               
log_error_verbosity                log_min_duration_statement        
log_recovery_conflict_waits        log_temp_files                    
postgres=> show log_duration ;
 log_duration
--------------
 off
(1 row)

postgres=> set log
log_parameter_max_length_on_error  logical_decoding_work_mem
postgres=> set log_duration to on;
2021-09-08 16:23:39.216 JST [533860] ERROR:  permission denied to set
parameter "log_duration"
2021-09-08 16:23:39.216 JST [533860] STATEMENT:  set log_duration to
on;
ERROR:  permission denied to set parameter "log_duration"

So this would work:

postgres=> SHOW ROLE;
role
--------------
rolesetadmin
(1 row)

postgres=> \du
List of roles
Role name | Attributes | Member of
--------------+------------------------------------------------------------+---------------------------
postgres | Superuser, Create role, Create DB, Replication, Bypass RLS | {}
rolesetadmin | Cannot login | {pg_change_role_settings}
test | Cannot login | {}

postgres=> ALTER ROLE test SET log_statement='all';
ALTER ROLE
postgres=> \drds
List of settings
Role | Database | Settings
------+----------+-------------------
test | | log_statement=all
(1 row)

I am not sure if there is anything to be done about tab completion, can
you clarify here?

Michael

--
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
E-Mail: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz

As discussed in [1]/messages/by-id/flat/0ab66589-2f71-69b3-2002-49e821740b0d@timescale.com, we're taking this opportunity to return some
patchsets that don't appear to be getting enough reviewer interest.

This is not a rejection, since we don't necessarily think there's
anything unacceptable about the entry, but it differs from a standard
"Returned with Feedback" in that there's probably not much actionable
feedback at all. Rather than code changes, what this patch needs is more
community interest. You might

- ask people for help with your approach,
- see if there are similar patches that your code could supplement,
- get interested parties to agree to review your patch in a CF, or
- possibly present the functionality in a way that's easier to review
overall.

(Doing these things is no guarantee that there will be interest, but
it's hopefully better than endlessly rebasing a patchset that is not
receiving any feedback from the community.)

Once you think you've built up some community support and the patchset
is ready for review, you (or any interested party) can resurrect the
patch entry by visiting

https://commitfest.postgresql.org/38/2918/

and changing the status to "Needs Review", and then changing the
status again to "Move to next CF". (Don't forget the second step;
hopefully we will have streamlined this in the near future!)

Thanks,
--Jacob

[1]: /messages/by-id/flat/0ab66589-2f71-69b3-2002-49e821740b0d@timescale.com
/messages/by-id/flat/0ab66589-2f71-69b3-2002-49e821740b0d@timescale.com