Don't store full extracted file content data in the database

hi,
it's in the database to be cached
when requesting a file extraction, we check if we don't have it already in the cache. if not, we run extraction (that can be CPU consuming ) and the cache it for late usage (example: when reindexing all content, we won't extract again all the files content as we already have them in cache)

probabely yes
i'm for merging any contribution that enhaces performance

a hook or event when something is indexed : actually this is search_api level : maybe hook_search_api_index_items_alter or hook_search_api_items_indexed

if you meant a hook or event when a file content is extracted : actually the alone hook search_api_attachments provides is to prevent a file from beeing extracted : that's hook_search_api_attachments_indexable

if none of these meet your need, feel free to suggest a new hook or event listener

hope this helps

Would it not make more sense to store a hash value rather than the ENTIRE content itself? I do not agree with keeping it as a "cache" for the purposes of "comparison". That is the wrong reason to be storing data. If you're looking for comparisons, hashes are the proper way to do that. When the hash of a file doesnt match, then you know it needs to be re-indexed. This is how any file comparison software works so I'm baffled why this would be any different.

Is there ANY other reason for the storing of the content? If there is, I'd like to hear it. If there is no other valid reason, then I think this is a great feature request (replace "cache" with a hash table).

EDIT: Nevermind, I just re-read the reply and the main reason is to avoid CPU usage when the indexes are cleared. It seems like that could be an added option similar to the " Preserve cached extractions across cache clears." option. Give users the option to store full data or instead store hashes. For some situations, the CPU savings you get would be minimal, especially for 100000s of small PDF files that would take up a lot of space in the DB layer, but take no time at all to re-extract. Something to ponder I guess.

If we are storing data in the DB, it may also be better to compress it rather than store it uncompressed. This could be used in conjunction with the hashes suggestion I made earlier. Throwing out ideas here as I think there is room for a lot of improvement with the current implementation.

I'm changing this to a critical bug report as it is in no way expected that the entire contents of indexed files will be stored in the database.

WIP to just use the KeyValue structure for tracking whether the file was processed, not the actual data. I haven't tested it yet though.

#11 tentatively works for me on a development site.

I agree that database usage of this is a problem, but the patch is just removing the cache alltogether? the point is to avoid doing the possibly time consuming extraction again when e.g. resaving content or reindexing everything when the file hasn't actually changed. That has to be stored somewhere, just a flag means nothing here.

Maybe the extracted content could be stored in files instead or so instead of the database.

Could you just hash the file and store that somewhere instead of the complete content?

Sorry, didn't read above comments.

Wouldn't moving the cached data from key_value to a custom table 'solve' the overal performance issue?
I have a case where I see the entire application slowing down because of the key_value table growing.

I don't think it is necessary to cover the case when the index is cleared. I think it is normal to extract the documents again in that case. What might make sense it to avoid re-extracting the documents when a node is updated but the attachments aren't. In that case it would be ideal to get the existing data from solr and send that again as the update.

I don't see why a custom table would be any different from key value, why should that slow down your site more than a custom table? There should be indexes on that table to make lookups fast, unless maybe if you do a lot of writes there yourself?

I think we should rename the title a bit to be more clear.

The patch is working fine for me.

Again, the patch as it is is not useful. Sure, it stops writing data to key value, but then key value is completely meaningless, there is no benefit in writing a boolean value there, it doesn't mean anything. You have to extract it every time anyway. The queue process is also entirely pointless then, if it doesn't do anything anymore, it just sets a TRUE flag, always.

If you'd rather re-process the files every time content is reindexed (which can be a lot if you index documents through file fields on nodes for example), then a simple solution would be a setting that allows to disable the cache. But that will need quite a bit more work, like simply not bother to create queue items. Instead of just a true/false flag, a more improved version would put the key value stuff behind an interface, then you could have different implementations: key value, none, text files, and the setting would set which implementation is used.

then a simple solution would be a setting that allows to disable the cache

This would indeed be the first step.

In the meantime, here's a reroll

Hi,

As Berdir mentioned in #13, what about storing cached file extractions in files and running MD5 (or other hashing method) as a key?
e.g: private://search_api_attachments/[md5:1]/[md5:2]/[md5]

Where:
- [md5:1] is the 1st char of the MD5
- [md5:2]: is the 2nd char of the MD5
- [md5]: is the full MD5

The folders are there to prevent reaching the max amount of files in a single folder.
Then on hook_file_update() and hook_file_delete() we just delete the right MD5 file.
I did just that in a D7 site where we had enormous amount of files and this still works AFAIK.

This is a work in progress with the approach explained in #23.
I added a new "SearchApiAttachmentsCache" plugin type.
Is this on the right track? Any feedback is welcome.

IANAM (I am not a maintainer) but yes, that's what I had in mind. Not sure if it really needs to be a plugin, plugins are useful if you likely have multiple instances with their own configuration, in this case it might be enough to just have it as a service?

The downside is that making it (UI) configurable is a bit awkward then. Maybe a factory service that delegates to a configurable service name? A bit like \Drupal\Core\Cache\CacheFactory::get() but with a config setting instead of Settings::get() (and minus all the $bin complexity as we only have a single instance). Also don't need a factory factory pattern then.

Thanks for your input @Berdir.
It's true plugins feel a bit un-necessary here.

Couldn't we somehow leverage the cache API?
So that by default it uses a "search_api_attachments" cache bin, which would store in DB, table "cache_search_api_attachments" (or memcache, redis, etc if present).
Then somehow we could switch to file storage, maybe using \Drupal\Component\PhpStorage\FileStorage or similar?

I think the point is that it should survive cache clears, which at least the default cache backends would not. You could write a custom backend that ignores the deleteAll() and configure that by default, not sure if that really makes it much easier compared to a custom factory and a specific backend.

Again, not the maintainer, would be good to have some "official" feedback.

hi,
thanks all for the ideas and work on that
my thoughts on this :
- i'm not against adding file storage method
- i'd love to let user choose to keep storing in database or in files (depending on their needs and possibilities), so an option on that would be great
- there is an option to keep the extracted content live across cache clears for the database storage, we should also honor it for the files storage

hope this helps, and again thanks for the time and effort on this guys

Just wanted to add that this patch has a major performance hit on the performance of search results when you are using the search excerpt highlighter. This patch causes the file extraction to happen again for each search result while the file is actually indexed completely into SOLR. Making search views extremely slow.

Moved from plugin to factory as proposed by @Berdir. There was quite a lot of moving stuff around, so I am not sure if I left out something that was meant to be in this patch ...

1. +++ b/config/install/search_api_attachments.admin_config.yml
   @@ -1,5 +1,7 @@
    
   +caching_method: keyvalue
   +
   

   maybe "cache_backend" as the setting name?

2. +++ b/search_api_attachments.services.yml
   @@ -2,6 +2,17 @@ services:
   +  search_api_attachments.cache_factory:
   +    class: Drupal\search_api_attachments\Cache\AttachmentsCacheFactory
   +    arguments: ['@config.factory']
   +    calls:
   +      - [setContainer, ['@service_container']]
   

   If you also define a search_api_attachments.cache service that uses this as its factory then you can skip the extra get() when accessing it and it's instantiation is also automatically "cached" as a service.

   We don't need multiple instances of this, unlike key value store.

3. +++ b/src/Cache/AttachmentsCacheFactory.php
   @@ -0,0 +1,44 @@
   +/**
   + * Defines the queue factory.
   + */
   +class AttachmentsCacheFactory implements ContainerAwareInterface {
   +
   

   copied comment.

4. +++ b/src/Cache/AttachmentsCacheFactory.php
   @@ -0,0 +1,44 @@
   +   */
   +  public function get() {
   +    $service_name = $this->config->get('caching_method');
   +    return $this->container->get($service_name);
   +  }
   +
   

   this seems to be expect the full service name but you've just set it to 'keyvalue'. It needs to be "search_api_attachments.cache_keyvalue" instead.

5. +++ b/src/Cache/AttachmentsCacheInterface.php
   @@ -0,0 +1,46 @@
   +   *
   +   * @param \Drupal\file\Entity\File $file
   +   *   The file object.
   +   */
   +  public function clear(File $file);
   +
   

   should type hint on the interface.

6. +++ b/src/Cache/Files.php
   @@ -0,0 +1,93 @@
   +
   ...
   +    $md5_file = md5_file($realpath);
   +    $sub_folder1 = substr($md5_file, 0, 1);
   +    $sub_folder2 = substr($md5_file, 1, 1);
   +    return implode('/', [self::FOLDER, $sub_folder1, $sub_folder2, $md5_file]);
   

   md5_file() gives you the md5 hash of the file *contents*. But at this point, it's likely that file doesn't even exist yet.

   We should also not use md5 as it is guaranteed to spark discussion about which hash mechanism should be used.

   I'm uncertain if we really want to go with a hashed path structure. Maybe we should just use the original URI? It's pretty much guaranteed to be unique.

   in other words, just do static::FOLDER . '/' . \Drupal\Core\StreamWrapper\StreamWrapperManager::getTarget($uri).

   also makes it easy to debug if you want to look up what was extracted from a given file.

7. +++ b/src/Form/TextExtractorFormSettings.php
   @@ -97,6 +97,20 @@ class TextExtractorFormSettings extends ConfigFormBase {
   +      '#empty_value' => '',
   +      '#options' => [
   +        'search_api_attachments.cache_keyvalue' => $this->t('Keyvalue'),
   +        'search_api_attachments.cache_files' => $this->t('Files'),
   +      ],
   +      '#default_value' => $config->get('caching_method'),
   

   ah, here you have the right keys.

   This isn't extensible though, you'd have to form alter it to add your own.

   What if we combine it with a service tag to discover possible implementations?

   See theme.negotiator for an example, that is tagged with - { name: service_id_collector, tag: theme_negotiator }

   And then the implementations do - { name: theme_negotiator}.

   The factory service then receives a list of all tagged services as an argument. And you can add a getLabel() method to the interface and a getOptions() method to the factory, which you can call here.

8. +++ b/src/Plugin/search_api/processor/FilesExtractor.php
   @@ -293,11 +293,7 @@ class FilesExtractor extends ProcessorPluginBase implements PluginFormInterface
   -          $extracted_data = $this->limitBytes($extracted_data);
   -          $this->keyValue->get($collection)->set($key, $extracted_data);
   -          $this->moduleHandler->invokeAll(
   -            'search_api_attachments_content_extracted', [$file, $entity]
   -          );
   +          $cache_service->set($file, $extracted_data);
   

   this seems to remove the limitBytes and alter hook? bad reroll maybe?

9. +++ b/src/TextExtractorPluginBase.php
   @@ -168,4 +178,11 @@ abstract class TextExtractorPluginBase extends PluginBase implements TextExtract
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getCachingService() {
   +    return $this->cachingService;
   +  }
   +
   

   I don't see why access to this needs to go through the plugin, they don't use it?

I also want to highlight the issue I mentioned in #29 again. Processed attachments which are already in the database are extracted again, causing a huge performance hit.

Yes, well aware that the older patches caused this problem, that's what I've said before as well. See #13,#17,#20. The recent patches do not have this problem but they are also not yet ready for production usage.

We were using patch #20 :-)

#20 is not a patch, but up until #22 was the old approach that I commented is not right. Only since #24 we start to have feasible solution.

Thanks for the review.

1. changed
2. added service
3. fixed, thx
4. yeah, forgot to update admin_config when moving stuff around
5. used FileInterface. Extractors have the same problems, maybe follow up?
6. implemented
7. added tags and logic
8. did a rebase first yes
9. fixed

Testing wise I just tried saving the form and indexing.

Not a full review.

1. +++ b/src/Form/TextExtractorFormSettings.php
   @@ -147,9 +170,11 @@ class TextExtractorFormSettings extends ConfigFormBase {
        $config->set('extraction_method', $extractor_plugin_id);
   -    // Set the redad text files directly option.
   +    // Set the caching method.
   +    $config->set('cache_backend', $form_state->getValue('cache_backend'));
   +    // Set the read text files directly option.
        $config->set('read_text_files_directly', $form_state->getValue('read_text_files_directly'));
        // Set the preserving cache option.
   

   I'm wondering if we could clear the old backend before we switch if we detect a change. Otherwise switching from key value to files will require you to manually do that. That said, only doing it in the UI won't work for drush cset and config import, the correct way for that would be a config write subscriber.

2. +++ b/src/TextExtractorPluginBase.php
   @@ -53,7 +53,6 @@ abstract class TextExtractorPluginBase extends PluginBase implements TextExtract
      protected $messenger;
    
   -
      /**
       * {@inheritdoc}
       */
   @@ -70,7 +69,13 @@ abstract class TextExtractorPluginBase extends PluginBase implements TextExtract
   
   @@ -70,7 +69,13 @@ abstract class TextExtractorPluginBase extends PluginBase implements TextExtract
       */
      public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
        return new static(
   -        $configuration, $plugin_id, $plugin_definition, $container->get('config.factory'), $container->get('stream_wrapper_manager'), $container->get('file.mime_type.guesser'), $container->get('messenger')
   +      $configuration,
   +      $plugin_id,
   +      $plugin_definition,
   +      $container->get('config.factory'),
   +      $container->get('stream_wrapper_manager'),
   +      $container->get('file.mime_type.guesser'),
   +      $container->get('messenger'),
        );
   

   no other changes left in this file, so I think we can revert those too, not related.

Implemented and cleaned up. I also noticed two more things:
1. more reroll issues
2. the file getCachedFilePath method was returning null if cache file was not there and using that for setting as well
3. old config name in the update hook

I think what's still missing is changing the ExtractorQueue plugin, that also interact with key value.

Good catch, thanks.

Noticed an error when no config value was set for the cache backend.

Might it be worth just deprecating this module and using file_extractor instead? I believe it has a different approach to this problem.

From a quick glance, using that together with the file cache and a persist strategy should basically result in the same as we did here with our own file cache. I wasn't aware that that module has a persist strategy configuration.

I didn't compare the actual extract implementations whether they are better or not. It might be a feasible alternative and a more modern approach I'd say, as it exposes it through a computed field that can be integrated with many things. Switching might be somewhat expensive, as you'll need to extract everything again. And make sure to use the file cache module with the persist strategy, or it will be deleted on every cache clear.

Re: #42, I suppose a txt prefix could be added, but it also doesn't matter too much, it's just a cache and shouldn't be interacted with directly. Removing HTML is not the purpose of this, it is to store whatever the extraction method returns. HTML might be relevant for boosting the content for example and it's up to search api processors to remove that or not.

Might it be worth just deprecating this module and using file_extractor instead? I believe it has a different approach to this problem.

Wow... Deprecating a module with 10K installs in favour of a module with 116 installs... no thank you :-)

HI,
as said in #28 there is no problem adding a new storage method here.
i think we just need some people testing the patch and RTBTC :)
Thank

It's not, a clean up is currently not implemented. Also note that it would only trigger if the actual file entity is deleted, that only happens if the respective hidden core feature is enabled (removing files when usage goes to 0).

#41 still applies on 9.0

I just ran into this, when one of my sites stopped being able to run a database backup via drush sql-dump.

It looks like an enormous blob in key_value is making MySQL generate a statement larger than max_packet_size, which makes mysqldump error out. I am not prepared to bump the max_allowed_packet to a number over 100MB, that's ridiculous. And shoveling blobs this size around the database isn't performant. Even for the smaller ~ 1.5MB ones.

If this data is indeed present for caching purposes, it should be in a cache, which can be backed by any old thing via the standard Drupal caching mechanisms.

It is a cache, but it's also one that doesn't change unless the underlying files change and it's very expensive to build, so not, the default cache is not a great place for this. We've been using our patch for 1y+ now and it's working fine.

Also no idea how you manage to create a single 100MB blob from this. We're talking about pure text, that would be a very, very large document.

Ha! My workplace is the queen of very, very, VERY large documents the file in question is a large excel worksheet.

100MB Text isnt all that large. We have files that are GBs in size. When you get into govt docs, things get rather.... how should we say. LARGE and boring. Saving a file to a DB should never be done for any reason what so ever, whether it's text or not. Files belong on a file system. Not in a DB. This is why if anything should be saved at all, it should be hashes, not the entire contents of the file. There's no reason to save an entire doc to a file just to see if it's changed. That's absurd.

This is not about seeing if a file changed or not. It's about preventing the expensive operation of extracting text from a file. A hash is meaningless for this. And the bigger the file, the more expensive that is. Depending on how you set up the indexing, the same file might even be indexed for multiple entities.

And again, I'm not arguing to keep things in the database, we wrote the patch that allows to move it to the file system, so I'm not sure who you are trying to convince of what. And if you think that's not worth doing, then you can use the pluggable system introduced here to have a null backend that does nothing.

Also, there is a max file size setting for the processor, because I'm sure it's going to be quite expensive to index a 100MB of text in Solr and I have no idea how useful that is then. Kind of surprised that even works, I assume an excel file with 100MB of text is going to be considerable bigger and in our case, Tika regularly blows up on files smaller than 100MB.

#41 - has this been tested on D10? Any plan of releasing this patch to 9.0? I am currently using the module in D9 and the key_value table is 3GB+ which is affecting the overall performance of the site.

I was wondering if this patch is compatible with D9 and most importantly is it planned to be meged to D10.

I've added a new feature which is the ability of changing the output file location

Could be interesting to make a merge-request now.

We just need to let user choose to keep storing in database or in files (depending on their needs and possibilities), so an option on that would be great

This is only about where to store the extracted text from files for indexing. This is a system/admin level, global decision, it can not be per user.

yes of course, this is what i meant, have an option to let the admin decide if they want to store on database (as currently) or in the file system (as done by the patch)

Reroll #57

@rahaf-albawab Please provide an interdiff of the patch on #57.

Re-roll patch #57

This was a rough one to reroll, I'm not sure against which branches the rerolls exactly where, but even the last had several complicated conflicts due to DI changes for me on 9.0.x. Updated the issue fork and created a MR for it, didn't test this yet at all.

Re #64:
> yes of course, this is what i meant, have an option to let the admin decide if they want to store on database (as currently) or in the file system (as done by the patch)

I don't get this. The patch introduces configuration and a UI to configure the desired cache implementation.

I noticed that there are now 10.0.x tags, but the branch is still 9.0.x. @ixus, note that you do _not_ need to create new major versions just to update the Drupal core requirement. That only needs a minor update.

Tested successfully on Drupal 11.0.10 / PHP 8.3 / Solr 8.11.3.

Nice work everybody!

Tested this and it works perfectly. Old key value entries were cleared once I configured to use the file cache, and the cached extracted files persist through cache clears when preserve_cache is true and are correctly deleted when preserve_cache is false. Both methods continue to use the cache when available when indexing.

As far as I can see the open discussions on the MR from Frank have all been addressed in the last commit and I agree with the proposal to only select the scheme rather than a specific directory for the storage.

CI showed a minor typo and deprecation warning - once those are applied I think this is RTBC as far as I am concerned.

Thank you everyone who worked on this - file storage is generally cheaper than database storage so there a potential this change will not only be more performant but could save a few cents here and there. Looking forward to seeing this land soon 🙏

as per #68 i tested this locally and it works.
This is now merged
Please feel free to open new issues for enhancements suggested above and any other suggestion related to this feature.

A big Thank you all for making this possible and for all your contributions.

here it lands https://www.drupal.org/project/search_api_attachments/releases/10.0.2