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

AO3-6712 Allow invites to be resent and index invitee_email #4791

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

weeklies
Copy link
Contributor

@weeklies weeklies commented Apr 15, 2024

Issue

https://otwarchive.atlassian.net/browse/AO3-6712

Purpose

Testing Instructions

See Jira issue.

@github-actions github-actions bot added Has Migrations Contains migrations and therefore needs special attention when deploying Awaiting Review labels Apr 15, 2024
@weeklies weeklies changed the title AO3-6712 Allow invites to be resent and index invitee_email AO3-6712 Allow invites to be resent and index invitee_email Apr 15, 2024
@weeklies weeklies marked this pull request as ready for review April 15, 2024 21:30
@Bilka2
Copy link
Contributor

Bilka2 commented Jun 29, 2024

The rails code that produces the slow query in AO3-6666 is Invitation.unredeemed.from_queue.find_by(invitee_email: params[:email]) in invite_requests#show and invite_requests#resend.

Before, first iteration of this feature:

MariaDB [otwarchive_development]> EXPLAIN SELECT `invitations`.* FROM `invitations` WHERE (invitee_email IS NOT NULL and redeemed_at IS NULL) AND `invitations`.`external_author_id` IS NULL AND (`invitations`.`creator_type` = 'Admin' OR `invitations`.`creator_type` IS NULL) AND `invitations`.`invitee_email` = '3@example.com' LIMIT 1;
+------+-------------+-------------+------+-----------------------------------------+-----------------------------------------+---------+-------+------+------------------------------------+
| id   | select_type | table       | type | possible_keys                           | key                                     | key_len | ref   | rows | Extra                              |
+------+-------------+-------------+------+-----------------------------------------+-----------------------------------------+---------+-------+------+------------------------------------+
|    1 | SIMPLE      | invitations | ref  | index_invitations_on_external_author_id | index_invitations_on_external_author_id | 5       | const | 10   | Using index condition; Using where |
+------+-------------+-------------+------+-----------------------------------------+-----------------------------------------+---------+-------+------+------------------------------------+
1 row in set (0.001 sec)

After, with the index added in this PR :

MariaDB [otwarchive_development]> EXPLAIN SELECT `invitations`.* FROM `invitations` WHERE (invitee_email IS NOT NULL and redeemed_at IS NULL) AND `invitations`.`external_author_id` IS NULL AND (`invitations`.`creator_type` = 'Admin' OR `invitations`.`creator_type` IS NULL) AND `invitations`.`invitee_email` = '3@example.com' LIMIT 1;
+------+-------------+-------------+------+----------------------------------------------------------------------------+------------------------------------+---------+-------+------+------------------------------------+
| id   | select_type | table       | type | possible_keys                                                              | key                                | key_len | ref   | rows | Extra                              |
+------+-------------+-------------+------+----------------------------------------------------------------------------+------------------------------------+---------+-------+------+------------------------------------+
|    1 | SIMPLE      | invitations | ref  | index_invitations_on_external_author_id,index_invitations_on_invitee_email | index_invitations_on_invitee_email | 1023    | const | 1    | Using index condition; Using where |
+------+-------------+-------------+------+----------------------------------------------------------------------------+------------------------------------+---------+-------+------+------------------------------------+
1 row in set (0.001 sec)

"Using index condition" sounds pretty good combined with the fact that it's using the invitee_email as the index now:

if parts of the WHERE condition can be evaluated by using only columns from the index, the MySQL server pushes this part of the WHERE condition down to the storage engine. The storage engine then evaluates the pushed index condition by using the index entry and only if this is satisfied is the row read from the table.

So in most cases only one row should be getting read from the table. (Invitee email seems to be intended to be unique, but this is not getting validated so it may be more than one row. But it should be pretty rare.) That sounds like near the best case we could have, so I would say just this index on invitee_email should be enough to get good performance. 👍

It looks like previously it was doing this strategy with the external_author_id index, with external_author_id = NULL. Invitations created via the "request an invite" page do not have an external author set, meaning most invitations have external_author_id = NULL. So it was reading a whole lot of rows -> performance problem.

If there are still performance problems after this PR, we could try to validate invitee_email with uniqueness: true, allow_nil: true on the model and convert to an unique index in the DB. However, adding this uniqueness validation may be a behaviour change so I think it's fine to hold off for now.

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

Note: I only reviewed the new changes, not the code that was reverted in AO3-6666, because that already went through review and QA on staging.

<% if AdminSetting.current.invite_from_queue_enabled? %>
<%= ts("We are sending out %{invites} invitations per day.", invites: AdminSetting.current.invite_from_queue_number) %>
<%= t(".send_rate", invites: AdminSetting.current.invite_from_queue_number) %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Completely optional, nitpick: It would be nice if this also used count as the interpolation variable and had the one/other distinction in the locale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Migrations Contains migrations and therefore needs special attention when deploying Reviewed: Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants