FIX slowness when adding user to workspace #1205

hexaltation · 2024-09-11T15:47:51Z

Context

On French administration we can experience instances not beeing able to add users to workspaces due to long queries, and never ending queries.

Proposed solution

e26673e

To fix this issue we propose the same strategy than in #824.
Splitting the query in to simpler queries to gather informations about workspace and organization.

bd43d12
We also propose to remove the promisyfyAll around iteration of getUsers queries.
During local test, it seems to be the main culprit.
The ForEach iteration launches asynchonously many queries recycling the same transaction. Sometime the transaction never ended.

3c6388c
Cause the newly added function getExistingUsersByLogin uses a WHERE on logins.email a INDEX is added to allow fastest queries.

Related issues

FIXES #1005

Has this been tested?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

app/gen-server/entity/Login.ts

app/gen-server/lib/homedb/UsersManager.ts

fflorent

Hmm, there seem to exist tests that don't pass and that may be related to your changes:
https://github.com/gristlabs/grist-core/actions/runs/11103855934/job/30846705667?pr=1205#step:17:2848

Can you fix it? Or provide feedback and tell us whether these are just instabilities? (in such case, it would be more convenient to have tests that pass in the CI)

berhalak · 2024-10-07T11:14:48Z

Hmm, there seem to exist tests that don't pass and that may be related to your changes: https://github.com/gristlabs/grist-core/actions/runs/11103855934/job/30846705667?pr=1205#step:17:2848

Can you fix it? Or provide feedback and tell us whether these are just instabilities? (in such case, it would be more convenient to have tests that pass in the CI)

Can you rebase and try again. I can see other PRs are passing now.

berhalak

Migration looks good. But have you checked those build errors, it looks connected to those changes.

berhalak · 2024-10-15T07:39:04Z

@hexaltation I see some access tests are failing, I think this is caused by this PR, but if it isn't or you have some problems in fixing them, let me know.

hexaltation · 2024-10-15T08:19:55Z

@hexaltation I see some access tests are failing, I think this is caused by this PR, but if it isn't or you have some problems in fixing them, let me know.

Hello @berhalak,

I'm trying to fixing it.
Corrections made last week only fix half of failing tests.
I hope to have made all corrections this afternoon.

hexaltation · 2024-10-16T08:38:41Z

Hello @berhalak,

All tests are now passing. :)

berhalak · 2024-10-23T12:21:59Z

Hello @berhalak,

All tests are now passing. :)

Hi @hexaltation,

Yes, all looks good. Consider this accepted, we just need to carefully schedule merging this one as it contains migration.

paulfitz

Content looks ok! Thanks for dealing with this @hexaltation. The migration is pretty harmless, but it would still be good to exercise upgrades and downgrades by updating:

grist-core/test/gen-server/migrations.ts

Lines 53 to 59 in c783a4f

    
           const migrations = [Initial, Login, PinDocs, UserPicture, DisplayEmail, DisplayEmailNonNull, 
        
                               Indexes, Billing, Aliases, TeamMembers, FirstLogin, FirstTimeUser, 
        
                               CustomerIndex, ExtraIndexes, OrgHost, DocRemovedAt, Prefs, 
        
                               ExternalBilling, DocOptions, Secret, UserOptions, GracePeriodStart, 
        
                               DocumentUsage, Activations, UserConnectId, UserUUID, UserUniqueRefUUID, 
        
                               Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures, 
        
                               UserLastConnection, ActivationEnabled, Configs];

It would also be important to rebase against latest main so existing migrations are up to date, and to make the timestamp in the name of the migration file be chronologically after any existing migration if it happens to come before another.

hexaltation · 2024-10-24T07:51:13Z

It would also be important to rebase against latest main so existing migrations are up to date, and to make the timestamp in the name of the migration file be chronologically after any existing migration if it happens to come before another.

Thanks @paulfitz for your review
Rebase has been done and the migration moved at last position.
And the tests are passing with the migration in it. :)

berhalak

Looks good, thank you.

paulfitz · 2024-10-24T16:47:48Z

app/gen-server/lib/homedb/UsersManager.ts

+      .select('user')
+      .from(User, 'user')
+      .leftJoinAndSelect('user.logins', 'logins')
+      .where('email IN (:...emails)', {emails: normalizedEmails});


@hexaltation is it possible for the list of emails to be empty? We're seeing a failure when running against postgres. Not sure yet if this is the code causing it, might not be, but the error message is of the form you can get with an empty in list:

zing=> SELECT 1 WHERE 1 IN (); ERROR: syntax error at or near ")"

Hello @paulfitz
As far as I understand the code emails come from a delta in users lists of :

orgs

workspaces

documents

billing access

As it a delta, I think that initiating update of one of this four against an empty delta should not occur.

May be we have to fix the following API routes to gently fail with bad request on empty deltas ?

PATCH '/api/orgs/:oid/access'

PATCH '/api/workspaces/:wid/access'

PATCH '/api/docs/:did/access'

or in doom.ts for updating Billing Accounts.

What do you think about that ?

Hello again @paulfitz!

After discussion with @hexaltation, we are working on something we hope that it would fix the issue.

Especially we found a case where delta.users is an empty object.

We'll keep you inform about our progress or if we still have questions.

Thanks @fflorent, @hexaltation ! I think reasonable behavior to mimic here is SQLite's, where IN () simply matches nothing (and is not a syntax error). This could be done in the code by skipping the query if the where clause will be trying to filter for the the empty set of emails.

hexaltation · 2024-09-19T13:36:15Z

app/gen-server/lib/homedb/UsersManager.ts

-      const emailUsers = await Promise.all(
-        emails.map(async email => await this.getUserByLogin(email, {manager: transaction}))
-      );
+      const existingUsers = await this.getExistingUsersByLogin(emails, transaction);


This have the purpose to replace the await Promise.all() around a map in previous implementation.
It seems that there were some race condition between the getUserByLogin callBacks sharing the same transaction locked the DB from time to time.
The new implementation hopes to avoid this concurrency inside the transaction.

app/gen-server/entity/Login.ts

hexaltation · 2024-10-25T06:31:49Z

app/gen-server/lib/homedb/UsersManager.ts

+      .select('user')
+      .from(User, 'user')
+      .leftJoinAndSelect('user.logins', 'logins')
+      .where('email IN (:...emails)', {emails: normalizedEmails});


Hello @paulfitz
As far as I understand the code emails come from a delta in users lists of :

orgs

workspaces

documents

billing access

As it a delta, I think that initiating update of one of this four against an empty delta should not occur.

May be we have to fix the following API routes to gently fail with bad request on empty deltas ?

PATCH '/api/orgs/:oid/access'

PATCH '/api/workspaces/:wid/access'

PATCH '/api/docs/:did/access'

or in doom.ts for updating Billing Accounts.

What do you think about that ?

hexaltation · 2024-10-28T16:24:07Z

Hello @paulfitz,
A patch is submitted in #1279

hexaltation requested a review from fflorent September 11, 2024 15:48

hexaltation changed the title ~~Issue 1005 slow add user to workspace~~ FIX slowness when adding user to workspace Sep 11, 2024

berhalak self-assigned this Sep 11, 2024

berhalak self-requested a review September 18, 2024 19:38

berhalak removed their assignment Sep 18, 2024

berhalak requested changes Sep 23, 2024

View reviewed changes

app/gen-server/entity/Login.ts Show resolved Hide resolved

app/gen-server/lib/homedb/UsersManager.ts Show resolved Hide resolved

fflorent suggested changes Sep 23, 2024

View reviewed changes

hexaltation force-pushed the issue-1005-slow-add-user-to-workspace branch from 9f8f09d to 1d4498f Compare September 24, 2024 10:27

berhalak self-assigned this Sep 25, 2024

fflorent approved these changes Sep 27, 2024

View reviewed changes

fflorent self-requested a review September 27, 2024 12:55

hexaltation requested a review from berhalak September 30, 2024 10:03

fflorent approved these changes Sep 30, 2024

View reviewed changes

fflorent suggested changes Sep 30, 2024

View reviewed changes

hexaltation force-pushed the issue-1005-slow-add-user-to-workspace branch from 2458ae5 to 286bf75 Compare October 10, 2024 10:24

berhalak requested changes Oct 10, 2024

View reviewed changes

hexaltation force-pushed the issue-1005-slow-add-user-to-workspace branch from 85fca2d to 2279bd9 Compare October 15, 2024 17:47

berhalak self-requested a review October 16, 2024 11:47

georgegevoian self-requested a review October 23, 2024 14:51

paulfitz reviewed Oct 23, 2024

View reviewed changes

hexaltation added 5 commits October 24, 2024 09:17

fix: splitQuery

a83c0ee

fix: lower number of queries in email delta check

4b06bf9

fix: add missing index on Logins.email column

9fbdafd

fix: fixes after @berhalak and @fflorent reviews

06ff057

chore: ADD migration for DB new Index logins__email

013e94e

hexaltation added 4 commits October 24, 2024 09:17

fix: getting orgs without verifying scope

1a89799

fix: Ensure emailUsers sync with delta.users using a map

3278560

fix: checking normalized emails rather than user input

f63bb15

chore: ADD LoginsEmailsIndex migration to tests

12cfb98

hexaltation force-pushed the issue-1005-slow-add-user-to-workspace branch from 592b4a6 to 12cfb98 Compare October 24, 2024 07:35

hexaltation requested review from fflorent and paulfitz October 24, 2024 07:49

fflorent approved these changes Oct 24, 2024

View reviewed changes

berhalak approved these changes Oct 24, 2024

View reviewed changes

berhalak merged commit c945fac into gristlabs:main Oct 24, 2024
11 checks passed

paulfitz reviewed Oct 24, 2024

View reviewed changes

hexaltation commented Oct 28, 2024

View reviewed changes

fflorent mentioned this pull request Oct 28, 2024

CI: add server tests with Postgresql #1278

Closed

4 tasks

hexaltation mentioned this pull request Oct 28, 2024

FIX: TYPEORM postgresql (...Emails) syntax bad handling in #1205 #1279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX slowness when adding user to workspace #1205

FIX slowness when adding user to workspace #1205

hexaltation commented Sep 11, 2024

fflorent left a comment

berhalak commented Oct 7, 2024

berhalak left a comment

berhalak commented Oct 15, 2024

hexaltation commented Oct 15, 2024

hexaltation commented Oct 16, 2024

berhalak commented Oct 23, 2024

paulfitz left a comment

hexaltation commented Oct 24, 2024

berhalak left a comment

paulfitz Oct 24, 2024

hexaltation Oct 25, 2024

fflorent Oct 28, 2024 •

edited

Loading

paulfitz Oct 28, 2024

hexaltation Sep 19, 2024

hexaltation Oct 25, 2024

hexaltation commented Oct 28, 2024

	const migrations = [Initial, Login, PinDocs, UserPicture, DisplayEmail, DisplayEmailNonNull,
	Indexes, Billing, Aliases, TeamMembers, FirstLogin, FirstTimeUser,
	CustomerIndex, ExtraIndexes, OrgHost, DocRemovedAt, Prefs,
	ExternalBilling, DocOptions, Secret, UserOptions, GracePeriodStart,
	DocumentUsage, Activations, UserConnectId, UserUUID, UserUniqueRefUUID,
	Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures,
	UserLastConnection, ActivationEnabled, Configs];

FIX slowness when adding user to workspace #1205

FIX slowness when adding user to workspace #1205

Conversation

hexaltation commented Sep 11, 2024

Context

Proposed solution

Related issues

Has this been tested?

fflorent left a comment

Choose a reason for hiding this comment

berhalak commented Oct 7, 2024

berhalak left a comment

Choose a reason for hiding this comment

berhalak commented Oct 15, 2024

hexaltation commented Oct 15, 2024

hexaltation commented Oct 16, 2024

berhalak commented Oct 23, 2024

paulfitz left a comment

Choose a reason for hiding this comment

hexaltation commented Oct 24, 2024

berhalak left a comment

Choose a reason for hiding this comment

paulfitz Oct 24, 2024

Choose a reason for hiding this comment

hexaltation Oct 25, 2024

Choose a reason for hiding this comment

fflorent Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

paulfitz Oct 28, 2024

Choose a reason for hiding this comment

hexaltation Sep 19, 2024

Choose a reason for hiding this comment

hexaltation Oct 25, 2024

Choose a reason for hiding this comment

hexaltation commented Oct 28, 2024

fflorent Oct 28, 2024 •

edited

Loading