Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: Support user/role-level defaults for session variables #21151

Closed
bdarnell opened this issue Jan 2, 2018 · 8 comments · Fixed by #68128
Closed

sql: Support user/role-level defaults for session variables #21151

bdarnell opened this issue Jan 2, 2018 · 8 comments · Fixed by #68128
Assignees
Labels
A-security A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@bdarnell
Copy link
Contributor

bdarnell commented Jan 2, 2018

In postgres, any session variable can be set at the user/role level so it will be set automatically when that user connects. For example, this could be used to set a user-scope default time zone without changing the global default from UTC or dealing with the raciness of cluster settings.

ALTER USER foo SET timezone='America/New_York';

This is especially useful because postgres drivers don't typically have the facility to automatically issue a SET TIME ZONE command at the start of each session (a feature which is commonly found in mysql drivers, for example).

Epic CRDB-2507

@bdarnell bdarnell added feature A-sql-pgcompat Semantic compatibility with PostgreSQL labels Jan 2, 2018
@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed feature labels Apr 24, 2018
@vy-ton
Copy link
Contributor

vy-ton commented Oct 6, 2020

Another use case that comes up is being able to configure statement_timeout, idle_in_session_timeout, or idle_in_transaction_session_timeout for different roles associated with different workloads.

@awoods187
Copy link
Contributor

The lack of flexibility in settings these on a user/role basis has come back to limit us a few times now.

@bdarnell
Copy link
Contributor Author

bdarnell commented Oct 8, 2020

Another example is in the recurring debate between @andy-kimball and myself about how disruptive it is to close a client connection. One reason that postgres drivers don't have good ways to reestablish session variables after a connection reset is because in postgres you can set these variables on the user/role.

@rafiss rafiss added the E-starter Might be suitable for a starter project for new employees or team members. label Nov 24, 2020
@knz
Copy link
Contributor

knz commented Feb 3, 2021

copy-pasting my comment from elsewhere:

When looking at this the importance of role-level (or db-level, or application_name-level) settings should not be underestimated. An implementation restricted to user-level configs would not be sufficient.

This is important to consider, because the security best practice would be to use different SQL user accounts within the same client app, to distinguish privilege groups, and a lack of shared configurability dicincentivizes that.

@knz knz changed the title sql: Support user-level defaults for session variables sql: Support user/role-level defaults for session variables Feb 3, 2021
@bdarnell
Copy link
Contributor Author

bdarnell commented Feb 3, 2021

Yes, it should cover both users and roles (I don't think we supported roles at the time this issue was created).

I'm not sure about db or appname-level settings. I could see it being useful, but it brings us farther away from postgres compatibility (while user and role settings bring us closer). The more places we have to define settings, the trickier it becomes to define precedence and avoid surprises.

@ajwerner
Copy link
Contributor

ajwerner commented Feb 3, 2021

The Postgres docs are quiet on how defaults for session variables get inherited. It's a tough problem because a user may be granted many roles. How would we decide which setting to use if there are conflicts. My guess (haven't tested) is that Postgres doesn't try.

@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
@rafiss
Copy link
Collaborator

rafiss commented May 26, 2021

It looks like there is a pg_roles.rolconfig where the defaults are stored. https://www.postgresql.org/docs/current/view-pg-roles.html

I'll check out the behavior with inherited roles / conflicts.

@knz knz removed the E-starter Might be suitable for a starter project for new employees or team members. label May 26, 2021
@knz knz added the A-security label May 26, 2021
@rafiss
Copy link
Collaborator

rafiss commented May 27, 2021

Postgres does not implement inheritance of session variable defaults. However, there is an order of precedence for "all role" defaults versus "specific role" defaults. See https://github.com/postgres/postgres/blob/d9809bf8694c17e05537c5dd96cde3e67c02d52a/src/backend/utils/init/postinit.c#L1166-L1170

SQL Experience will start work on this in the June milestone.

@rafiss rafiss self-assigned this May 27, 2021
craig bot pushed a commit that referenced this issue Jul 28, 2021
67955: roachtest: remove passing test from ruby-pg blocklist r=RichardJCai a=rafiss

fixes #67912

Release note: None

68001: sql/parser: add syntax for ALTER ROLE ... SET r=RichardJCai a=rafiss

touches #21151

Note that the telemetry re-parsing logic does not work. This is
consistent with the existing ALTER ROLE statement, and should be fixed
separately.

Release note (sql change): Add syntax for `ALTER ROLE ... SET`
statements. The business logic for these statements is not implemented
yet, but a later commit will add it. The following forms are supported:

- ALTER ROLE { name | ALL } [ IN DATABASE database_name ] SET var { TO | = } { value | DEFAULT }
- ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET var
- ALTER ROLE { name | ALL } [ IN DATABASE database_name ] RESET ALL

As with other statements, the keywords ROLE and USER are interchangeable.

This matches the PostgreSQL syntax: https://www.postgresql.org/docs/13/sql-alterrole.html

Co-authored-by: Rafi Shamim <[email protected]>
craig bot pushed a commit that referenced this issue Jul 28, 2021
68030: sql: implement write path for ALTER ROLE ... SET r=RichardJCai a=rafiss

Touches #21151

ALTER ROLE ... SET will now persist the default settings in the
system.database_role_settings table. The behavior matches Postgres in
the following ways:
- RESET does not validate the setting name.
- SET validates both the name and the proposed default value.
- The defaults are stored in a string array, in key=val format.

The privilege checking is slightly different: we diverge from Postgres
by prohibiting users from modifying their own defaults unless they
have CREATEROLE. This is analogous to our restriction that prevents
a user from modifying their own password.

There is no release note since this does not yet affect any
user-behavior. Everything in this commit is write-only

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
craig bot pushed a commit that referenced this issue Aug 5, 2021
68245: sql implement the pg_db_role_setting table r=RichardJCai a=rafiss

touches #21151

Release note (sql change): The pg_db_role_setting table of the
pg_catalog is now implemented. When `ALTER ROLE ... SET var`
is used to configure per-role defaults, these default settings will be
populated in pg_db_role_setting. This table contains the same data no
matter which database the current session is using.

See https://www.postgresql.org/docs/13/catalog-pg-db-role-setting.html

Co-authored-by: Rafi Shamim <[email protected]>
@craig craig bot closed this as completed in c19b1e3 Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security A-sql-pgcompat Semantic compatibility with PostgreSQL C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants