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

[stable21] Prevent duplicate auth token activity updates #29380

Closed
wants to merge 1 commit into from

Conversation

backportbot-nextcloud[bot]
Copy link

backport of #29357

@blizzz blizzz mentioned this pull request Nov 3, 2021
10 tasks
The auth token activity logic works as follows
* Read auth token
* Compare last activity time stamp to current time
* Update auth token activity if it's older than x seconds

This works fine in isolation but with concurrency that means that
occasionally the same token is read simultaneously by two processes and
both of these processes will trigger an update of the same row.
Affectively the second update doesn't add much value. It might set the
time stamp to the exact same time stamp or one a few seconds later. But
the last activity is no precise science, we don't need this accuracy.

This patch changes the UPDATE query to include the expected value in a
comparison with the current data. This results in an affected row when
the data in the DB still has an old time stamp, but won't affect a row
if the time stamp is (nearly) up to date.

This is a micro optimization and will possibly not show any significant
performance improvement. Yet in setups with a DB cluster it means that
the write node has to send fewer changes to the read nodes due to the
lower number of actual changes.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst added the 4. to release Ready to be released and/or waiting for tests to finish label Nov 3, 2021
@skjnldsv
Copy link
Member

skjnldsv commented Nov 4, 2021

Ci says no it seems, restarted

@ChristophWurst
Copy link
Member

Ci says no it seems, restarted

which job?

@blizzz
Copy link
Member

blizzz commented Nov 10, 2021

is green, but right before final → moved to 21.0.7

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Blocking since it would probably result in a regression, as well. See #29678

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.

Needs this follow up too: #29681

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

This PR should probably get modified to also feature this change
https://github.com/nextcloud/server/pull/29682/files

@blizzz blizzz removed this from the Nextcloud 21.0.7 milestone Nov 18, 2021
@blizzz blizzz added this to the Nextcloud 21.0.8 milestone Nov 18, 2021
@PVince81 PVince81 added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 19, 2021
This was referenced Jan 7, 2022
@skjnldsv skjnldsv mentioned this pull request Jan 20, 2022
9 tasks
@skjnldsv
Copy link
Member

What is the status? Otherwise closing?

@skjnldsv skjnldsv mentioned this pull request Jan 25, 2022
8 tasks
@blizzz
Copy link
Member

blizzz commented Feb 11, 2022

@ChristophWurst what's the state with it? Close?

@blizzz
Copy link
Member

blizzz commented Mar 21, 2022

closing

@blizzz blizzz closed this Mar 21, 2022
@skjnldsv skjnldsv deleted the backport/29357/stable21 branch March 14, 2024 07:44
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.

6 participants