Field loading can hit the MySQL 61 table join limit if there are ~60 fields

As mentioned by @berdir on Slack, this likely is not related to views but the change to entity field loading to single queries in #3564689: Combine multiple cardinality field loading into a single database query and #3551308: Combine single cardinality fields into a single database query when loading entities.

Can start with a test:

what would be useful is a test. should be pretty straightforward. create enough fields in a loop with something like field_foo_$i, create, save and load that entity, watch it explode
probably best with entity_test or so in an entity kernel test

This will be the single and multiple cardinality field loading which do one query each.

The most immediate thing I can think of is chunking the fields into groups of 50, so that it always stays under the join limit.

Moved the single cardinality field loading to a helper method, the method has a function signature of doom. Front page of Umami loads and phpstan is happy, didn't check beyond those yet.

Changing tags per Issue tags -- special tags

From a quick review of the changes overnight it overall looks good - always load shared fields even when we don't have any single cardinality fields, don't load shared fields twice when we don't need to, and fix silly mistakes like not passing $translations in. All of these are a consequence of moving the field loading to the loop, so necessary to preserve the previous behaviour. Didn't do a line by line review yet.

I guess there's a further question of whether we think a site could have 60ish multiple cardinality fields, to be honest that feels extremely unlikely compared to single cardinality ones, but I'm not sure whether we should do it here just in case, or open a follow-up to try to land this earlier and keep it reviewable. The actual code change will be pretty independent either way.

Web search results suggests that the table join limit for MySQL and MariaDB is 61, no hardcoded limit for PostgreSQL, and 64 for SQLite, so it might make sense to bump the number of fields in the test to 65.

Sources:
https://stackoverflow.com/questions/26246413/how-many-max-join-table-at-...
https://stackoverflow.com/questions/23389820/what-is-the-maximum-number-...
https://stackoverflow.com/questions/18713348/is-there-join-number-limita...
https://stackoverflow.com/questions/5599867/does-postgresql-have-a-limit...

The cardinality -1 branch will always fail because we're not handling that case yet. For now I've pushed a commit to remove the data provider, we can add it back either if we tackle multiple cardinality fields in this issue, or in the issue that does. Also fixed the test regression I introduced which was failing to update one number.

Haven't looked at moving to a kernel test or removing the translation bits yet though.

Haven't looked at moving to a kernel test or removing the translation bits yet though.

Added a commit to change this. If we're not doing multi-fields in this issue, should a comment to the test that it's for single cardinality fields only.

Ran the MariaDB, SQLite, and Postgres jobs just in case. A couple failures that look unrelated.

I've opened #3594066: Avoid table join limits when loading multiple cardinality fields.

It should hopefully be a bit simpler than the single cardinality case because we don't have base/revision fields to worry about as well. Will try to get something going on there next time I'm properly at computer, if it turns out to be not too bad and this hasn't landed yet, we could try to fold it back in here.

Use multi-valued base fields to save on creating a ton of config.

@joachim this issue is for single valued fields, see #3594066: Avoid table join limits when loading multiple cardinality fields for multiple value fields. They have separate code paths.

MR is up on #3594066: Avoid table join limits when loading multiple cardinality fields and is green. The multiple case is easier because it only has to worry about multiple cardinality fields, so it's pretty much a 100% copy and paste of the logic to a helper + the chunking and foreach loop. This means that we probably could merge the change back into this issue and fix everything in one go if we want, or at least it'll be a hopefully straightforward follow-up if we keep them separate.

@catch We're still iterating here, so it might make sense to pull that one in?

OK brought those changes back over here.

Added constants for both the threshold and chunk size.

Dropped the field number in the test back down to 71.

Test-only run here: https://git.drupalcode.org/project/drupal/-/jobs/10156659

Unchecking "Show whitespace changes" in GL UI made comparing the diff easy.

Comments for typos in the MR.

I almost wonder whether the new methods should be closures instead. We wouldn't need those the giant useless docblocks. Though instead of a long parameter list, there'd need to be a long `use` clause. Not a blocker though. Otherwise I think the fix change looks good. I think someone else will need to approve the test change, since I contributed to that.

Postgres test-only: https://git.drupalcode.org/project/drupal/-/jobs/10156891 doesn't fail (as expected because no join limit)

sqlite test only: https://git.drupalcode.org/project/drupal/-/jobs/10156912 fails as expected.

I almost wonder whether the new methods should be closures instead. We wouldn't need those the giant useless docblocks. Though instead of a long parameter list, there'd need to be a long `use` clause.

I nearly did this but thought they'd be very long closures. Also in the back of my mind I remembered this method is in the top four complexity hotspots https://dbuytaert.github.io/drupal-core-metrics/ although the massive list of parameters does not make it more readable IMO. So.. hard to tell which will be better without having both to compare probably. It's something we could change in a follow-up too though, probably without a deprecation if we do it before 11.4.0 is actually released.

Pushed a slight change to the chunking logic.

There's now a minimum chunk size of 25, when there are two or more chunks, we first group into chunks of 25, then append the last chunk to the penultimate chunk. This means 26 field gets loaded in a single chunk of 26, 50 fields get loaded in a single chunk of 50, 51 fields get loaded as a group of 25 and 26 - e.g. it should eliminate the 'last chunk only has one item' problem regardless of the number of fields.

Applied two of @godotislate's suggestions and replied to the comment.

lgtm

Is there a way to configure/override number chunks? as pgsql and sqlite can use more

Also comment need adjustments as SQL is too broad for explanation

@andypost even if they can theoretically do more there are other limits that can be hit like max_allowed_packet. The MR also leaves a small amount of headroom which is intentional. Configurability could be added in a follow-up, this is blocking 11.4.0 and don't see any reason to hold it up on that.

The comments link to both mysql and sqlite docs which seems specific enough.

Committed and pushed 7d3b03497b6 to main and 364e29e132d to 11.x and ebe94815623 to 11.4.x. Thanks!

I credited myself on this issue as @catch and I chatted extensively about this issue and this lead to small improvements like https://git.drupalcode.org/project/drupal/-/merge_requests/16003/diffs?c... and https://git.drupalcode.org/project/drupal/-/merge_requests/16003/diffs?c... for example.