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

perf(session): Avoid select when canceling session #5140

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

tmessi
Copy link
Member

@tmessi tmessi commented Sep 30, 2024

When a resource relating to a session is deleted,
notably a target, host, or auth_token, the corresponding sessions are
marked as canceled via a database trigger that calls the cancel_session
function. This function was performing a select as part of the insert
call to ensure that it would not attempt to insert a session_sate of
canceled if it was an invalid state transition. However, since this
function was first introduced, there has been additional constraints
added that would prevent an invalid state transition. In addition,
performing a select as part of an insert increases the chances for
contention with other queries that interact with the session_state
table, especially since a large number of sessions and session_state
rows can be included in a single transaction when things like
auth_tokens or targets get deleted.

Now, the cancel_function will perform the insert, and catch any of the
potential violations, allowing the query to continue without needing to
perform the select, thus reduction the potential for contention.

When a resource relating to a session is deleted,
notably a target, host, or auth_token, the corresponding sessions are
marked as canceled via a database trigger that calls the cancel_session
function. This function was performing a select as part of the insert
call to ensure that it would not attempt to insert a session_sate of
canceled if it was an invalid state transition. However, since this
function was first introduced, there has been additional constraints
added that would prevent an invalid state transition. In addition,
performing a select as part of an insert increases the chances for
contention with other queries that interact with the session_state
table, especially since a large number of sessions and session_state
rows can be included in a single transaction when things like
auth_tokens or targets get deleted.

Now, the cancel_function will perform the insert, and catch any of the
potential violations, allowing the query to continue without needing to
perform the select, thus reduction the potential for contention.
@tmessi tmessi added this to the 0.18.x milestone Sep 30, 2024
@tmessi tmessi marked this pull request as ready for review September 30, 2024 18:40
Copy link

Database schema diff between main and tmessi-session-cancel-trigger @ 91fff8e

To understand how these diffs are generated and some limitations see the
documentation of the script.

Functions

diff --git a/.schema-diff/funcs_37bf59243a7a67fdb19ce942cde425301ed8eb12/cancel_session.sql b/.schema-diff/funcs_aef96cd9331c0ddfacec56532e63a298213c36a1/cancel_session.sql
index b685e5be6..98c8d5624 100644
--- a/.schema-diff/funcs_37bf59243a7a67fdb19ce942cde425301ed8eb12/cancel_session.sql
+++ b/.schema-diff/funcs_aef96cd9331c0ddfacec56532e63a298213c36a1/cancel_session.sql
@@ -23,31 +23,20 @@ set row_security = off;
 create function public.cancel_session(sessionid text) returns void
     language plpgsql
     as $$
-declare
-  rows_affected numeric;
-begin
-  insert into session_state(session_id, state)
-  select
-    sessionid::text, 'canceling'
-  from
-    session s
-  where
-      s.public_id = sessionid::text and
-      s.public_id not in (
-      select
-        session_id
-      from
-        session_state
-      where
-          session_id = sessionid::text and
-          state in('canceling','terminated')
-    ) limit 1;
-  get diagnostics rows_affected = row_count;
-  if rows_affected > 1 then
-    raise exception 'cancel session: more than one row affected: %', rows_affected;
-  end if;
-end;
-$$;
+  declare
+    rows_affected numeric;
+  begin
+    insert into session_state(session_id, state)
+         values              (sessionid,  'canceling');
+  exception when unique_violation
+              or foreign_key_violation
+              or check_violation
+            then
+    -- do nothing. any one of these violations would indicate that the session
+    -- either already is canceled, or is in a terminated state and cannot
+    -- be canceled.
+  end;
+  $$;
 
 
 --

Tables

Unchanged

Views

Unchanged

Triggers

Unchanged

Indexes

Unchanged

Constraints

Unchanged

Foreign Key Constraints

Unchanged

Copy link
Collaborator

@louisruch louisruch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tmessi tmessi merged commit 3609658 into main Oct 1, 2024
66 checks passed
@tmessi tmessi deleted the tmessi-session-cancel-trigger branch October 1, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants