Tracker-Sync Task does not scale well with a lot of content

Thanks for reporting this issue!

I think you will agree that we're at least already doing a much better job of handling this than previously (where large sites just resulted in WSoD when initializing tracking). So, "takes basically forever" is already a large improvement.
But of course, there is always room for improvement, and I believe you that the experience is still not pleasant with large sites.
I also have to admit that I didn't think of the performance problem large offsets might pose. (I also never knew the internals of entity queries.)

In any case, thanks a lot for your suggestions on how this might be improved! Even if they aren't 100% practical to apply to the plugin code directly, it's at least good to discuss such options.

1. Avoid the GROUP by, which is obviously not trivial as this is done by entity qury, the only way for that would be to not use an entity query, so check for sql storage I guess and then implement a custom query that doesn't do that. Could for example always query the base table, then it wouldn't need the group by.

We did something like this in Drupal 7, but as you mention yourself, this isn't very reliable or generic. Even if an SQL storage is used, people might have custom overrides in place which could break this. So, if we implemented this, we'd have to make it optional in some way, which is always bad with options that 99% of users won't be able to understand. It's probably safe to have it enabled for the vast majority of sites, but there might be popular modules out there that break this, and non-technical site builders using them, so how do we ensure they'll know what to do, even if they realize there's a problem?
On the other hand, since this is now quite performant and unproblematic for most (smaller) sites, leaving this off by default and having it as a (hidden) expert setting could make sense.

But if it's easy enough to implement, then yes, I would consider adding this as an optional variant for the method.

2. Instead of working with paging it could basically query on ID > $last_id and then just the limit. Of course that's not really possible since the API works with a pager. :-/

You're right, we don't have $ast_id, but we can still pretty easily get rid of the offset – see the attached patch. However, especially for small page sizes, this can easily fail if multiple items in between are deleted – as soon as an empty page is encountered, the code will assume there are no more items left, and not make further calls.
The same is true, but even more likely, if bundles and/or languages are specified. If all of the first 100 (e.g.) nodes are in a certain language, but a different one is specified, no nodes at all will be found and added.
To circumvent this, we could potentially just retrieve the maximum ID and return NULL if the minimum ID we'd specify is greater than that.
However, all of this would fail for entities with non-integer IDs. Do you happen to know if they are guaranteed to be integers for all (content) entities? (Negative IDs might also pose a problem. Are these also explicitly forbidden?)
And even if we could resolve all of this, it would make for a lot of empty sets being returned in some cases (with a lot of deleted items, or when languages/bundles are specified), potentially making the whole process even slower, not faster, in such cases.

As a nice side effect, though, this would also resolve the original problem described in #2880026: Fix bugs in entity datasource's item discovery code in edge cases. (Which, by the way, would also finally add proper testing for this logic. Running the tests on the attached patch, though, right away shows its flaws.) So, an interesting idea in any case. If we can get it to work reliably, we might actually commit this to the module. (We might want to make it optional, though, since this will actually worsen performance in some cases. On the other hand, though, this isn't called that often anyways, so improving performance for large sites would probably be worth making it slower in some cases for smaller sites.)

As an alternative, we could consider just saving the $last_id for a certain set of method parameters, and just use that instead of the $page (or in conjunction with it – i.e., verify that it was incremented by 1 and panick otherwise). Would be a quite complex solution compared to the current code, but not much more complicated than the above, and probably more reliable. Would have to think further about possible problems with it. (Should of course use the site state for $last_id storage.)

Also, as a side note:

The problem is that the entity query that it is doing can be quite slow, combined with the fact that it only loads 100 entries at at a time means that it takes basically forever on a site with a large amount of content.

You can change that value with the search_api.settings:tracking_page_size configuration key. Just set it to whatever value you think won't result in out-of-memory errors (or experiment). That might at least alleviate the problem for your site.

In any case, thanks again for kicking off this discussion!

Yes, you are right, non-integer entity types are a problem and my approach there doesn't work.

Hm, actually, it's not your approach that's not working, just my adaption/implementation: "greater than" comparison works fine for strings, too, so neither string IDs nor negative integers would actually pose a problem. They just become a problem when we try to calculate $last_id based on $page.
Entities created after adding the index (or changing the datasource settings) are already tracked properly*, so it also doesn't matter whether entities are added in order of ascending IDs (which is unlikely for string IDs, of course).

All in all, I think my suggestion of just tracking the $last_id in the site state looks pretty feasible after all. Adding at least "advanced" options to switch to this, and the raw-SQL variant, might really be a good idea.

* Actually, just saw that they're not tracked completely properly: #2886978: Newly created entities of disabled languages are still tracked.

Just ran into the same problem. The patch I think is slightly wrong in as much as it should still use range with offset 0, not try to guess the max ID.

Oh, looking deeper - I see this needs a rather more substantial rewrite to do this correctly. You need to track the last ID, rather than the page in the task.

Just ran into the same problem. The patch I think is slightly wrong in as much as it should still use range with offset 0, not try to guess the max ID.

Oh. Interesting. You’re right, that would eliminate at least the problem of sparse/empty sets. Good idea!
However, the problem regarding non-integer IDs remains, so we’d either have to check the data type of the entity ID field, or switch to the solution via $last_id (which, as a bonus, also works for negative integers, if that ever happens).
What do you think of the other suggestion, the raw-SQL fallback? Also worth pursuing?

Of course, we can implement either, both, or all three (including the “don’t serialize container“ suggestion), should all help somewhat.

Untested and needs work, but here's a start on the idea of using state.

Yes, looks exactly like what I had in mind, thanks!
Won’t go into nit-picking, as you say yourself that it’s a WIP, just this: unless I’m mistaken, $languages (even though not part of the entity query, there still could be tasks running for different sets of languages) and $this->getIndexId()->id() also need to be part of the key for $last_ids. So, probably we’d best just collect all the context in an array, serialize and create a hash? Seems most practical. (In any case, this should probably be cleaned up again when returning NULL.)

Also, as discussed, while the change at the end of the method makes a lot of sense, it will have to be dependent on a version check for Drupal 8.6+ for the time – or we just wait until we depend on that (i.e., after the release of 8.7) before committing, if you’re in no hurry in that regard. (Otherwise, have to come back again and remove the BC part.)

Ok, here's a more complete patch including some feedback from drunken monkey in slack.

Takes the process of creating several large indexes locally (on entity type has > 600k) from ~30 min to ~11 min.

Great job, thanks! Looks pretty good now.
I still had some changes, but mostly minor style issues, a lot of them just personal taste.
I think there were only two actual changes:

• I think $selected_bundles is unnecessary and might, in fact, even cause problems in edge cases. The tracking code is always called with the same parameters for one batched tracking operation, so just using the method arguments as-is for the context key should be enough. Whether the SQL query then uses a different set of bundles isn’t really our concern, I think. (Side note: A lot of such edge cases, where important config changes between start and finish of the batch operation (which, after all, can be aborted and continued later, so could in theory take days, too) could currently lead to bugs where some entities wouldn’t end up in tracking. A nice side effect of this new approach would be eliminating that potential problem.)
• I changed the state key to 'search_api.datasource.entity.last_ids'. As the datasource ID is entity, I found content_entity in the key a bit off, so I think it’s cleaner this way. Plus extracting it into a constant makes it easier to use.

Otherwise, as said, mostly just code style changes and small refactoring.

As for tests: I think this is actually already pretty well covered. But even so, we should probably add test coverage for the fallback, just in case the site state becomes corrupted for any reason. If we include a fallback, we should make sure it works.
Adding test coverage to make sure the static cache is correctly cleared (which should happen for all versions, pre-8.6 and higher) might also make sense.

But first I’m curious for feedback – both from Peter and, optimally, also from others.
Berdir, still interested in this? What do you say to this approach? (The “raw SQL query” workaround could later also be added, but is pretty independent from this after all.)

Hm, RenderedItemTest doesn’t seem to finish now. Will have to investigate.

Er, OK, due to (probably) a mistake the RenderedItemTest included the entity:search_api_task datasource, which (for understandable reasons) doesn’t play very well with this change.
The datasource is absolutely ridiculous to begin with, of course, so maybe we should just remove it from \Drupal\search_api\Plugin\search_api\datasource\ContentEntityDeriver::getDerivativeDefinitions()? (Though I guess that would be a breaking change – but can you really imagine anyone actually using that datasource?)
(Otherwise, I guess we’d better check via getEntityTypeId() instead, to also cover sub-classes – now that I think about it.)

In any case, feedback would still be very welcome!

Feedback, anyone? This would be a great improvement for large sites, I think, so bit of a shame to leave it uncommitted just for lack of reviews.

Also, thinking about it, we should generally avoid adapting tests just because they fail – especially when, as now, they reveal an actual problem (albeit an almost certainly theoretical one). Not really sure why I did that, in hindsight.
So, patch revision attached which reverts that change, and adapts the fix for the test fail as discussed in the previous comment.

+++ b/src/Plugin/search_api/datasource/ContentEntity.php
@@ -129,6 +153,12 @@ public static function create(ContainerInterface $container, array $configuratio
+    // This service was added in 8.6.x.
+    // @todo Remove the "if" once we depend on Drupal 8.6+.
+    if ($container->has('entity.memory_cache')) {

I think we already do this? That's the only thing I could see being off in the patch.

I think we already do this? That's the only thing I could see being off in the patch.

No, we’re not – see #3053200: Increase Core version requirement to 8.6.
Since I unfortunately still can’t commit that issue, I committed this one with the 8.5 compatibility code. Can be removed once we commit the other issue. (Hm, or I can just add it to the patch there right away, I guess …)

I’d have hoped for more input from the people who actually proposed this, and could have tried whether this still brought the desired improvements, but I guess we can at least be reasonable sure it won’t make things worse, and is a step in the right direction.
So, committed. Thanks again for everyone’s work here!

Not sure whether to keep this open for the other suggestion(s) or whether we should use a follow-up issue for that/those? What would you say?

I think a followup that links to this issue is the easiest way forward. That way other don't have to dig trough all the history in this one?

As it happens, there already is an issue for that, even one with very recent activity: #3013663: Speed up item tracking for large sites.
So, I created a second follow-up for the other suggestion (#3057644: Avoid serialization of the container in task batches) and am marking this one as “Fixed”.