Insane etid / {field_config_entity_type} abstraction

Now that's... unpleasant.

With RC1 out, how much of it can we kill? Technically, these are internal implementation details, nothing user-facing (if we disregard Views, which would benefit with this changing anyway...)

Stil waiting to see if anyone has an explanation why this is good, the Field API group brainstormed on a lot of details that I'm not familiar with.

There is absolutely no good reason for this. Definite WTF.
Unfortunately, by the time this fact was established, we were already in 'no schema change' freeze.

Yes, we discussed changing that a few times already (including in Vienna while porting Views to the Field API...), but simply failed to actually do it.

I don't believe it is actually a bottleneck for Views.

I can't stand this but while I have worked on Field API a fair bit, I've never had to directly deal with the etid stuff, so was never motivated to look at it. I've not seen anyone else really mention it until sun opened this issue, which suggests if we killed it, no-one is going to notice.

Also:

function _field_sql_storage_etid($entity_type) {
  $etid = variable_get('field_sql_storage_' . $entity_type . '_etid', NULL);
  if (!isset($etid)) {
    $etid = db_insert('field_config_entity_type')->fields(array('type' => $entity_type))->execute();
    variable_set('field_sql_storage_' . $entity_type . '_etid', $etid);
  }
  return $etid;
}

Is this 'caching'? If it is, let's either remove it or have it be a cache_get().

If we have to have etid, what about adding it to entity_info()? That would be both cached, and an api function.

I guess field.module could add the etid in field_entity_info_alter().
That's taking a sad but relatively self-contained-in-field_sql_storage WTF, and promoting it to the entity API though...

@sun : relation.module directly hits field_sql_storage table ??

Well, that makes things less ugly.
Nice cleanup, sun!

Is there any chance we could address:

4. and only field_sql_storage module contains a private helper function to resolve the mapping...

Can't see that change hurting anyone...

Sure, thanks sun ! patch #11 is a nice no-brainer. RTBC if the bot comes green

re #12 : Only field_sql_storage uses that ugly construct. I see no need to promote it outside of field_sql_storage ?

@yched
Okay, then. Makes sense (I was only responding to sun's initial list of problems from the original post)

Attached patch fixes failures in "Field SQL storage tests" and "ExpandNode entity query alter" test classes, caused by leftover variable_get('field_sql_storage_[TYPE]_etid') calls in FieldSqlStorageTestCase::testEntityTypeId() and _node_query_node_access_alter().

The fail in "Comment interface" looks like it is caused by the static used in _field_sql_storage_etid(), not being refreshed on the client side while being outdated on the tested side. Your typical fun with tests and statics. The test passes if I remove the drupal_static() call.

I did not fix that last fail, stopped there for tonight. There should be one fail left, but I'll let the bot confirm.

The _node_query_node_access_alter() bit mentioned above worries me a lot. I didn't realize etids had spilled in node access query rewriting.

field_sql_storage_field_storage_query() adds a 'entity_field_access' tag to the entityQueries.
This triggers _node_query_node_access_alter(), which in turn has code which relies on field_sql_storage's etids, even though field_sql_query might not be the storage engine in use (the module itself is required, though, so etids are at least always available). This means that a module that wants to provide alternate SQL storage *has* to use field_sql_storage's sick etid construct if it wants to trigger node_access checks.

At what point do we decide that this is silly and we're better off altering the schema to get rid of {field_config_entity_type}, and store string entity_type names in our field data tables ?
I hardly see how this would affect anything else than Views, and my bet is they would be more than happy with the change.

Oh good lord this is major WTF. I've been wondering for a long time why we've even needed etids.

Started a patch in #995534: Helper issue for #986992

Attached patch removes the field_config_entity_type table, and stores an 'entity_type' string instead of an 'etid' int in the field data tables. Upgrade path included.

Note : around 1/3 of the patch is updating sample hooks copied straight from field_sql_storage in field.api.php (eww).
The actual changes are relatively straightforward.

Fix comment typo.

I vaguely remember something about using the numeric etid instead of the string entity type name to speed up performance or improve indexing or something like that in the field tables. In case that bit of history helps or changes anything.

I have code that will probably have to be rewritten but I am not at all opposed to making this change.

@KarenS : Exactly. The etid indirection was introduced under the assumption that running JOINs or WHEREs on an int column would be faster than on a string.
According to DamZ, this is useless to begin with, as db engines already perform such an optimization internally on indexed string columns having a small number of distinct values - which exactly maps the case of the 'entity_type' column in field data tables.

So, big WTF for no gain :-). But this spills over to Views (the generated query can be different on dev site and on production site because etids can be different), and, as #17 shows, to any other field storage backend that might want to store fields in SQL.

I can haz simplification pliz?

Nice beefy 38k patch. Hopefully the test coverage is good enough so that we don't have regressions here :)

This patch simplifies the views code a bit.

Additional as sun explained in irc the join will not be slower.

I'll commit this patch, but I'd love to have @sun sign off on the latest patch.

re $indexes : right, leftover from a previous version where the indexes were added along with the column. #30 is correct , thanks !

Great. Thanks for this important. Committed to CVS HEAD.

Breathtaking. I think this probably needs to be announced, with basically the description in #30.

If you announce this then also announce that if you are affected by this then it's very , very likely you are doing it wrong.

Docs page needs to be fixed at http://drupal.org/node/890070

Updated http://drupal.org/node/890070

I pulled and ran update.php today and got these results:

The following updates returned messages
field_sql_storage module
Update #7001

    * Failed: DatabaseSchemaObjectExistsException: Cannot add field <em class="placeholder">field_deleted_revision_11</em>.<em class="placeholder">entity_type</em>: field already exists. in DatabaseSchema_mysql->addField() (line 324 of /home/rfay/workspace/d7git/includes/database/mysql/schema.inc).

After the failed update, visiting /node gets this result:

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'entity_type' in 'where clause': SELECT t.* FROM {field_data_field_i2} t WHERE (entity_type = :db_condition_placeholder_0) AND (entity_id IN (:db_condition_placeholder_1, :db_condition_placeholder_2, :db_condition_placeholder_3, :db_condition_placeholder_4)) AND (language IN (:db_condition_placeholder_5)) AND (deleted = :db_condition_placeholder_6) ORDER BY delta ASC; Array ( [:db_condition_placeholder_0] => node [:db_condition_placeholder_1] => 206 [:db_condition_placeholder_2] => 208 [:db_condition_placeholder_3] => 209 [:db_condition_placeholder_4] => 210 [:db_condition_placeholder_5] => und [:db_condition_placeholder_6] => 0 ) in field_sql_storage_field_storage_load() (line 326 of /home/rfay/workspace/d7git/modules/field/modules/field_sql_storage/field_sql_storage.module).

It's possible that this is related only to my local and something about its history, in which case this can be demoted from critical. I'll try to recreate it. I unfortunately did not make a backup of my db before updating.

Related views issue: #998666: Adding FieldAPI fields causes "Column not found" errors, expecting an "entity type" field
On the other hand, I updated two old dev setups without a problem...

@rfay, maybe you pressed two times simultaneos(double click) in the upgrade button, and execute two times the upgrade? or this don't work like that?

this patch worked fine for my site.

I'm working on trying to recreate. I did find a dump of my local from a couple of weeks ago.

Reverting the code to right before this commit and updating the db and then trying to add a field to a content type I get this right when trying to add the field:

PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd7git.field_config_entity_type' doesn't exist: INSERT INTO {field_config_entity_type} (type) VALUES (:db_insert_placeholder_0); Array ( [:db_insert_placeholder_0] => node ) in _field_sql_storage_etid() (line 110 of /home/rfay/workspace/d7git/modules/field/modules/field_sql_storage/field_sql_storage.module).

I'll see if I can get a clear result on this.

I am able to recreate this #fail with this sequence of events.

1. Update the code to latest
2. Create a new multivalue file field on the article content type
3. Then run updatedb.

Since this is not an acceptable upgrade scenario (work done after code changed but before updatedb) I'm going to put this back to fixed. If there are appropriate scenarios in which it happens we can reopen.

The error message in #37 does seem to indicate that either :
a) the update was run twice
b) for some reason, the field_deleted_revision_11 table was added twice in the list of tables to process
at the beginning of field_sql_storage_update_7001()

I hardly see how b) could happen, but this could possibly be prevented by keying the $sandbox['tables'] array :

       $sandbox['tables']["field_deleted_data_{$field['id']}"] = "field_deleted_data_{$field['id']}";
       $sandbox['tables']["field_deleted_revision_{$field['id']}"] = "field_deleted_revision_{$field['id']}";

Not where I can roll a patch, unfortunately...

crosspost :-)

I guess we *could* meet nasty edge cases producing this mix of 'old version' (etid) and 'new version' (entity_type) schemas for field data tables.

Example :
A few weeks/months from now, a d7-RC2 site gets wants to update to d7.2, and updates a couple contrib modules too. One of the updates, foo_update_7003() creates a field (foo can be core or contrib).
If foo_update_7003() runs before field_sql_storage_update_7001(), its field will be created using the new code, and thus directly with the 'new' schema. When field_sql_storage_update_7001() runs, it produces the errors mentioned in #37 when it tries to process the field table.
Predicting what happens in the future with updates and dynamically created tables has always been a fun sport with CCK :-)

In our case, it's easy to be more tolerant : only process a table if it still has an 'etid' column. Patch attached.

That's a good one.

I also had a problem with this fix that was committed to core. My {field_revision_body} table has two revisions for node 2. The update drops the current primary key on etid, revision_id, deleted, delta, and language, and then attempts to add a primary key on the fields entity type, entity id, deleted, delta, and language, which are all the same for both revisions, and so I get SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry[error] 'node-2-0-0-und' for key 'PRIMARY' when running updatedb. Previously my revision tables do not have any primary key. So now I'm stuck with tables that have no primary key and have not had their etid columns removed.

Crap. Correct, my bad. Will provide a fix tonight.

So, the update functon that is currently in HEAD forgets that revision tables use different primary keys than 'current version' tables.
Fixing the current update function is easy, patch attached.

Working on a followup update func to fix dbs broken by the previous update :-(

Well the update failed for me, so we can fix it in the same update function. Looks good to me, but could use another tester.

Dbs where the upgrade has failed are left in a state where some tables have been processed, some haven't, and one (the one where the upgrade failed) is left in between (no PK, both the old 'etid' and the new 'entity type' columns).

Here's a new version of field_sql_storage_update_7001() that accounts for all cases, by wrapping checks around each step (add the new column, create the PK, drop the old column). As Dave Reid points, 7001 will replay on dbs where it previously failed.

I tested this to work on a 'clean' pre-commit d7 db, and on a db where the current field_sql_storage_update_7001() broke in the middle.

Sorry for the trouble :-(

Latest patch works for me, thanks yched. The upgrade function from HEAD left me in table mismatch hell.

Patch works for me on the side where I had the primary key error.

I have a different site where I got the following error:

Failed: DatabaseSchemaObjectExistsException: Cannot add field field_revision_field_properties.entity_type: field already exists. in DatabaseSchema_mysql->addField()

It looks like it tries to add the entity_type field again, this error already happened when i run the update the first time, without this patch.

#57 / #58 make sense, but then _field_sql_storage_schema() needs to be adjusted for the new PK for revision tables.

re #56: it's not clear whether that "Cannot add field entity_type: already exists" error still happens with #54. It normally shouldn't, the column is only added if !db_field_exists($table, 'entity_type')

#60 is good for me, although I can't RTBC myself.

New error, don't ask me what the hell this is.. :)

Failed: DatabaseSchemaObjectDoesNotExistException: Cannot add field field_deleted_data_7.entity_type: table doesn't exist. in DatabaseSchema_mysql->addField()

No idea where field_deleted_data_7 is coming from. I deleted a field that was using a different storage_backend (mongodb) on that site, maybe that's confusing it..

Wrapped both updates in if (db_table_exists($table))

Yup, agreed.

subscribing.

The comment "Revision tables of deleted fields are irrelevant, since no new data is written to them." would be much nicer if it would say "Revision tables of deleted fields do not need to be fixed here, since no new data is written to them."

Second, I do not understand what scenario got broken here. If it's the D6->D7 upgrade path then why is there no test? If it's not then why is this critical? If this is critical, then there must be a test for sure.

The D6 -> D7 upgrade is not involved. field and field_sql_storage did not exist in D6, so field_sql_storage_update_X() are not played, and don't need to. Fields created during the upgrade are created with the correct schema straight ahead.

What was broken here is the D7 -> D7 upgrade. We currently don't test this, and I don't really see us adding tests for each new upgrade in the cycle ?

The latest patch now runs through the update of my crazy site successfully :)

I agree that we don't need to add tests for a RC -> RC update. Instead, I think it is important that this is fixed ASAP, before more sites are messed up. My site with deleted mongod fields and revisions already is quite tricky to get right, we can always add another bugfix if yet another special case shows up.

Subscribe.

Noticed that revision saves failed on HEAD but not on RC2. This patch fixes that issue (which is item 2 in comment #66). +1.

Then let's go.

Committed to HEAD. Thanks!

@sun : it's not clear to me why we need this ?

IMO we shouldn't do performance optimizations on RC => RC sites. IMO we should go back to fixed.

Agreed.

It's true that running 7002 is not needed if you ran the correct version of 7001 that is now in HEAD.

I'm a bit wary that we start to add those 'conditional update' considerations though.

Not critical anymore though.

Sorry, crosspost. Back to 'fixed', as per webchick's #80.

+ Resetting original title and priority, for history's sake.

I ran into this error while upgrading my website from RC2 to RC3 and had to revert to a backup.

From what I read, this problem was supposedly fixed before RC3 came out, so what might explain this happening to me?

Which error did you actually get?

In general Reverting a version isn't supported.

Please update to RC3 and fix the problem there.

Tried to reproduce this...

So I upgrade from RC2 to RC3 after having disabled all non-core modules (mostly "media gallery" and prerequisites)

After the upgrade, before running update.php, trying to visit the site results in this:

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'entity_type' in 'where clause': SELECT t.* FROM {field_data_body} t WHERE (entity_type = :db_condition_placeholder_0) AND (entity_id IN (:db_condition_placeholder_1, :db_condition_placeholder_2)) AND (language IN (:db_condition_placeholder_3)) AND (deleted = :db_condition_placeholder_4) ORDER BY delta ASC; Array ( [:db_condition_placeholder_0] => node [:db_condition_placeholder_1] => 2 [:db_condition_placeholder_2] => 20 [:db_condition_placeholder_3] => und [:db_condition_placeholder_4] => 0 ) in field_sql_storage_field_storage_load() (line 323 of /customers/ketelpatrouille.be/ketelpatrouille.be/httpd.www/modules/field/modules/field_sql_storage/field_sql_storage.module).

Then I run update.php:

field module

* 7001 - Fix fields definitions created during the d6 to d7 upgrade path.

field_sql_storage module

* 7001 - Remove the field_config_entity_type table and store 'entity_type' strings.
* 7002 - Fix primary keys in field revision data tables.

list module

* 7001 - Rename the list field types and change 'allowed_values' format.

It fails with the following error:

Failed: DatabaseSchemaObjectDoesNotExistException: Cannot add field field_deleted_data_6.entity_type: table doesn't exist. in DatabaseSchema_mysql->addField() (line 321 of /customers/ketelpatrouille.be/ketelpatrouille.be/httpd.www/includes/database/mysql/schema.inc).

running update.php again says:

field_sql_storage module

* 7001 - Remove the field_config_entity_type table and store 'entity_type' strings.
* 7002 - Fix primary keys in field revision data tables.

But it still fails with the same error. Visiting the site keeps showing the first error and the site remains broken...

@dereine: by "reverting" I simply meant reinstating a DB backup.

What I am reporting here happens during an upgrade from RC2 to RC3, not during a reversion.

Sigh. I guess there exists an awkward timeframe during field 'bulk deletion' ops where the sql storage table has been deleted while a 'field defnition' record still exists in {field_config}.

The update should just skip tables that do not exist...
Patch attached (just wraps a code block within if (db_table_exists($table)) {)

Affected sites have failed field_sql_storage_update_7001(), the fixed version will rerun on the next update.php.

If I interpret things correctly, this change has not made it in RC4. Is this because the status has not been set to "fixed" yet?

I am willing to test if someone could point me to a tarball which I can use to test?

@saintiss ; the change itself is in RC3 and RC4 - it's only the followup patch in #89 that is still pending

@yched: I tried RC3 and my problem was still present, hence my question...

saintiss: Can you test the patch in #89 and let us know if it works for you? Then this can get fixed before final release.

@webchick: which tar.gz can I download that already includes the patch?

@saintiss : the proposed fix is not available in any downloadable archive. You need to apply the patch attached in comment #89 above.

Read http://drupal.org/patch/apply for information. For a relatively simple patch like this one, you can also manually edit the field_sql_storage.install file in modules/field/modules/field_sql_storage/ to reproduce the changes.

Tried it, and I can confirm that the patch works in combination with RC4

Marking #89 for commit.

Awesome! Thanks for your help in testing, saintiss!

Committed to HEAD. This fix will be available in Drupal 7.0. :)