Changes in related entities are not being re-indexed for all levels of entity references

It looks like we either do not have sufficient test-coverage or this change did not make an impact on them. But from the anecdotal evidence provided by @carolpettirossi this change makes sense.

I would love to have more testcoverage for this, but i'm not sure how much this will get us, because setting up that scenario is really complicated.
I'd love to get feedback from drunken monkey on this before moving this to rtbc.

Hi everyone,

As usual thank you for the incredible hard work on this amazing platform that is Search API.

I'd like to take this issue to express some concerns with the approach already committed to Search API in https://www.drupal.org/project/search_api/issues/2007692

The main problem is this: when we start baking in logic in regards to index invalidation on entity save, we need to consider scenarios where there are a LOT of dependent entities being saved. This means an entity save will eventually (on sufficient large sites) lead to a timeout or memory exhaustion.

Before this patch I was thinking of implementing this as a queue based system (which makes sense in our case) as, for us, an entity save can lead up to 10k index items changing or more (for example a change in a taxonomy term). This means we can take the Search API "we don't deal with this is too complex" approach (that I personally agree with) and bake in a solution for our specific problem.

With this now committed I'm concerned about the potential ramifications for sites like ours and more importantly how we can deal with this in order to stay compatible with Search API's new approach.

Hope this makes sense and looking forward to hear your thoughts!

I saw your post yesterday @kyuubi, thanks for adding another voice in this discussion.
Also a huge thanks for those numbers @carolpettirossi - those are really helpful.

So it seems like there are 3 paths forward.

1. Ensure that this code works more better with more performance.
2. Remove this behavior again.
3. Put it behind a checkbox.

I think trying the first option is going to be a whole lot of work so I personally would prefer we do this with a checkbox that is enabled by default.

Hi @borisson_,

Thank you very much for the quick feedback.

I think 1, realistically is always going to be tough to achieve generically. I imagine this is why Search API has historically steered away from this problem, due to the amount of possibilities and use cases.

I also think removing 2 is a shame, because realistically I'd imagine for more than 50% of sites out there, the current solution will be totally fine (unfortunately that's not our case).

Option 3 seems like a simple and very effective way to offer a good out of the box solution, with well disclaimed caveats.

So, I'd propose:

1. Add a feature flag for this (checkbox)
2. Add a warning/disclaimer informing the user of the potential performance implications on large sites with many references (not unlike the "index immediately" situation)

Would that work for everyone?

Thanks again!

1. Add a feature flag for this (checkbox)
2. Add a warning/disclaimer informing the user of the potential performance implications on large sites with many references (not unlike the "index immediately" situation)

This sounds like great solution, +1!

Agreed that a kill switch toggle like this is needed as the performance implications are pretty intense.

Tentatively setting this at RTBC, only bit I'm unsure on is the $relation_info['entity_type'] !== $entity_reference['entity_type'] change.

Attached is a simple re-roll of #10 to fix a whitespace issue.

There is an open bug for how the relationship info is applied to the calculation of related entities: #3178941: Fatal error "Call to a member function getColumns() on bool". Perhaps the bug report in #2 is related to or fixed by that?

+++ b/src/Utility/TrackingHelper.php
@@ -212,8 +217,7 @@ class TrackingHelper implements TrackingHelperInterface {
-        if ($entity_reference
-            && $relation_info['entity_type'] !== $entity_reference['entity_type']) {
+        if ($entity_reference) {

I do agree, these few changes look a little out of place in the current patch.

Agreed that the bug report in #2 does look like #3178941: Fatal error "Call to a member function getColumns() on bool", so removing that from here. Leaving as RTBC.

Looks good to me, +1

Thanks a lot for reporting this issue, carolpettirossi, for providing the clear analysis and the simple but effective patch. Sorry it took me so long to get back to this, unfortunately I had very little time for contrib work in the past few months.

Anyways, the above just pertains to the first few comments. Starting at #5, I’m not sure what happened.
@ kyuubi/#5: This is a completely different problem! Why did you think it would be a good idea to hijack this issue and make it about your problem instead?
And then, why did everyone else go along with it – in the end even removing the fix for the actual issue reported, without waiting for a reaction from the issue reporter? (I also don’t see how “doesn’t work sometimes for deeper nesting” sounds similar to “I get a fatal error”?)

@ carolpettirossi: Could you please try and test again with the latest dev version of the module (after #3178941: Fatal error "Call to a member function getColumns() on bool")? Is the problem still present, and does your patch (#2) still fix it? (As noted above, I don’t see why the two should be related, but better to verify first.)

@drunken monkey you are absolutely right, it's actually not related and it was not my intent to deviate things from the original issue.

To clarify, I was actually working on the consequences of this patch with Carol and given it was caused by the linked patch it sort of evolved to "lets fix the issues the patch created" (which are the ones stated in the title). I understand that might not have been the best way to go about it and I apologise.

However given everyone is just trying to help here, maybe we can do away with salty responses for everyone's benefit? I wasn't trying to make it "my problem instead" as I'm literally working on the exact same issue as Carol (we work together). For us they are one problem (caused by the patch) but I can see that wouldn't be the case for a maintainer.

Let me know if you want me to move the issue (and fix patch) to a separate issue and I'll open it up.

Thanks

OK, thanks for your apology, kyuubi, and sorry in turn that my response came out a bit harsh. It just seemed very rude to me – I couldn’t know you’re working together on this, which of course changes things. You might have just mentioned it in your comment, to explain why you felt it appropriate to discuss both issues at once.

However, it’s still unclear to me whether the original issue is a) already resolved in HEAD or b) resolved by the latest patch. (Neither seems to be the case, from the looks of it – but would be great to have confirmation.) If neither is the case, then yes, please create a new issue for the performance issue. Otherwise, it seems OK to just continue discussing this here. (Feedback from carolpettirossi in this regard would be great, in case you can ping her.)

That is very strange indeed. As you’re aware, we have automated test coverage for this functionality, so it’s unlikely this was broken by some new code. Also, I just tested and it works fine for me. Sorry, but I don’t really know why it suddenly stopped working for you.

However, I now tried myself and the problem (and functionality) is easy to reproduce on a new Drupal installation. Just …

1. add a node reference field to a content type
2. create a chain of three nodes referencing each other (1 -> 2 -> 3)
3. add a field two levels down to the search index – for instance, “Node ref » Content » Node ref » Content » Title”
4. re-index
5. (create a search page, if necessary, searching just that new field)
6. verify that searching for a word in the title of node 3 yields node 1 as a result
7. edit node 3’s title and see if the search reflects the change without manual re-indexing

The attached patch revision adds test coverage on top of #2. Through it, I was also able to determine why that check you removed was there in the first place – without it, it seems the nested :entity property for all entity reference fields causes us to lose the bundle information. A bit awkward to work around, unfortunately – or at least I couldn’t think of a good way to do it. I even had to hard-code the 'entity' property name – but at least it should now work again.
Please test/review!

@ kyuubi: As the original problem still exists, please create a new issue for the performance problem.

As there is test coverage for this new functionality, I’m pretty confident it works as designed, but would still be great to get feedback on this before committing.

Great to hear, thanks a lot for testing and getting back to me. Sorry that I now produced a further delay at my end.
Anyways, committed. Thanks again!