data records not deleted when a field instance is removed

I wonder if this is really the same thing as http://drupal.org/node/89351. In that case, though, it looked like the field value got deleted, but not the value in the content table. Looks like we need to do some investigation of what is happening on deletions....

I've been digging into this and am beginning to see what is happening, but still working on the best way to fix it.

The problem I'm looking at affects shared fields that live in their own tables. When you remove a field instance, none of the values for that content type are removed from the shared table, which ends up with invalid values in it. When you remove the next to last instance, the storage type changes from per-field back to per-content type and *all* values in that shared field table are copied to the surviving per-content table and the shared table is dropped. Now you have lots of invalid values in the surviving content type table, possibly values that originated in several other content types. If you happen to add the original field back to all the original content types it was in, all those invalid values get moved back to the per-field table and all is well again. If not, well, you have a mess. Clearly we cannot even pretend this is a 'feature', it is definitely a bug that needs to be fixed.

I'm still trying to figure out if there are similar problems when you delete a field from a per-content type table, but I don't think so. In that case the unused columns are dropped, which deletes their values.

BTW, this behavior is the same in both HEAD and 4.7, so it has nothing to do with the 5.x changes.

Here is a patch for 4.7. Initial testing says it may do the job.

@yched, I think you have a point about allowing modules to react to the deletion, I hadn't thought about that. I'm worried about execution time, though. I have instances where there are thousands of nodes for a content type. There is a routine you can use in install files that allows you to do updates in stages so they don't time out. We would need something like that. I wonder if we can include install.inc and use that multi-part routine somehow (see http://drupal.org/node/51220), or at least dig into the code and borrow the method.

I don't have time to work on this right now. Does anyone else have time to try to get something like this figured out??

Doug provided a patch to make sure the field tables are dropped when there are no more instances at http://drupal.org/node/181144. That is a part of the solution of cleaning things up.

I'm working on the D6 port right now which is uncovering another related issue -- what happens when a field module is disabled or uninstalled? In D5, there's no way to clean its data out because we don't have a way to know when that happens. In D6 we're adding a content_notify() function so we'll know when that happens, and we now have a batch update function we can use to clean data out, but there are other issues.

One question is *when* should we remove the data? When you remove a field using the field settings, you will lose your data for data stored in content tables because the columns will be removed, so that's a given. But we need to do the same thing for data stored in field tables that still have good data in them from other content types. (And we need to be sure to drop the field table, too, once it's no longer being used, as Doug points out). But we don't always warn people when they're doing something that will cause them to lose data, so we need to identify exactly when data is going to be lost, handle it consistently, and warn people appropriately.

And what should happen when you disable or uninstall a module? We should definitely delete the data when it's uninstalled, but we should not remove the data when it's disabled, because sometimes you need to temporarily disable modules for administration reasons like when doing upgrades from one version to another, and you don't want to lose your data when that happens. But if you come back later and uninstall the module (which has to have been disabled to even show up on the uninstall list) there's no longer any way to get any information about its database schema or fields so we can clean up after it. So we need to store information about disabled modules in the database so we can get back to it. Or maybe we need to store field and db information in the database for all modules to make clean up easier.

What a tangled web...

After lots of research and trying various solutions out, I think the best way to get this working correctly is to store the module name and column info in the field settings table. With that info available in the database, we can use the crud functions to alter and delete even fields that are disabled. I reworked the D6 port to do this, and I also added a content_module_delete() function that uses that information to clean out all field settings and data for fields created by a module that is being uninstalled.

This involved a lot of changes -- none of the database manipulation functions can use module_invoke() to get column info since that isn't reliable, so many functions needed a rewrite to ensure they were getting info directly from the stored info in the database instead.

Once the D6 port is finished, we can try to backport some of these changes to D5, but that won't happen right away.

Just to making sure, but are you talking about modules with node types at the end of #13? That particular issue came to my attention and I wanted to make sure it was on someone's radar.

@Island Usurper, not sure what you're talking about.

The work done in D6 is to fix problems when a CCK field module is uninstalled after it was used to create data which left orphan field data records that did not get removed. Now when a field module is uninstalled we can also clean up any node it attached its field to and get rid of all its field data.

I guess I'm really talking about the opposite problem, data being deleted when it shouldn't be, but I think it'll be based in the same code.

I'll make a new issue about it.

sorry related but different issue

This is active again now that we removed the batching from content_alter_schema() since we are no longer cleaning up data when we alter the schema. We still need to figure out a way to use batching to clean up field data when fields are removed so we don't end up with orphan values in the database.

I've been thinking about other ways to do this and my best idea right now is to add into the UI processing a batch step before we alter the schema to empty the values in the node that are going to get deleted, then do a node_save on each. After all the values have been emptied, then go in and alter the schema. The alter_db() processing will still drop the tables and fields it does now, but they should all contain empty values by that time. I am pretty sure this will work for UI field changes, then anyone make changes via the API will have to take care of that step themselves.

Thoughts?

That last post was not too clear. Let me try again.

Example:

A multiple value field that we're going to change to a single value.

Current data looks like:

nid vid delta field_1_value
1     1      0    Something
1     1      1    Else
1     1      2    Another

First, we create a batch that loads one node at a time and change the value to the following and save it:

nid vid delta field_1_value
1     1      0    Something
1     1      1    NULL
1     1      2    NULL

Then we do the content_db_alter() on the table, which will move the zero delta to the content table and drop the other two values. But the other two values are already empty, so that's OK.

Bumping this forward. I can't decide if we still have this issue or not.

hmm... I opened another issue that is a dup of this one: #445436: Dropping shared field from one content type leaves orphan records

markus said in that thread:

If CCK leaves the orphans, then it would probably be nice to show a warning or something. It could even show the SQL statement that needs to be executed, so admins can get it and schedule a batch process or use phpMyAdmin to run the remaining step.

Another approach that could be taken in order to fully automate this process could be have a queue of these clean up processes stored somewhere that is read by cron and processed in chunks, just a few records per cron execution until all is clean.

Could something like that be implemented in CCK for D6 or should we rely on custom/contrib code?

I've been reading all the conversation between KarenS and yched. But, I'm not sure what's really left. Is it possible to enumerate the cases where CCK leaves orphans? ...or is it just when a shared field is deleted from one type (which is the case I noticed on our installation)? Could we add code to CCK to warn the user when that happens?

If it was possible to perform automatic clean up, is it viable to introduce this in CCK, or we should rely on custom/contrib code? Now, I realize that it is not only necessary to remove records from the DB, but also trigger CCK fields to let them do their clean up.

The problem is real in D6, so which would be the best practice at this moment in time?

So we could try to introduce a new cron task to deal with this? ...such a cron would be triggered by a variable that contains all relevant information, that would be removed when the task is completed.

...or is it viable to try to introduce the use of Batch API to perform all these heavy tasks? I would assume it is too late in the D6 cycle for something like that, though. Batch API could solve the timeouts, but then affected content types would have to be locked somehow until the changes are completed.

So... is hook_cron() the way to go in D6 to deal with orphans that remain when a shared field is removed from types while it still remains using per field storage (still shared or multiple values)? If so, I may try to write a patch so it can be tested, etc.

hmm... maybe we can resolve this remaining clean up from content_field_instance_delete() as follows?

   }
+  // if more than one instance remains, we may need to remove records related
+  // to the content type where the field is being removed.
+  elseif (sizeof($instances) > 1 && $field['db_storage'] == CONTENT_DB_STORAGE_PER_FIELD) {
+    $db_info = content_database_info($field);
+    $table = $db_info['table'];
+    $vids = array();
+    $result = db_query("SELECT f.vid FROM {". $table ."} f INNER JOIN {node} n ON n.vid = f.vid WHERE n.type = '%s'", $field['type_name']);
+    while ($row = db_fetch_object($result)) {
+      $vids[] = $row->vid;
+    }
+    if (!empty($vids)) {
+      $placeholders = implode(', ', array_fill(0, count($vids), '%d'));
+      db_query("DELETE FROM {". $table ."} WHERE vid IN (". $placeholders .")", $vids);
+    }
+  }

   content_clear_type_cache(TRUE);

The patch

Oops, the process ought to be dealing with nids, rather than vids. More efficient, and the SELECT does not use filesorts.

Now, instead of build one big DELETE statement, it could be splitted into one for each (say) 10000 nids. If so, please let me know and I'll update the patch. This can easily be done while building the $nids array, for example.

Here's a code snippet that can be used to check how many orphans exist in per field tables:

function _content_get_per_field_storage_tables() {
  module_load_include('inc', 'content', 'includes/content.crud');
  $tables = array();
  $instances = content_field_instance_read(NULL, TRUE);
  foreach ($instances as $instance) {
    if ($instance['db_storage'] == CONTENT_DB_STORAGE_PER_FIELD) {
      $field_name  = $instance['field_name'];
      $table = _content_tablename($field_name, CONTENT_DB_STORAGE_PER_FIELD);
      if (!isset($tables[$table])) {
        $tables[$table] = array(
          'field_name' => $field_name,
          'types' => array(),
        );
      }
      $tables[$table]['types'][] = $instance['type_name'];
    }
  }
  return $tables;
}

function _content_get_per_field_storage_orphans(&$tables) {
  $orphans_per_table = array();
  foreach ($tables as $table => $info) {
    $nids = array();
    $placeholders = implode(', ', array_fill(0, count($info['types']), "'%s'"));
    $result = db_query("SELECT f.nid FROM {". $table ."} f INNER JOIN {node} n ON n.nid = f.nid WHERE n.type NOT IN (". $placeholders .")", $info['types']);
    while ($row = db_fetch_object($result)) {
      $nids[] = $row->nid;
    }
    $nids_count = count($nids);
    $tables[$table]['orphans'] = $nids;
    if ($nids_count > 0) {
      $orphans_per_table[$table] = $nids_count;
    }
  }
  return $orphans_per_table;
}

$tables = _content_get_per_field_storage_tables();
$orphans = _content_get_per_field_storage_orphans($tables);

ksort($orphans);
print_r($orphans);

This kind of information is quite important for our project because we'll have to deal with thousands of nodes when the site is launched, I'll be compiling for us similar snippets into a custom module, so that we can check data integrity along the time. This is not only relevant while building the site, but also it could be used to detect inconsistencies caused by bugs, crashes, etc.

However, I'm wondering how many installations out there will see a few records when the $orphans array is printed (snippet above). Maybe something like this could live as a contrib add-on? ...or maybe included in CCK itself?

hmm... how many nodes would be reasonable to deleted per cron task execution? ...CCK does not have a generic settings panel where to place a setting for this. Maybe it could be a constant on top of content.module 'CONTENT_NODES_PER_CRON_STEP' equal to something... maybe 1000? 10000?

I'll work out a patch implementing hook_cron() for this... ok? ...or do you think there's enough material here so that you can implement it yourself, polishing these fine details?