sessions: Migrate to custom indexed session model. #29668

abdelrahman725 · 2024-04-10T11:09:02Z

Fixes #19490.

New custom Session model indexed by realm and user.

Created RealmSession model with fields realm, user and ip_address.

Customized SessionStore to work with RealmSession.

Migrate code to RealmSession to get the performant benefits of the index (e.g. zerver/lib/sessions.py has the most impact)

This introduced one extra query for zerver/tests/test_signup.py and zerver/tests/test_home.py, so we increment the query count param for assert_database_query_count()

zulipbot · 2024-04-10T11:09:45Z

Hello @zulip/server-production members, this pull request was labeled with the "area: production" label, so you may want to check it out!

alexmv

Create RealmSession model with fields: realm, user and ip_address,
indexed by realm and user.

Indexing the IP address would also be useful.

This change introduced one extra query for 'assert_database_query_count()'

Which query, and why?

It also looks like you'll need to add some tests for complete coverage.

alexmv · 2024-04-11T15:36:28Z

zerver/lib/sessions.py

+    for session in RealmSession.objects.filter(realm=user_profile.realm, user=user_profile):
+        delete_session(session)


This is going to do O(n) SQL queries for the delete.

That's fine for now, but (as a future commit) we would like to do this in three steps:

Fetch the matching session keys

Bulk-delete all of the matching sessions from the database using the filter

Bulk-delete all of the matching sessions from the cache using the session keys

alexmv · 2024-04-11T15:36:40Z

zerver/lib/sessions.py

+    for session in RealmSession.objects.filter(realm=realm):
+        delete_session(session)


alexmv · 2024-04-11T15:37:10Z

zerver/lib/sessions.py

-    for session in Session.objects.all():
+    for session in RealmSession.objects.all():
        delete_session(session)


alexmv · 2024-04-11T15:42:18Z

zerver/lib/sessions.py

+    for session in RealmSession.objects.filter(user__isnull=False).select_related("realm", "user"):
+        # Guaranteed to be UserProfile since we are excluding Null users from the queryset.
+        user_profile = cast(UserProfile, session.user)
+        # Guaranteed to be Realm since a user (at least in this session model) always has a realm.
+        user_realm = cast(Realm, session.realm)
+
+        if not user_profile.is_active or user_realm.deactivated:


There's no reason to not do these filters in SQL, with:

from django.db.models import Q for session in RealmSession.objects.filter(Q(user__is_active=False) | Q(realm__deactivated=True)): # ...

These casts would also have been better expressed as user_realm = assert_is_not_none(session.realm) -- we don't want to cast (we almost never do), we just want to tell mypy that we know that it won't be None.

Yes, and i think that filter will also remove the need for the select_related and if condition checks because we now have the right filtered sessions so we can delete them directly

alexmv · 2024-04-11T15:44:48Z

zerver/migrations/0507_sessions_new_custom_model.py

+    operations = [
+        migrations.CreateModel(
+            name="RealmSession",
+            fields=[
+                (
+                    "session_key",
+                    models.CharField(
+                        max_length=40, primary_key=True, serialize=False, verbose_name="session key"
+                    ),
+                ),
+                ("session_data", models.TextField(verbose_name="session data")),
+                ("expire_date", models.DateTimeField(db_index=True, verbose_name="expire date")),
+                ("ip_address", models.GenericIPAddressField(null=True)),
+                (
+                    "realm",
+                    models.ForeignKey(
+                        null=True, on_delete=django.db.models.deletion.CASCADE, to="zerver.realm"
+                    ),
+                ),
+                (
+                    "user",
+                    models.ForeignKey(
+                        null=True,
+                        on_delete=django.db.models.deletion.CASCADE,
+                        to=settings.AUTH_USER_MODEL,
+                    ),
+                ),
+            ],
+        ),


This is a new table. That implies that we need to copy over the data from the existing sessions table -- we don't want to suddenly log out everyone who had sessions in the old table.

Bear in mind that this is a large table. We will want to use a INSERT INTO ... SELECT or similar to do this copy as performantly as possible.

Where should this code (for copying over the data) live ?

We want it be raw SQL query not django orm, right ?

It should be a new database migration, probably with a RunSQL migration.

After INSERT INTO... query is successful, should i delete data from existing session table, TRUNCATE TABLE django_session ?

What is the reason for prefering raw SQL over django orm, performance ?

alexmv · 2024-04-11T15:47:14Z

zerver/models/sessions.py

+    if instance.user is not None:
+        instance.realm_id = instance.user.realm_id


If the user logs out, and instance.user becomes None, should we be unset'ing realm_id?

When user logs out, that instance gets deleted

alexmv · 2024-04-11T15:49:31Z

zerver/models/sessions.py

+    @override
+    def create_model_instance(self, data: Dict[str, Union[int, str]]) -> RealmSession:
+        session_object = super().create_model_instance(data)
+        assert isinstance(session_object, RealmSession)


This implies a bug or limitation in the typing of create_model_instance, since it should ideally be able to determine this type from the get_model_class type, above.

Yes, and in that case assert isinstance is the right way to silent that false alarm from mypy ?

In that case the right way to silence the false alarm is to file a bug with the project whose types aren't correct. We can then make this:

session_object: RealmSession = super().create_model_instance(data) # type: ignore[assignment] # link to bug report

...so we can know to come back and clean this up later.

alexmv · 2024-04-11T15:50:43Z

zerver/views/home.py

@@ -205,6 +205,8 @@ def home_real(request: HttpRequest) -> HttpResponse:
        # that we're a spectator, not a logged-in user.
        user_profile = None

+    request.session["realm_id"] = realm.id


Why is this line required? Why is it only necessary here, and not in some middleware to be more generic?

Maybe needs more investigation, but i put it there based on this suggestion from CZO

You should have an argument for why this change is the right change, not just that it seems to work.

abdelrahman725 · 2024-04-20T12:59:49Z

This change introduced one extra query for 'assert_database_query_count()'

Which query, and why?

@alexmv

UPDATE "zerver_realmsession" SET "session_data" = 'some value"',
"expire_date" = 'some value'::timestamptz, 
"realm_id" = 2, 
"user_id" = 10,
"ip_address" = NULL 
WHERE "zerver_realmsession"."session_key" = 'some value'

That extra query is triggered because in create_realm_session_store() called by zerver.models.create_model_instance() we add the custom fields to RealmSession instance when they are available in the session data, so we need to update that table to save those fields, hence the query.

Fixes zulip#19490 and has other performance benefits. Create RealmSession model with custom fields: realm, user and ip_address, indexed by (realm, user) and ip_address. Migrate code to use that model, "zerver/lib/sessions" has most impact. Create data_migration to copy over data from django_session table to zerver_realmsession table.

zulipbot added size: XL area: performance Performance and scalability issues area: production labels Apr 10, 2024

alexmv reviewed Apr 11, 2024

View reviewed changes

abdelrahman725 force-pushed the session-indexing branch 7 times, most recently from efa5c75 to ef3ad42 Compare April 18, 2024 05:40

abdelrahman725 force-pushed the session-indexing branch from ef3ad42 to 964de7d Compare April 21, 2024 04:34

abdelrahman725 force-pushed the session-indexing branch 3 times, most recently from 61aadc0 to 2edb41e Compare April 29, 2024 12:24

zulipbot added the has conflicts label May 6, 2024

abdelrahman725 force-pushed the session-indexing branch from 2edb41e to b86cb52 Compare May 9, 2024 11:54

zulipbot removed the has conflicts label May 9, 2024

abdelrahman725 force-pushed the session-indexing branch from b86cb52 to e222570 Compare May 9, 2024 12:39

zulipbot added the has conflicts label May 13, 2024

abdelrahman725 force-pushed the session-indexing branch from e222570 to 9cf24bc Compare May 14, 2024 19:20

zulipbot removed the has conflicts label May 14, 2024

abdelrahman725 force-pushed the session-indexing branch from 9cf24bc to 4946373 Compare May 14, 2024 19:21

zulipbot added the has conflicts label May 14, 2024

abdelrahman725 force-pushed the session-indexing branch from 4946373 to 8cc9fc2 Compare May 14, 2024 19:28

zulipbot removed the has conflicts label May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sessions: Migrate to custom indexed session model. #29668

sessions: Migrate to custom indexed session model. #29668

abdelrahman725 commented Apr 10, 2024 •

edited

zulipbot commented Apr 10, 2024

alexmv left a comment

alexmv Apr 11, 2024

alexmv Apr 11, 2024

alexmv Apr 11, 2024

alexmv Apr 11, 2024

abdelrahman725 Apr 12, 2024 •

edited

alexmv Apr 11, 2024

abdelrahman725 Apr 12, 2024

alexmv Apr 12, 2024

abdelrahman725 Apr 14, 2024 •

edited

alexmv Apr 11, 2024

abdelrahman725 Apr 12, 2024

alexmv Apr 11, 2024

abdelrahman725 Apr 12, 2024

alexmv Apr 12, 2024

alexmv Apr 11, 2024

abdelrahman725 Apr 12, 2024

alexmv Apr 12, 2024

abdelrahman725 commented Apr 20, 2024 •

edited

		for session in RealmSession.objects.filter(realm=user_profile.realm, user=user_profile):
		delete_session(session)

		if instance.user is not None:
		instance.realm_id = instance.user.realm_id

sessions: Migrate to custom indexed session model. #29668

Are you sure you want to change the base?

sessions: Migrate to custom indexed session model. #29668

Conversation

abdelrahman725 commented Apr 10, 2024 • edited

New custom Session model indexed by realm and user.

zulipbot commented Apr 10, 2024

alexmv 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

abdelrahman725 Apr 12, 2024 • edited

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

abdelrahman725 Apr 14, 2024 • edited

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

abdelrahman725 commented Apr 20, 2024 • edited

abdelrahman725 commented Apr 10, 2024 •

edited

abdelrahman725 Apr 12, 2024 •

edited

abdelrahman725 Apr 14, 2024 •

edited

abdelrahman725 commented Apr 20, 2024 •

edited