Skip to content

Fixing cross joins in lib-banzai

Omar Qunsul requested to merge 417466-fix-cross-joins-in-lib-banzai into master

What does this MR do and why?

In the MR: !124319 (merged) we temporarily allow-listed cross joins between users and namespaces. In this MR we are fixing the cross joins in lib/banzai between these two tables, to prepare them to be on separate databases, as part of the Cells project.

We follow these guidelines on how to resolve cross joins between tables that would belong to different databases: https://docs.gitlab.com/ee/development/database/multiple_databases.html#removing-joins-between-ci-and-non-ci-tables

Addressing: #417466 (closed)

Changelog: fixed

How to verify the changes locally

A. To verify the change on UserReferenceParser

Create an issue and reference users, using for example @root. When you save the issue the link should be rendered and point to the User personal namespace.

B. To verify the change on UserParser

Create an issue and reference some group or @all. When you save the issue:

  1. The members of the projects are added, as long as the group doesn't have mentioned_disabled
  2. With @all, all the project members are added to the subscribers as well. You will need to test that by having members added to the project, but not the parent group. This is assuming that disable_all_mention FF is not enabled.

Newly introduced database queries

In UserReferenceFilter#namespaces

  • Preloading users on namespaces
SELECT * FROM "users" WHERE "users"."id" IN (1, 2, 3, 1614863)

https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23217/commands/74690

In UserParser

SELECT "project_authorizations"."user_id" FROM "project_authorizations" WHERE "project_authorizations"."project_id" IN (6, 7, 13083)

https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/23285/commands/74854

This query is bad in performance, but should happen only when we mention @all on project issue. It's not worth optimzing for two reasons:

  1. It's not a common case
  2. There is a plan to disable the @all. See https://gitlab.com/gitlab-org/gitlab/-/blob/master/config/feature_flags/development/disable_all_mention.yml
SELECT DISTINCT "members"."user_id" FROM "members" WHERE "members"."type" = 'GroupMember' AND "members"."source_type" = 'Namespace' AND "members"."source_id" IN (SELECT "namespaces"."id" FROM "namespaces" WHERE "namespaces"."type" = 'Group' AND "namespaces"."id" = 9970 AND (mentions_disabled IS NOT TRUE)) AND "members"."requested_at" IS NULL AND "members"."invite_token" IS NULL AND (members.access_level > 5)

https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/23315/commands/74944

SELECT * from users where ID (1, 13, 5, 8, 9)

The plan has been shared before

Some remarks

  1. In UserReferenceParser: Even though I am removing the cross joins with routes, this cross join is still enforced from inside Namespace.where_full_path_in, which itself has another allowed cross join to be solved later in #420046. I still have to include preload(:route) in this method, because I cannot guarantee that in the future Namespace.where_full_path_in will keep :route preloaded on the Namespace objects.
  2. I removed the verifies the number of queries test because the two update_issue requests are not equal, and cause different code paths to be executed. The first one caused an Note with a user mention to be rendered. While the 2nd update has a not that doesn't a mention. This causes discrepancy in the number of queries. So I decided to remove this test, because it doesn't make sense to test requests at such a high level. Many parts of the app are touched by this request.

Open Questions

  1. Do you think it makes sense to execute the new logic in UserParser under a feature flag? what are the consequences of something does not go as planned in the UserParser? maybe that'a question to @engwan
  2. If a feature flag is introduced, should I make the actor current_user, because that's available on the BaseParser.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #417466 (closed)

Edited by Omar Qunsul

Merge request reports