Optimize sql query ws acl (version 2) #824

fflorent · 2024-01-11T12:08:38Z

Context

In our self-hosted instance, when we fetch the ACLs rules of a workspace, we get a gateway timeout error. This is due to the fact that with this workspace, the SQL request made in getWorkspaceAccess returns ~900 000 results, which not only is heavy for the database but also takes time for TypeORM to parse.

@vviers made an analysis here in this document: https://outline.incubateur.anct.gouv.fr/doc/grist-list-workspace-access-issue-rxer0X7e44

Proposed solution

Here is another proposal than the one in #818

@vviers also suggested to rework the function so it does two separate queries.

I changed my mind as it seemed like:

The risk for regressions of the patch in Optimize a bit the SQL request to fetch workspace access #818 also seem high (and maybe higher);
There are benefits for this rework, as a much better performance and a better clarity (2 queries for two types of resources requested);
I would say we gain control over the logic, hence we lower the risk for regressions (especially in the future).

I hope this goes in the right way.

Without this optimization, we fetched loads of entries from the database, which led to database and nodejs overloads. We could go further, this is a modest patch towards better performance.

fflorent · 2024-01-11T12:12:29Z

app/gen-server/lib/HomeDBManager.ts

+    const orgQuery = this.org(scope, org, {
+      markPermissions: Permissions.VIEW,
+      needRealOrg: true,
+      allowSpecialPermit: true


I still have doubts on this part: we not only control the permissions for the groupes associated with the workspaces, but also now the permissions for the orgs.

Otherwise, I may remove the call to verifyIsPermitted and these options when we call that method for the workspaces, if that makes sense.

That sounds right. Makes sense to me to exclude them when called from that method.

I ended up renaming verifyIsPermitted to verifyEntity to allow discarding the permission check.

f8bed0b

I felt like we may have wanted dealing with monad in that function (like Result in Rust), or throw exceptions that would be handled in the express route that would deduce the status code to return.

We use two queries: one fetches the workspaces, the second the organization that the workspace belongs to.

vviers · 2024-01-17T13:54:38Z

@paulfitz it seems that tests are passing here, do we need to test some more (e.g. in staging) or can we merge ? This would be a great relief to us if this was fixed :)

paulfitz · 2024-01-18T13:58:12Z

@paulfitz it seems that tests are passing here, do we need to test some more (e.g. in staging) or can we merge ? This would be a great relief to us if this was fixed :)

Hi @vviers, sorry for the delay. Previous version of this PR did pass all our own internal tests. Will get going on reviewing new version as soon as someone's available, I understand it is a priority for you.

app/gen-server/lib/HomeDBManager.ts

georgegevoian · 2024-01-29T07:02:30Z

app/gen-server/lib/HomeDBManager.ts

+    const orgQuery = this.org(scope, org, {
+      markPermissions: Permissions.VIEW,
+      needRealOrg: true,
+      allowSpecialPermit: true


That sounds right. Makes sense to me to exclude them when called from that method.

georgegevoian · 2024-01-29T17:33:00Z

Thanks @fflorent. Latest changes are passing our internal test suites.

Just waiting for a second reviewer now - I left a nudge in our Slack.

paulfitz · 2024-01-29T19:54:39Z

app/gen-server/lib/HomeDBManager.ts

+
+    // Also fetch the organization ACLs so we can determine inherited rights.
+    const orgQuery = this._buildOrgWithACLRulesQuery(scope, workspace.org.id);
+    const orgQueryResult = await verifyEntity(orgQuery, { discardPermissionsCheck: true });


Do you have any concerns about changes to the database that may have happened between this query and the previous one?

To be sure, by "the previous one", do you mean:

previous in the code? (which would refer to the workspace ACL selection)

previous in the commits? (which would refer to the org ACL selection with permission check)

Previous in the code.

To be clearer, I'm asking about this sequence:

Query 1 happens

Orgs/workspaces/documents/users... are created/updated/deleted

Query 2 happens

Right. I am no DB expert, my guess is that the drawback of this change is the lack of isolation.

To limit that drawback, I guess we might otherwise gather these two queries into one using UNION, though I don't know how much work it would represent.

I am scratching a bit my head tomorrow, I can challenge a bit more myself about it, or ask the opinions of colleagues.

I am still a bit divided, I would say that if there are some resources modified in between, as the function is only used for the API (and client-side by the ACL popup), the user might be slightly misled but not much (they may see outdated rights in workspaces). If the organization is deleted in between, they should see a 404 error.

The important thing is consistency. Without the transaction, it is hard to reason about what the call could return, because a lot could happen in between the queries. With the transaction, it is easier to think about what it will do. Whether the result is more or less stale is not that important, to my mind, because no matter what you do, by the time the caller receives the reply there's no longer any guarantee it is still valid - anything could have happened in the meantime.

Thanks for your feedback. I am sorry, I am not sure to understand your remark, is it OK what I did or should I bring some change to my PR?

I think it looks ok @fflorent. Agree with @paulfitz that the isolation level isn't that important here, so ok to go with the default. Thanks.

Done! 690fdce

Thanks @fflorent !

app/gen-server/lib/HomeDBManager.ts

paulfitz

Appreciate your work and patience on this @fflorent

Optimize a bit the SQL request to fetch workspace access

26db93b

Without this optimization, we fetched loads of entries from the database, which led to database and nodejs overloads. We could go further, this is a modest patch towards better performance.

fflorent commented Jan 11, 2024

View reviewed changes

Florent FAYOLLE added 2 commits January 11, 2024 13:15

Optimize the query for workspace ACL

d9fe7c3

We use two queries: one fetches the workspaces, the second the organization that the workspace belongs to.

Tests: Add server logs DEBUG is set to some truthy value

b7d0c64

fflorent force-pushed the optimize-sql-query-ws-acl-2 branch from 9bbe80d to b7d0c64 Compare January 11, 2024 12:15

fflorent mentioned this pull request Jan 11, 2024

Optimize a bit the SQL request to fetch workspace access #818

Closed

fflorent requested a review from paulfitz January 11, 2024 13:46

CamilleLegeron added the gouv.fr label Jan 12, 2024

georgegevoian self-requested a review January 19, 2024 05:10

georgegevoian reviewed Jan 29, 2024

View reviewed changes

Florent FAYOLLE added 2 commits January 29, 2024 10:20

Fix typos

93d14a4

Discard permissions check for workspaces' parent org

f8bed0b

fflorent requested a review from georgegevoian January 29, 2024 09:46

paulfitz reviewed Jan 29, 2024

View reviewed changes

Florent FAYOLLE added 4 commits January 29, 2024 21:26

Rename discardPermissionCheck to skipPermissionCheck

2ee931e

Wrap queries into a transaction

3262f58

Use isolation level

10ddac5

Use default isolation level for transactions

690fdce

fflorent requested a review from paulfitz January 31, 2024 16:43

paulfitz approved these changes Jan 31, 2024

View reviewed changes

paulfitz merged commit 866ec66 into gristlabs:main Jan 31, 2024
11 of 13 checks passed

fflorent deleted the optimize-sql-query-ws-acl-2 branch February 1, 2024 08:59

fflorent mentioned this pull request May 29, 2024

Adding a user to a workspace can be very slow #1005

Closed

hexaltation mentioned this pull request Sep 11, 2024

FIX slowness when adding user to workspace #1205

Merged

4 tasks

vviers mentioned this pull request Oct 16, 2024

Gateway Timeout error when moving a document in a large team site #1271

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize sql query ws acl (version 2) #824

Optimize sql query ws acl (version 2) #824

fflorent commented Jan 11, 2024

fflorent Jan 11, 2024

georgegevoian Jan 29, 2024

fflorent Jan 29, 2024

vviers commented Jan 17, 2024

paulfitz commented Jan 18, 2024

georgegevoian Jan 29, 2024

georgegevoian commented Jan 29, 2024

paulfitz Jan 29, 2024

fflorent Jan 29, 2024

paulfitz Jan 29, 2024

paulfitz Jan 29, 2024

fflorent Jan 29, 2024

paulfitz Jan 30, 2024

fflorent Jan 30, 2024

georgegevoian Jan 31, 2024

fflorent Jan 31, 2024

paulfitz Jan 31, 2024

paulfitz left a comment

Optimize sql query ws acl (version 2) #824

Optimize sql query ws acl (version 2) #824

Conversation

fflorent commented Jan 11, 2024

Context

Proposed solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vviers commented Jan 17, 2024

paulfitz commented Jan 18, 2024

Choose a reason for hiding this comment

georgegevoian commented Jan 29, 2024

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

Choose a reason for hiding this comment

paulfitz left a comment

Choose a reason for hiding this comment