feat(session_config): system wide session config by alter system set #16062

yuhao-su · 2024-04-02T01:33:12Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Allow set a system-wide session config by alter system set xxx to yyy

The session config will be initiated in every new session using the system-wide session config

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

You can set a system-wide default value for a session parameter, from which parameters in a new session will be initialized.

alter system set session_param_name to session_param_value

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

…hao/system_session_var

BugenZhao · 2024-04-02T04:46:23Z

Is this PR ready for review?

src/meta/model_v2/src/session_parameter.rs

yuhao-su · 2024-04-02T14:54:02Z

src/common/src/session_config/transaction_isolation_level.rs

+    type Err = &'static str;

-    fn from_str(_s: &str) -> Result<Self, Self::Err> {
-        bail_not_implemented!(issue = 10736, "isolation level");
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {


Simply bail will cause deserialization fail

yuhao-su · 2024-04-02T14:55:21Z

src/meta/src/controller/session_params.rs

+        // FIXME: use a real reporter
+        let reporter = &mut ();


@TennyZhuang do we need to report this when alter system?

kwannoel

Is it possible for a session variable to be entirely unconfigured?

Consider some session variable A.

User did not set A.
User did not set default for A system wide either.

Then I suppose it will fallback to the default we wrote for it in the code?
Is it possible for session variables to be None by default after this PR?

yuhao-su · 2024-04-03T03:47:30Z

Is it possible for a session variable to be entirely configured?

No. You can check the SessionConfig struct and there is no Option. In the case you mentioned, it will be set to the default value.

BugenZhao · 2024-04-03T05:50:00Z

proto/meta.proto

@@ -593,6 +593,27 @@ service SystemParamsService {
  rpc SetSystemParam(SetSystemParamRequest) returns (SetSystemParamResponse);
 }

+message GetSessionParamsRequest {}


Can we first unify the terms? Or is there any difference between "session config" and "session param"?

I tried to use session config in frontend. But I guess it's ok to just use session params everywhere

But let me rename it in the next pr.

Hmmm, why renaming in next

You probably won't want to review 200 lines of rename in this pr...

BugenZhao · 2024-04-03T05:51:51Z

src/common/proc_macro/src/session_config.rs

+            /// Show all parameters except those specified `NO_SHOW_ALL`.
            pub fn show_all(&self) -> Vec<VariableInfo> {
                vec![
                    #(#show_all_list)*
                ]
            }
+
+            /// List all parameters
+            pub fn list_all(&self) -> Vec<VariableInfo> {


The difference of these two methods is subtle. Is NO_SHOW_ALL essentially hidden? 😄

BTW, which set of parameters should be attributed with NO_SHOW_ALL and why do they need this? Is there any documentation from Postgres that we can refer to?

@TennyZhuang

src/common/src/session_config/mod.rs

BugenZhao · 2024-04-03T05:54:55Z

src/common/src/session_config/transaction_isolation_level.rs

-    fn from_str(_s: &str) -> Result<Self, Self::Err> {
-        bail_not_implemented!(issue = 10736, "isolation level");
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {


Should ignore the case.

BugenZhao · 2024-04-03T05:56:45Z

src/frontend/src/session.rs

+                )));
+            }
+        };
+        let session_config = self.env.meta_client().get_session_params().await?;


This looks bad. Should we adopt the notification/subscription mechanism just like the catalogs?

Maybe it is enough to only load the config when the frontend starts for now?
https://www.postgresql.org/docs/current/sql-altersystem.html

Values set with ALTER SYSTEM will be effective after the next server configuration reload, or after the next server restart in the case of parameters that can only be changed at server start. A server configuration reload can be commanded by calling the SQL function pg_reload_conf(), running pg_ctl reload, or sending a SIGHUP signal to the main server process.

The reload behavior of different config depends on its context value, according to https://www.postgresql.org/docs/current/view-pg-settings.html. We may verify the behavior for calling ALTER SYSTEM on these session configs in Postgres and provide a precise definition then.

We have no plan to support config file for session parameters cc @fuyufjh

We have no plan to support config file for session parameters

Yes. Different from PG, RW is distributed, which naturally makes config file different from the system params. I'd like to consider them as non-overlapped sets (except state store's initial config, which is the only exception)

See also https://github.com/risingwavelabs/rfcs/blob/main/rfcs/0034-system-parameters.md

src/meta/model_v2/migration/src/lib.rs

src/meta/src/manager/env.rs

BugenZhao · 2024-04-03T06:02:28Z

Please add some e2e tests to demonstrate the behavior.

fuyufjh

The approach in this PR is acceptable to me, but it seems far away from the thing you folks discussed before - that is, unifying the session params and global params.

The rest LGTM.

fuyufjh · 2024-04-04T09:27:36Z

src/common/src/session_config/mod.rs

    /// If `RW_IMPLICIT_FLUSH` is on, then every INSERT/UPDATE/DELETE statement will block
    /// until the entire dataflow is refreshed. In other words, every related table & MV will
    /// be able to see the write.
+    #[serde(rename = "rw_implicit_flush")]
    #[parameter(default = false, rename = "rw_implicit_flush")]
    implicit_flush: bool,


2 renames here 😄. How about just renaming the field to rw_implicit_flush

Rename this will cause too many unrelated changes to this pr. Maybe later.

src/meta/src/manager/session_params.rs

src/frontend/src/handler/alter_system.rs

src/meta/model_v2/src/session_parameter.rs

src/frontend/src/session.rs

fuyufjh

LGTM!

BugenZhao

Rest LGTM

BugenZhao · 2024-04-12T04:51:59Z

e2e_test/error_ui/simple/main.slt

-
-
-statement error
-set transaction_isolation to 'read committed';


This case is for testing the error ui for setting an invalid value to a session variable. If you find this not applicable to transaction_isolation, please replace it with another session variable instead of simply removing it.

src/common/proc_macro/src/session_config.rs

BugenZhao · 2024-04-12T04:57:31Z

src/frontend/src/meta_client.rs

+            serde_json::from_str(&self.0.get_session_params().await?)
+                .context("failed to parse session config")?;


This essentially introduces compatibility issue for session variables. Please document it somewhere and consider covering it with backwards-compat tests.

This will not have compatibility issue as long as the meta and front version are the same

You're right. JSON is only used as an intermediate representation.

However, we still persist the keys and values. Prior to this PR, we avoid breaking the session variables (like names or value types) only with best effort to provide SQL-level compatibility. After this PR, however, we must be more careful about that. 😕

I guess this is also true for many other things we persisted. For session params if we want to change the name, we can provide a persist key name.

src/frontend/src/observer/observer_manager.rs

src/frontend/src/session.rs

src/meta/src/controller/session_params.rs

e2e_test/ddl/alter_session_params.slt

yuhao-su added 6 commits March 12, 2024 14:02

init

5c4feaa

add proto

ca6f6d0

Merge branch 'main' of github.com:singularity-data/risingwave into yu…

32ba5e9

…hao/system_session_var

add backend

144e252

resolve

7c08141

fix

67342c9

yuhao-su requested review from hzxa21, fuyufjh, yezizp2012, BugenZhao, st1page and kwannoel April 2, 2024 01:33

github-actions bot added Invalid PR Title type/feature labels Apr 2, 2024

yuhao-su changed the title ~~feature(session_config): system wide session config by alter system set~~ feat(session_config): system wide session config by alter system set Apr 2, 2024

github-actions bot removed the Invalid PR Title label Apr 2, 2024

yuhao-su commented Apr 2, 2024

View reviewed changes

yuhao-su requested a review from TennyZhuang April 2, 2024 14:56

yuhao-su marked this pull request as ready for review April 2, 2024 14:56

yuhao-su requested a review from a team as a code owner April 2, 2024 14:56

kwannoel reviewed Apr 3, 2024

View reviewed changes

BugenZhao reviewed Apr 3, 2024

View reviewed changes

yuhao-su added 3 commits April 3, 2024 11:31

e2e and fix

d2f19bd

fix e2e

ca0122d

fix

03e628c

fuyufjh reviewed Apr 4, 2024

View reviewed changes

comment

9994716

yuhao-su added 3 commits April 8, 2024 23:02

fix test

feac7df

fix

e7d1c76

fix

5cd809f

yezizp2012 reviewed Apr 9, 2024

View reviewed changes

src/frontend/src/session.rs Outdated Show resolved Hide resolved

yuhao-su added 3 commits April 9, 2024 11:27

resolve

4ec9547

fix

0414e4e

use init notification

415f3af

yuhao-su requested review from BugenZhao, yezizp2012, kwannoel and fuyufjh April 9, 2024 18:32

yuhao-su added the user-facing-changes Contains changes that are visible to users label Apr 9, 2024

yuhao-su added 5 commits April 10, 2024 10:41

resolve

7c9e5fd

fix

70e15c8

resolve

4139c1a

fix

770b324

remove serde rename

88b79aa

fuyufjh approved these changes Apr 12, 2024

View reviewed changes

BugenZhao reviewed Apr 12, 2024

View reviewed changes

improve

1010558

yuhao-su added this pull request to the merge queue Apr 12, 2024

Merged via the queue into main with commit 1d25497 Apr 12, 2024
30 of 31 checks passed

yuhao-su deleted the yuhao/system_session_var branch April 12, 2024 16:44

neverchanje mentioned this pull request Apr 12, 2024

Document: feat(session_config): system wide session config by alter system set risingwavelabs/risingwave-docs#2065

Closed

ShanlanLi mentioned this pull request Apr 15, 2024

Test risingwavelabs/risingwave-docs#2068

Closed

2 tasks

kwannoel mentioned this pull request Apr 16, 2024

fix(compatibility): backwards compatibility of session params #16310

Merged

9 tasks

Chengyi-Ying mentioned this pull request Apr 22, 2024

add alter system command and use it for system wide session config risingwavelabs/risingwave-docs#2092

Merged

2 tasks

yuhao-su mentioned this pull request May 1, 2024

set timezone globally (as a system parameter?) #16524

Closed

BugenZhao mentioned this pull request May 21, 2024

risingwave 1.9.1 risingwavelabs/homebrew-risingwave#38

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(session_config): system wide session config by alter system set #16062

feat(session_config): system wide session config by alter system set #16062

yuhao-su commented Apr 2, 2024 •

edited by Chengyi-Ying

Loading

BugenZhao commented Apr 2, 2024 •

edited

Loading

yuhao-su Apr 2, 2024

yuhao-su Apr 2, 2024

kwannoel left a comment •

edited

Loading

yuhao-su commented Apr 3, 2024

BugenZhao Apr 3, 2024

yuhao-su Apr 3, 2024

yuhao-su Apr 3, 2024

fuyufjh Apr 4, 2024

yuhao-su Apr 7, 2024

BugenZhao Apr 3, 2024

BugenZhao Apr 3, 2024

yuhao-su Apr 3, 2024

BugenZhao Apr 3, 2024

BugenZhao Apr 3, 2024

st1page Apr 3, 2024

BugenZhao Apr 3, 2024

yuhao-su Apr 3, 2024

fuyufjh Apr 4, 2024 •

edited

Loading

BugenZhao commented Apr 3, 2024

fuyufjh left a comment

fuyufjh Apr 4, 2024

yuhao-su Apr 9, 2024

fuyufjh left a comment

BugenZhao left a comment

BugenZhao Apr 12, 2024

BugenZhao Apr 12, 2024

yuhao-su Apr 12, 2024

BugenZhao Apr 12, 2024

yuhao-su Apr 12, 2024



		statement error
		set transaction_isolation to 'read committed';

		serde_json::from_str(&self.0.get_session_params().await?)
		.context("failed to parse session config")?;

feat(session_config): system wide session config by alter system set #16062

feat(session_config): system wide session config by alter system set #16062

Conversation

yuhao-su commented Apr 2, 2024 • edited by Chengyi-Ying Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

BugenZhao commented Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwannoel left a comment • edited Loading

Choose a reason for hiding this comment

yuhao-su commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

BugenZhao commented Apr 3, 2024

fuyufjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhao-su commented Apr 2, 2024 •

edited by Chengyi-Ying

Loading

BugenZhao commented Apr 2, 2024 •

edited

Loading

kwannoel left a comment •

edited

Loading

fuyufjh Apr 4, 2024 •

edited

Loading