Skip to content

Commit e90dd66

Browse files
authored
refactor: SessionContext doesn't require Arc (#15491)
Refactor: `SessionContext` doesn't require `Arc` Embrace idiomatic Rust practices by reducing unnecessary sharing. In this case, since `SessionContext` is not shared among threads, there is no need to wrap it in an `Arc`. This change streamlines the code and enhances performance by eliminating unneeded atomic reference counting.
1 parent 88f7a76 commit e90dd66

File tree

5 files changed

+55
-47
lines changed

5 files changed

+55
-47
lines changed

src/query/service/src/sessions/session.rs

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ use crate::sessions::SessionType;
4646
pub struct Session {
4747
pub(in crate::sessions) id: String,
4848
pub(in crate::sessions) typ: RwLock<SessionType>,
49-
pub(in crate::sessions) session_ctx: Arc<SessionContext>,
50-
pub(in crate::sessions) privilege_mgr: SessionPrivilegeManagerImpl,
49+
pub(in crate::sessions) session_ctx: Box<SessionContext>,
5150
status: Arc<RwLock<SessionStatus>>,
5251
pub(in crate::sessions) mysql_connection_id: Option<u32>,
5352
format_settings: FormatSettings,
@@ -57,17 +56,15 @@ impl Session {
5756
pub fn try_create(
5857
id: String,
5958
typ: SessionType,
60-
session_ctx: Arc<SessionContext>,
59+
session_ctx: Box<SessionContext>,
6160
mysql_connection_id: Option<u32>,
6261
) -> Result<Arc<Session>> {
6362
let status = Arc::new(Default::default());
64-
let privilege_mgr = SessionPrivilegeManagerImpl::new(session_ctx.clone());
6563
Ok(Arc::new(Session {
6664
id,
6765
typ: RwLock::new(typ),
6866
status,
6967
session_ctx,
70-
privilege_mgr,
7168
mysql_connection_id,
7269
format_settings: FormatSettings::default(),
7370
}))
@@ -114,7 +111,8 @@ impl Session {
114111
}
115112

116113
pub fn quit(self: &Arc<Self>) {
117-
let session_ctx = self.session_ctx.clone();
114+
let session_ctx = self.session_ctx.as_ref();
115+
118116
if session_ctx.get_current_query_id().is_some() {
119117
if let Some(shutdown_fun) = session_ctx.take_io_shutdown_tx() {
120118
shutdown_fun();
@@ -138,9 +136,7 @@ impl Session {
138136
}
139137

140138
pub fn force_kill_query(self: &Arc<Self>, cause: ErrorCode) {
141-
let session_ctx = self.session_ctx.clone();
142-
143-
if let Some(context_shared) = session_ctx.get_query_context_shared() {
139+
if let Some(context_shared) = self.session_ctx.get_query_context_shared() {
144140
context_shared.kill(cause);
145141
}
146142
}
@@ -205,7 +201,11 @@ impl Session {
205201
}
206202

207203
pub fn get_current_user(self: &Arc<Self>) -> Result<UserInfo> {
208-
self.privilege_mgr.get_current_user()
204+
self.privilege_mgr().get_current_user()
205+
}
206+
207+
pub fn privilege_mgr(&self) -> SessionPrivilegeManagerImpl<'_> {
208+
SessionPrivilegeManagerImpl::new(self.session_ctx.as_ref())
209209
}
210210

211211
// set_authed_user() is called after authentication is passed in various protocol handlers, like
@@ -218,21 +218,23 @@ impl Session {
218218
user: UserInfo,
219219
restricted_role: Option<String>,
220220
) -> Result<()> {
221-
self.privilege_mgr
221+
self.privilege_mgr()
222222
.set_authed_user(user, restricted_role)
223223
.await
224224
}
225225

226226
#[async_backtrace::framed]
227227
pub async fn validate_available_role(self: &Arc<Self>, role_name: &str) -> Result<RoleInfo> {
228-
self.privilege_mgr.validate_available_role(role_name).await
228+
self.privilege_mgr()
229+
.validate_available_role(role_name)
230+
.await
229231
}
230232

231233
// Only the available role can be set as current role. The current role can be set by the SET
232234
// ROLE statement, or by the `session.role` field in the HTTP query request body.
233235
#[async_backtrace::framed]
234236
pub async fn set_current_role_checked(self: &Arc<Self>, role_name: &str) -> Result<()> {
235-
self.privilege_mgr
237+
self.privilege_mgr()
236238
.set_current_role(Some(role_name.to_string()))
237239
.await
238240
}
@@ -242,33 +244,33 @@ impl Session {
242244
self: &Arc<Self>,
243245
role_names: Option<Vec<String>>,
244246
) -> Result<()> {
245-
self.privilege_mgr.set_secondary_roles(role_names).await
247+
self.privilege_mgr().set_secondary_roles(role_names).await
246248
}
247249

248250
pub fn get_current_role(self: &Arc<Self>) -> Option<RoleInfo> {
249-
self.privilege_mgr.get_current_role()
251+
self.privilege_mgr().get_current_role()
250252
}
251253

252254
pub fn get_secondary_roles(self: &Arc<Self>) -> Option<Vec<String>> {
253-
self.privilege_mgr.get_secondary_roles()
255+
self.privilege_mgr().get_secondary_roles()
254256
}
255257

256258
#[async_backtrace::framed]
257259
pub async fn unset_current_role(self: &Arc<Self>) -> Result<()> {
258-
self.privilege_mgr.set_current_role(None).await
260+
self.privilege_mgr().set_current_role(None).await
259261
}
260262

261263
// Returns all the roles the current session has. If the user have been granted restricted_role,
262264
// the other roles will be ignored.
263265
// On executing SET ROLE, the role have to be one of the available roles.
264266
#[async_backtrace::framed]
265267
pub async fn get_all_available_roles(self: &Arc<Self>) -> Result<Vec<RoleInfo>> {
266-
self.privilege_mgr.get_all_available_roles().await
268+
self.privilege_mgr().get_all_available_roles().await
267269
}
268270

269271
#[async_backtrace::framed]
270272
pub async fn get_all_effective_roles(self: &Arc<Self>) -> Result<Vec<RoleInfo>> {
271-
self.privilege_mgr.get_all_effective_roles().await
273+
self.privilege_mgr().get_all_effective_roles().await
272274
}
273275

274276
#[async_backtrace::framed]
@@ -280,7 +282,7 @@ impl Session {
280282
if matches!(self.get_type(), SessionType::Local) {
281283
return Ok(());
282284
}
283-
self.privilege_mgr
285+
self.privilege_mgr()
284286
.validate_privilege(object, privilege)
285287
.await
286288
}
@@ -290,12 +292,12 @@ impl Session {
290292
if matches!(self.get_type(), SessionType::Local) {
291293
return Ok(true);
292294
}
293-
self.privilege_mgr.has_ownership(object).await
295+
self.privilege_mgr().has_ownership(object).await
294296
}
295297

296298
#[async_backtrace::framed]
297299
pub async fn get_visibility_checker(&self) -> Result<GrantObjectVisibilityChecker> {
298-
self.privilege_mgr.get_visibility_checker().await
300+
self.privilege_mgr().get_visibility_checker().await
299301
}
300302

301303
pub fn get_settings(self: &Arc<Self>) -> Arc<Settings> {
@@ -328,9 +330,7 @@ impl Session {
328330
}
329331

330332
pub fn set_query_priority(&self, priority: u8) {
331-
let session_ctx = self.session_ctx.clone();
332-
333-
if let Some(context_shared) = session_ctx.get_query_context_shared() {
333+
if let Some(context_shared) = self.session_ctx.get_query_context_shared() {
334334
context_shared.set_priority(priority);
335335
}
336336
}

src/query/service/src/sessions/session_ctx.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ pub struct SessionContext {
7070
}
7171

7272
impl SessionContext {
73-
pub fn try_create(settings: Arc<Settings>, typ: SessionType) -> Result<Arc<Self>> {
74-
Ok(Arc::new(SessionContext {
73+
pub fn try_create(settings: Arc<Settings>, typ: SessionType) -> Result<Self> {
74+
Ok(SessionContext {
7575
settings,
7676
abort: Default::default(),
7777
current_user: Default::default(),
@@ -87,7 +87,7 @@ impl SessionContext {
8787
query_ids_results: Default::default(),
8888
typ,
8989
txn_mgr: Mutex::new(TxnManager::init()),
90-
}))
90+
})
9191
}
9292

9393
// Get abort status.

src/query/service/src/sessions/session_info.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,12 @@ use crate::sessions::SessionType;
2626

2727
impl Session {
2828
pub fn process_info(self: &Arc<Self>) -> ProcessInfo {
29-
let session_ctx = self.session_ctx.clone();
30-
self.to_process_info(&session_ctx)
29+
self.to_process_info()
3130
}
3231

33-
fn to_process_info(self: &Arc<Self>, session_ctx: &SessionContext) -> ProcessInfo {
32+
fn to_process_info(self: &Arc<Self>) -> ProcessInfo {
33+
let session_ctx = self.session_ctx.as_ref();
34+
3435
let mut memory_usage = 0;
3536

3637
let shared_query_context = &session_ctx.get_query_context_shared();

src/query/service/src/sessions/session_mgr.rs

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,12 @@ impl SessionManager {
145145
};
146146

147147
let session_ctx = SessionContext::try_create(settings, typ.clone())?;
148-
let session = Session::try_create(id.clone(), typ.clone(), session_ctx, mysql_conn_id)?;
148+
let session = Session::try_create(
149+
id.clone(),
150+
typ.clone(),
151+
Box::new(session_ctx),
152+
mysql_conn_id,
153+
)?;
149154

150155
self.try_add_session(session.clone(), typ.clone())?;
151156

@@ -357,14 +362,18 @@ impl SessionManager {
357362

358363
let mut queries_profiles = HashMap::new();
359364
for weak_ptr in active_sessions {
360-
if let Some(session_ctx) = weak_ptr.upgrade().map(|x| x.session_ctx.clone()) {
361-
if let Some(context_shared) = session_ctx.get_query_context_shared() {
362-
if let Some(executor) = context_shared.executor.read().upgrade() {
363-
queries_profiles.insert(
364-
context_shared.init_query_id.as_ref().read().clone(),
365-
executor.get_profiles(),
366-
);
367-
}
365+
let Some(arc_sesssion) = weak_ptr.upgrade() else {
366+
continue;
367+
};
368+
369+
let session_ctx = arc_sesssion.session_ctx.as_ref();
370+
371+
if let Some(context_shared) = session_ctx.get_query_context_shared() {
372+
if let Some(executor) = context_shared.executor.read().upgrade() {
373+
queries_profiles.insert(
374+
context_shared.init_query_id.as_ref().read().clone(),
375+
executor.get_profiles(),
376+
);
368377
}
369378
}
370379
}

src/query/service/src/sessions/session_privilege_mgr.rs

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,6 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use std::sync::Arc;
16-
1715
use databend_common_exception::ErrorCode;
1816
use databend_common_exception::Result;
1917
use databend_common_meta_app::principal::GrantObject;
@@ -73,12 +71,12 @@ pub trait SessionPrivilegeManager {
7371
// fn show_grants(&self);
7472
}
7573

76-
pub struct SessionPrivilegeManagerImpl {
77-
session_ctx: Arc<SessionContext>,
74+
pub struct SessionPrivilegeManagerImpl<'a> {
75+
session_ctx: &'a SessionContext,
7876
}
7977

80-
impl SessionPrivilegeManagerImpl {
81-
pub fn new(session_ctx: Arc<SessionContext>) -> Self {
78+
impl<'a> SessionPrivilegeManagerImpl<'a> {
79+
pub fn new(session_ctx: &'a SessionContext) -> Self {
8280
Self { session_ctx }
8381
}
8482

@@ -126,7 +124,7 @@ impl SessionPrivilegeManagerImpl {
126124
}
127125

128126
#[async_trait::async_trait]
129-
impl SessionPrivilegeManager for SessionPrivilegeManagerImpl {
127+
impl<'a> SessionPrivilegeManager for SessionPrivilegeManagerImpl<'a> {
130128
// set_authed_user() is called after authentication is passed in various protocol handlers, like
131129
// HTTP handler, clickhouse query handler, mysql query handler. auth_role represents the role
132130
// granted by external authenticator, it will over write the current user's granted roles, and

0 commit comments

Comments
 (0)