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: Allow covering relation queries with minimal index #6581

Merged
merged 2 commits into from
Apr 8, 2020

Conversation

noahsilas
Copy link
Contributor

@noahsilas noahsilas commented Apr 8, 2020

When finding objects through a relation, we're sending Mongo queries that look like this:

db.getCollection('_Join:foo:bar').find({ relatedId: { $in: [...] } });

From the result of that query, we're only reading the owningId field, so we can start by adding it as a projection:

db.getCollection('_Join:foo:bar')
  .find({ relatedId: { $in: [...] } })
  .project({ owningId: 1 });

This seems like the perfect example of a query that could be satisfied with an index scan: we are querying on one field, and only need one field from the matching document.

For example, this can allow users to speed up the fetching of user roles in authentication, because they query a roles relation on the _Role collection. To add a covering index on that, you could now add an index like the following:

db.getCollection('_Join:roles:_Role').createIndex(
  { relatedId: 1, owningId: 1 },
  { background: true }
);

One caveat there is that the index I propose above doesn't include the _id column. For the query in question, we don't actually care about the ID of the row in the join table, just the owningId field, so we can avoid some overhead of putting the _id column into the index if we can also drop it from the projection. This requires adding a small special case to the MongoStorageAdapter, because the _id field is special: you have to opt-out of using it by projecting { _id: 0 }.

It appears that there is also now an automatic commit hook that applies some linting and styling rules, so I also broke this PR up into two commits: one that just applies those automatic fixes, and one that then makes the actual change. Hopefully this makes. it easier to review the diff!

My actual changes were quite difficult to find when buried in this sea
of style changes, which were getting automatically applied during a
pre-commit hook. Here I just run the hooks against the files I'm going
to be touching in the following commit, so that a reviewer can ignore
these automatically generated diffs and just view the meaningful commit.
When finding objects through a relation, we're sending Mongo queries
that look like this:
```
db.getCollection('_Join:foo:bar').find({ relatedId: { $in: [...] } });
```

From the result of that query, we're only reading the `owningId` field,
so we can start by adding it as a projection:
```
db.getCollection('_Join:foo:bar')
  .find({ relatedId: { $in: [...] } })
  .project({ owningId: 1 });
```

This seems like the perfect example of a query that could be satisfied
with an index scan: we are querying on one field, and only need one
field from the matching document.

For example, this can allow users to speed up the fetching of user roles
in authentication, because they query a `roles` relation on the `_Role`
collection. To add a covering index on that, you could now add an index
like the following:
```
db.getCollection('_Join:roles:_Role').createIndex(
  { relatedId: 1, owningId: 1 },
  { background: true }
);
```

One caveat there is that the index I propose above doesn't include the
`_id` column. For the query in question, we don't actually care about
the ID of the row in the join table, just the `owningId` field, so we
can avoid some overhead of putting the `_id` column into the index if we
can also drop it from the projection. This requires adding a small
special case to the MongoStorageAdapter, because the `_id` field is
special: you have to opt-out of using it by projecting `{ _id: 0 }`.
@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #6581 into master will decrease coverage by 0.09%.
The diff coverage is 83.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6581      +/-   ##
==========================================
- Coverage   93.89%   93.80%   -0.10%     
==========================================
  Files         169      169              
  Lines       11968    11970       +2     
==========================================
- Hits        11237    11228       -9     
- Misses        731      742      +11     
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 91.29% <72.72%> (-2.21%) ⬇️
src/Controllers/DatabaseController.js 95.03% <100.00%> (-0.17%) ⬇️
src/batch.js 89.79% <0.00%> (-2.05%) ⬇️
src/ParseServerRESTController.js 96.36% <0.00%> (-1.82%) ⬇️
src/Adapters/Storage/Mongo/MongoTransform.js 88.67% <0.00%> (-0.15%) ⬇️
src/RestWrite.js 93.81% <0.00%> (+0.32%) ⬆️
src/Adapters/Auth/httpsRequest.js 100.00% <0.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b791e13...725019b. Read the comment docs.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

LGTM!

@davimacedo davimacedo merged commit 19086a8 into parse-community:master Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants