Use default project filter for issue/merge request project searches (!67921) · Merge requests · GitLab.org / GitLab

Terri Chu requested to merge 327106-use-default-filter-for-project-issue-and-merge_request-searches into master Aug 11, 2021

What does this MR do?

Related to #327106

This relates to basic search queries which are backed by PostgreSQL.

The issues and merge requests searches both use the respective Finder classes to search for records. Unless the default_project_filter variable is set to true, both immediately add on a where clause to limit by projects the user has access to (see code for issues and merge requests). This is causing a double filter in the Project scoped searches and increasing response times.

This MR:

Sets default_project_filter to true for Project scoped searches
Replaces an active record call with a scope

Note: This also removes from the Project scoped merge requests searches filtering by source_project_id. This will bring the PostgreSQL query in line with the Elasticsearch query which currently only takes target_project_id into account.

Database timings

All timings done with the gitlab-org/gitlab project in database-lab. There are no user_ids in the query because the permission is checked prior to running a search and will not run against the project if the permission is not allowed.

Merge Requests

Before

Explain plan: https://console.postgres.ai/shared/a858edd6-771c-48b3-bee9-5bdde4dcc594

SQL

SELECT
    "merge_requests".*
FROM ((
        SELECT
            "merge_requests".*
        FROM
            "merge_requests"
        WHERE
            "merge_requests"."source_project_id" IN (
                SELECT
                    "projects"."id"
                FROM
                    "projects"
                WHERE
                    "projects"."id" = 278964))
        UNION (
            SELECT
                "merge_requests".*
            FROM
                "merge_requests"
            WHERE
                "merge_requests"."target_project_id" IN (
                    SELECT
                        "projects"."id"
                    FROM
                        "projects"
                    WHERE
                        "projects"."id" = 278964))) merge_requests
    WHERE
        "merge_requests"."target_project_id" = 278964
        AND ("merge_requests"."title" ILIKE '%test%'
            OR "merge_requests"."description" ILIKE '%test%')
ORDER BY
    created_at DESC
LIMIT 20 OFFSET 0

cold cache

Time: 17.921 s
  - planning: 20.467 ms
  - execution: 17.900 s
    - I/O read: 2.565 s
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 1381722 (~10.50 GiB) from the buffer pool
  - reads: 77374 (~604.50 MiB) from the OS file cache, including disk I/O
  - dirtied: 196 (~1.50 MiB)
  - writes: 0

warm cache

Time: 16.453 s
  - planning: 16.348 ms
  - execution: 16.437 s
    - I/O read: 30.965 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 1457324 (~11.10 GiB) from the buffer pool
  - reads: 1075 (~8.40 MiB) from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

After

Explain plan: https://console.postgres.ai/shared/3df378be-bfcb-45d5-8e50-2ca6394a7cfd

SQL

SELECT
    "merge_requests".*
FROM
    "merge_requests"
WHERE
    "merge_requests"."target_project_id" = 278964
    AND ("merge_requests"."title" ILIKE '%test%'
        OR "merge_requests"."description" ILIKE '%test%')
ORDER BY
    created_at DESC
LIMIT 20 OFFSET 0

cold cache

Time: 31.360 ms
  - planning: 11.422 ms
  - execution: 19.938 ms
    - I/O read: 16.999 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 56 (~448.00 KiB) from the buffer pool
  - reads: 120 (~960.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 1 (~8.00 KiB)
  - writes: 0

warm cache

Time: 10.662 ms
  - planning: 8.114 ms
  - execution: 2.548 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 124 (~992.00 KiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

Issues

Before

Explain plan: https://console.postgres.ai/shared/817cd13c-8aa7-4f3e-9b5c-460a4589e024

SQL

SELECT
    "issues".*
FROM
    "issues"
WHERE
    "issues"."project_id" = 278964
    AND ("issues"."title" ILIKE '%test%'
        OR "issues"."description" ILIKE '%test%')
    AND "issues"."project_id" IN (
        SELECT
            "projects"."id"
        FROM
            "projects"
        WHERE
            "projects"."id" = 278964)
ORDER BY
    created_at DESC
LIMIT 20 OFFSET 0

cold cache

Time: 147.876 ms
  - planning: 22.071 ms
  - execution: 125.805 ms
    - I/O read: 119.882 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 50 (~400.00 KiB) from the buffer pool
  - reads: 95 (~760.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 1 (~8.00 KiB)
  - writes: 0

warm cache

Time: 20.567 ms
  - planning: 17.205 ms
  - execution: 3.362 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 143 (~1.10 MiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

After

Explain plan: https://console.postgres.ai/shared/aa28ae78-7444-4bfc-b87b-d0eb431bd1f8

SQL

SELECT
    "issues".*
FROM
    "issues"
WHERE
    "issues"."project_id" = 278964
    AND ("issues"."title" ILIKE '%test%'
        OR "issues"."description" ILIKE '%test%')
ORDER BY
    created_at DESC
LIMIT 20 OFFSET 0

cold cache

Time: 31.464 ms
  - planning: 18.412 ms
  - execution: 13.052 ms
    - I/O read: 9.542 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 131 (~1.00 MiB) from the buffer pool
  - reads: 9 (~72.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 1 (~8.00 KiB)
  - writes: 0

warm cache

Time: 19.544 ms
  - planning: 16.230 ms
  - execution: 3.314 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 138 (~1.10 MiB) from the buffer pool
  - reads: 0 from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

Screenshots or Screencasts (strongly suggested)

How to setup and validate locally (strongly suggested)

Note: If you have Advanced Search enabled you will need to navigate to the Admin UI and disable Search with Elasticsearch

Navigate to the search UI
Select a project from the dropdown
Perform a search
Validate that the SQL does not have double filtering for projects

Does this MR meet the acceptance criteria?

Conformity

I have included changelog trailers, or none are needed. (Does this MR need a changelog?)
I have added/updated documentation, or it's not needed. (Is documentation required?)
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?)
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?)
I have self-reviewed this MR per code review guidelines.
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines)
I have followed the style guides.
This change is backwards compatible across updates, or this does not apply.

Availability and Testing

I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.)
I have tested this MR in all supported browsers, or it's not needed.
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

Label as security and @ mention @gitlab-com/gl-security/appsec
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
Security reports checked/validated by a reviewer from the AppSec team

Edited Aug 11, 2021 by Terri Chu

Use default project filter for issue/merge request project searches

What does this MR do?

Database timings

Merge Requests

Issues

Screenshots or Screencasts (strongly suggested)

How to setup and validate locally (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Merge request reports