Problem/Motivation

While testing 11.4.0-beta1 we noticed that our views no longer loaded if the 'changed' date was added.

Entity loading loads all single cardinality fields in a single join query, and the same for mulitiple cardinality fields. This runs up against the 61 table join limit if there are about 59 or more single cardinality fields on a site. See https://dev.mysql.com/doc/refman/9.7/en/join.html

Mutliple cardinality fields have the same issue but are less likely to be present in high numbers on the same entity bundle.

Steps to reproduce

Have a content type with 50+ fields
Create a view adding the changed date
View will throw erorr

Proposed resolution

Chunk the fields into groups of 50 (per #3)

Remaining tasks

Review

User interface changes

NA

Introduced terminology

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

Issue fork drupal-3593963

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

smustgrave created an issue. See original summary.

godotislate’s picture

godotislate
he/him
commented
Version: 11.4.x-dev » main
Issue tags: +Release blocker, +11.4.0 release priority
Related issues: +#3564689: Combine multiple cardinality field loading into a single database query, +#3551308: Combine single cardinality fields into a single database query when loading entities

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

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

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.

catch opened merge request !16003

smustgrave’s picture

smustgrave commented

Pushed up a test case. I was able to replicate on a fresh install too using a script to create fields.

smustgrave’s picture

smustgrave commented

Locally with the fix in the MR I get

) Drupal\FunctionalTests\Entity\SqlContentEntityStorageTest::testEntityPageWithManyFields
Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'created' cannot be null: INSERT INTO "test83509083users_field_data" ("uid", "langcode", "preferred_langcode", "preferred_admin_langcode", "name", "pass", "mail", "timezone", "status", "created", "changed", "access", "login", "init", "default_langcode") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14); Array
(
    [:db_insert_placeholder_0] => 1
    [:db_insert_placeholder_1] => en
    [:db_insert_placeholder_2] => 
    [:db_insert_placeholder_3] => 
    [:db_insert_placeholder_4] => admin
    [:db_insert_placeholder_5] => $2y$04$XuiJSqAuM6JpAqlNyIDgMOK7KbrrNwkoA52onl5tz0/XW5ZSezlp2
    [:db_insert_placeholder_6] => admin@example.com
    [:db_insert_placeholder_7] => Australia/Sydney
    [:db_insert_placeholder_8] => 1
    [:db_insert_placeholder_9] => 
    [:db_insert_placeholder_10] => 
    [:db_insert_placeholder_11] => 
    [:db_insert_placeholder_12] => 
    [:db_insert_placeholder_13] => admin@example.com
    [:db_insert_placeholder_14] => 
)
smustgrave’s picture

smustgrave commented

Did use claude for some help

https://git.drupalcode.org/project/drupal/-/merge_requests/16003/diffs?c... was failing with entities with no fields was my understanding

https://git.drupalcode.org/project/drupal/-/merge_requests/16003/diffs?c... phpstorm was complaining that $translation was updated but never used. And loadSingleCardinalityFields was added to not readd fields like created, I was getting duplicate entries locally without that.

But all green now :)

smustgrave’s picture

smustgrave commented
Issue summary: View changes
quietone’s picture

quietone commented
Issue tags: -Release blocker +11.4.0 blocker

Changing tags per Issue tags -- special tags

catch’s picture

catch
he/him
Primary language English
commented

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.

catch’s picture

catch
he/him
Primary language English
commented
Title: Fatal error when node has 50+ fields » Field loading can hit the MySQL 61 table join limit if there are ~60 fields
Issue summary: View changes
smustgrave’s picture

smustgrave commented

This is where I’ll need help turning to a kernel test returns 200 without a change

godotislate’s picture

godotislate
he/him
commented

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...

catch’s picture

catch
he/him
Primary language English
commented

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.

godotislate’s picture

godotislate
he/him
commented

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.

godotislate’s picture

godotislate
he/him
commented

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

joachim’s picture

joachim commented
Status: Needs review » Needs work