Skip to content

Fix long transactions blocking _feed table#5574

Closed
tqcq wants to merge 2 commits intoFreshRSS:edgefrom
tqcq:edge
Closed

Fix long transactions blocking _feed table#5574
tqcq wants to merge 2 commits intoFreshRSS:edgefrom
tqcq:edge

Conversation

@tqcq
Copy link
Copy Markdown

@tqcq tqcq commented Aug 9, 2023

Changes proposed in this pull request:

  • The long transaction with more than 3 second is submitted automatically.
  • When updating a large amount of RSS, users can use it normally.

How to test the feature manually:

  1. Add a huge RSS or a time-consuming extension (HOOK: entry_before_insert, xExtension-Readable).
  2. Delete all articles and then reload.
  3. Click the management subscription button, the page will not be stuck.

Pull request checklist:

  • clear commit messages
  • code manually tested
    ~~ - [ ] unit tests written (optional if too hard)~~
    ~~ - [ ] documentation updated~~

@tqcq tqcq mentioned this pull request Aug 9, 2023
4 tasks
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Aug 9, 2023

Hello, Could you please explain the problem you observe more precisely? This is with MySQL only, right? (Tests with SQLite and PostgreSQL welcome). And what is blocked during the process more precisely (e.g. a specific table)? And with what consequence (e.g. that specific user cannot access FreshRSS)? I believe your suggested approach might create other problems, but we can try to find another way to fix. In particular, we have an intermediate entrytmp table (which is OK to block) before updating the entry table (for which the transaction should be fast).

When I enable the full-text crawling plugin, if the number of my articles reaches thousands, this transaction will last for hours, causing the _feed table to be locked.

I don't know about other databases and whether they lock after opening a transaction.

Can you post of the link to the extension you are using?

I cannot accept the approach of this PR as it risks breaking other things, such as the detection of gone articles, count of unread articles, etc. (However, you can put this code in the extension itself, or in another extension, if you accept the drawbacks)

It would be interesting to test whether the lock comes from articles that have been updated, as opposed to new articles.
The logic about updated articles has been improved a bit in the edge branch.

We should make sure that our transaction do not block the other operations. See https://dev.mysql.com/doc/refman/8.1/en/innodb-transaction-isolation-levels.html
In other words, we need a better investigation of what is blocking what more precisely. And also to know whether it is MySQL-specific.

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Aug 9, 2023
The detection of modified articles was wrong for feeds using loadCompleteContent. Indeed, the hash is supposed to computed on the content provided by the server of the RSS feed, excluding further modifications.
Furthermore, read hash from database instead of recomputing it all the time.
Slightly related to FreshRSS#5574
@tqcq
Copy link
Copy Markdown
Author

tqcq commented Aug 10, 2023

Hello, Could you please explain the problem you observe more precisely? This is with MySQL only, right? (Tests with SQLite and PostgreSQL welcome). And what is blocked during the process more precisely (e.g. a specific table)? And with what consequence (e.g. that specific user cannot access FreshRSS)? I believe your suggested approach might create other problems, but we can try to find another way to fix. In particular, we have an intermediate entrytmp table (which is OK to block) before updating the entry table (for which the transaction should be fast).

When I enable the full-text crawling plugin, if the number of my articles reaches thousands, this transaction will last for hours, causing the _feed table to be locked.
I don't know about other databases and whether they lock after opening a transaction.

Can you post of the link to the extension you are using?

I cannot accept the approach of this PR as it risks breaking other things, such as the detection of gone articles, count of unread articles, etc. (However, you can put this code in the extension itself, or in another extension, if you accept the drawbacks)

It would be interesting to test whether the lock comes from articles that have been updated, as opposed to new articles. The logic about updated articles has been improved a bit in the edge branch.

We should make sure that our transaction do not block the other operations. See https://dev.mysql.com/doc/refman/8.1/en/innodb-transaction-isolation-levels.html In other words, we need a better investigation of what is blocking what more precisely. And also to know whether it is MySQL-specific.

When the transaction is started, the <user_name>_feed table will be blocked, and any related modification and update operations will be blocked. I used the xExtension-Readable plugin here, for getting the full text.

According to the logic of the feedController, the entry_before_insert hook will be executed whether it is updating an article or getting a new article, so both operations will be affected.

Both of the following may be time-consuming operations, making the transaction slow.

$entry = Minz_ExtensionManager::callHook('entry_before_insert', $entry);

$entry = Minz_ExtensionManager::callHook('entry_before_insert', $entry);

Alkarex added a commit that referenced this pull request Aug 13, 2023
* Fix hash of articles with loadCompleteContent
The detection of modified articles was wrong for feeds using loadCompleteContent. Indeed, the hash is supposed to computed on the content provided by the server of the RSS feed, excluding further modifications.
Furthermore, read hash from database instead of recomputing it all the time.
Slightly related to #5574

* Explicit SQL alias

* PHPDocs
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Aug 14, 2023

I am running a test at the moment with PostgreSQL on a use-case resembling yours, with a refresh process that takes several hours due to many entries calling entry_before_insert, and all is good, i.e. the rest of the interface is completely usable and not blocked.

I have also checked the code from https://github.com/printfuck/xExtension-Readable can cannot see anything significantly different than my own teste.

So what remains to be done is to compare with MySQL / MariaDB and SQLite.

@tqcq
Copy link
Copy Markdown
Author

tqcq commented Aug 19, 2023

I am running a test at the moment with PostgreSQL on a use-case resembling yours, with a refresh process that takes several hours due to many entries calling entry_before_insert, and all is good, i.e. the rest of the interface is completely usable and not blocked.

I have also checked the code from https://github.com/printfuck/xExtension-Readable can cannot see anything significantly different than my own teste.

So what remains to be done is to compare with MySQL / MariaDB and SQLite.

Thanks for your test, I test in sqlite, MySQL, PostgreSQL, where sqlite and MySQL block other operations like Toggle favorite or Toggle read. And PostgreSQL is fine.

If it is convenient, please help to test sqlite and MySQL.

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Sep 1, 2023
Remove `updateTTL` function used to help migration to 5+ year-old FreshRSS 1.10 and FreshRSS 0.7.3 FreshRSS#1750
This function contributed to locking the database FreshRSS#5574
Subset of FreshRSS#3558
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Sep 1, 2023

@tqcq I had a quick test with SQLite already and identified one path, which was blocking the database, fixed with #5625
Feedback welcome, if you can give it a try. I has not yet debugged with MySQL.

Alkarex added a commit that referenced this pull request Sep 6, 2023
Remove `updateTTL` function used to help migration to 5+ year-old FreshRSS 1.10 and FreshRSS 0.7.3 #1750
This function contributed to locking the database #5574
Subset of #3558
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Sep 6, 2023

@tqcq I have tested this evening with MySQL after the patch #5625 and I cannot reproduce any lock anymore. Additional tests welcome

@Alkarex Alkarex closed this Sep 6, 2023
@Alkarex Alkarex added this to the 1.22.0 milestone Sep 6, 2023
@Alkarex
Copy link
Copy Markdown
Member

Alkarex commented Sep 10, 2023

#5648

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