Skip to content

Conversation

provokateurin
Copy link
Member

Before every authenticated request caused a query (SELECT * FROM "oc_notifications_settings" WHERE "user_id" = :dcValue1; {"dcValue1":"admin"}) which was largely unnecessary, stemming from the PostLoginListener.

Now the default settings are only initialized when the settings are requested and don't exist already.
To make sure the next send time is set for new users, the default settings are also initialized when a new user is created.
A tiny additional improvement is that the user values are not set now if the user doesn't change them, so if the app default is updated the user default will be updated as well (and it saves the additional queries to insert the rows into the table).

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Good for me code wise, I do not know about the behavior change regarding defaults, waiting for @nickvergessen comment.

@provokateurin provokateurin force-pushed the perf/default-settings branch 2 times, most recently from aa4948a to 680927d Compare May 20, 2025 09:07
@nickvergessen
Copy link
Member

Will check later this week or first week of June

@nickvergessen
Copy link
Member

Rebased on master to have a fresh footprint

@nickvergessen
Copy link
Member

nickvergessen commented Jun 23, 2025

The integration test query count is a bit rigged as all requests don't use cookies and therefore authenticate again everytime. So I checked PMM and there were 562k

SELECT * FROM `oc_notifications_settings` WHERE `user_id` = ?

queries last week.
That is much more calls to the login code than I ever expected.

@provokateurin
Copy link
Member Author

That is much more calls to the login code than I ever expected.

So this change makes sense? I'm not sure I understand what you are trying to imply.

@nickvergessen
Copy link
Member

So this change makes sense?

Not sure, I'd prefer to keep it on login, so that users that get their entry dropped recover automatically by some login.

Otherwise we'd need to remove the if ($user->isEnabled()) { check on the background job and that would mean the background job runs millions of queries on some instances.
We could also instead write a user preference whether it's populated, but that could be out of sync with the table then as well.

The main problem is that without this entry, users will not be considered anymore to receive notification reminder emails, which would be rather critical. But we have a listener for creation and it's recovering when opening the settings.
It still gives me a non-satisfying feeling, but as far as I remember, I just wanted to avoid a migration that does a user per query on update. So "should be fine", if you extract the other change from lib/Settings/Personal.php:

This is something else, it needs alignment in BeforeTemplateRenderedListener and the headline of the admin settings needs adjusting too as it's no longer about new users only.

Configure the default notification settings for new users

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

As per above

@nickvergessen nickvergessen force-pushed the perf/default-settings branch from c98cb9f to 41ebb56 Compare August 28, 2025 09:36
@nickvergessen
Copy link
Member

Before merging we need to check the query count of the performance test, in case it changed more than just a bit

Signed-off-by: Joas Schilling <[email protected]>
@nickvergessen nickvergessen merged commit 99b6c34 into master Aug 28, 2025
50 checks passed
@nickvergessen nickvergessen deleted the perf/default-settings branch August 28, 2025 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf: Only apply default settings when user is created or settings are requested
3 participants