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

[4.2] Smart Search: Prevent "#__finder_tokens full" error #36749

Merged
merged 15 commits into from
Jul 6, 2022

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jan 20, 2022

Summary of Changes

Smart Search has a long standing issue that the #__finder_tokens table can run full and abort the indexing process. That table is of type "MEMORY", which can run full and there is an option to reduce the number of tokens to store in that table, which unfortunately doesn't work in all cases. The MEMORY table is used since it is a lot faster than a normal InnoDB table and since this is just used for temporary storage, writing speed is the main factor here.
This change pushes the code of addTokenToDb to the tokenizeToDbShort() method and better counts the number of tokens written to DB. It also removes the option from the Smart Search configuration and instead reads from the database what it is capable of. This most likely isn't a 100% solution, since there are edge cases where this could still fail, but it should improve the situation greatly.
This removes the addTokenToDb method, but since this is a protected method in a class of a component, this would still be in accordance to our backwards compatibility promise. Last but not least, this is remade from the PR #35995.

Testing Instructions

  1. Get a very long text and insert it into an article.
  2. When saving, see it fail with the error message from the title.
  3. Apply this PR.
  4. Save again and see it pass this time.

@Hackwar Hackwar changed the title Finder: prevent "table full" message during indexing [4.1] Smart Search: Prevent "#__finder_tokens full" error Jan 20, 2022
@richard67
Copy link
Member

@Hackwar The title says "[4.1]" but the target branch is 4.0-dev. What's right now?

@Hackwar
Copy link
Member Author

Hackwar commented Jan 20, 2022

Damn, I was hoping nobody would notice. I accidentally based this PR on 4.0-dev instead of 4.1-dev and since the latest changes from 4.0-dev haven't been merged upwards, I tried to get around the bigger diff for now by selecting the 4.0-dev branch. I'll select the 4.1-dev branch.

@Hackwar Hackwar changed the base branch from 4.0-dev to 4.1-dev January 20, 2022 08:23
@richard67
Copy link
Member

"I was hoping nobody would notice" does not work with me 😛

@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Feb 3, 2022
@Fedik
Copy link
Member

Fedik commented Feb 20, 2022

It partially works :)

It works for 100 paragraphs https://loremipsum.io/generator/?n=100&t=p
But fail for 500 paragraphs https://loremipsum.io/generator/?n=500&t=p
(Warning: do not try more, or your browser will explode 😄 )

Not sure if it success ;)

@Hackwar
Copy link
Member Author

Hackwar commented Apr 2, 2022

Lets call it an improvement, which is why I would still consider this a success.

@Fedik
Copy link
Member

Fedik commented Apr 3, 2022

I have tested this item ✅ successfully on fb77012


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36749.

@richard67
Copy link
Member

Shouldn't the parameter be removed from the params column in the extensions table in database for new installations here
https://github.com/joomla/joomla-cms/blob/4.1-dev/installation/sql/mysql/base.sql#L173
and here
https://github.com/joomla/joomla-cms/blob/4.1-dev/installation/sql/postgresql/base.sql#L179
?

@brianteeman What do you think? Should we do that here or leave it to a general cleanup for your issue #37386 ?

@richard67
Copy link
Member

richard67 commented Apr 3, 2022

P.S. to my previous comment: @Hackwar If you want to fix the SQL with this PR, too, the best is that you just save the parameters after having made a new, clean installation with this PR applied, and then copy the value from database into the insert statement in base.sql.

@superknutsel
Copy link

I have tested this item 🔴 unsuccessfully on fb77012

Saving an article with 23000 words gives me the message that the table is full. But after aplying the patch it still gives me the message that the table is full.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36749.

@brianteeman
Copy link
Contributor

@richard67 always do it when you see it. Another pr may not happen

@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:06
@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@Hackwar Hackwar changed the title [4.1] Smart Search: Prevent "#__finder_tokens full" error [4.2] Smart Search: Prevent "#__finder_tokens full" error Jul 4, 2022
@Quy Quy removed the PR-4.1-dev label Jul 4, 2022
@Hackwar
Copy link
Member Author

Hackwar commented Jul 4, 2022

Ok, I fixed a few more issues. @superknutsel , @Fedik would you be willing to test this again? I think I finally got this issue solved for good. Would be nice if we could get this into 4.2 still.

@Fedik
Copy link
Member

Fedik commented Jul 4, 2022

I have tested this item ✅ successfully on 1b625a3


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36749.

@Fedik
Copy link
Member

Fedik commented Jul 4, 2022

Still works for me, tried with ~33k words

@Hackwar
Copy link
Member Author

Hackwar commented Jul 4, 2022

I tried it with "War and Peace" from Tolstoi and while it failed to store the whole text, (because the input was too large) about a quarter of it was saved and then properly indexed.

@superknutsel
Copy link

I have tested this item ✅ successfully on 1b625a3

Hi, this time I tested it successfully.
Thanks for this.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36749.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36749.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 5, 2022
@HLeithner
Copy link
Member

Not sure what this PR fixes because for me "War and Peace" with 597.373 words worked before and afterwards but at least we have an option less YaY.

PS: tinymce is not optimized for writing books^^

@heelc29
Copy link
Contributor

heelc29 commented Jul 10, 2022

@HLeithner Can you please add the milestone Joomla! 4.2.0?

@richard67 richard67 added this to the Joomla! 4.2.0 milestone Jul 10, 2022
Kostelano added a commit to JPathRu/localisation that referenced this pull request Jul 19, 2022
joomla/joomla-cms#36749 +
joomla/joomla-cms#38228 +
joomla/joomla-cms#38227 - (только для en-GB, у нас исправлено ранее)
joomla/joomla-cms#38255 +
joomla/joomla-cms#38244 +
joomla/joomla-cms#38271 - (только для en-GB, у нас исправлено ранее)
joomla/joomla-cms#38287 - (только для en-GB, у нас исправлено ранее)
joomla/joomla-cms#38301 - (только для en-GB, у нас исправлено ранее)
@Hackwar Hackwar deleted the 4.2-finder-memorytable branch September 7, 2022 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants