Implement hybrid storage for Fields in Core

I'm mostly done implementing this, so no one else should start on it.

Eaton : could you elaborate on point 5 ? What's your definition of 'grouped together' ? Is that 'combo field' ?

I have your specs 1-4 working for read and write, though I am not yet confident enough it is correct to commit it. I hope to work on it more tonight.

My plan is to handle shared, limited-value fields in a new way: I'll write them both to per-bundle and per-field tables. So, field_attach_load() can load shared fields in the single query along with non-shared limited-value fields, but the per-field table is still available for Views or other shared uses. This requires exactly twice the storage for shared fields but zero additional queries because (a) if I weren't doing it this way, I'd still be reading/writing the per-field table and (b) assuming the bundle has at least one non-shared limited-value field, I'd be reading/writing the per-bundle table anyway.

Here is the initial core patch to enable hybrid storage. The newly defined hook_field_attach_{load,insert,update}() are implemented by the contrib pbs.module to actually do the deed.

Note that pbs.module's tests will fail unless/until #369413: Reset Field API static caches during tearDown() is fixed, but module works anyway (it's just a simpletest bug).

I should add that I am not a HUGE fan of the way I implemented this. I like that it is outside the main storage engine; I think it is much cleaner to implement the hybrid storage field completely independent code. I do not really like the signature of the new field_attach hooks. Also, this approach requires an awful lot of PHP memory wrangling which might completely eliminate the performance benefit.

Oops.

Note to self: There is a bug in pbs.module. It intentionally loads and stores shared fields in the per-bundle tables. It correctly returns the shared fields in the list of field names that the field storage module should not load (because pbs has already loaded them). However, it should NOT return shared field names in the list of fields it has saved because it does not want to prevent the field storage module from saving them to the shared location as well, since that is where Views will look for them (once the Views integration gets written).

I note that eaton's goal #5, storing "grouped" shared fields in a single table so they can be loaded/saved in a single query, can be implemented by another module using the new capabilities of hook_field_attach_{load,insert,update}. Neither the field storage module nor pbs.module needs to know anything about it.

Patch re-rolled against the current head. Eaton's left out quite some. Note that these changes only an enabler for pbs.module, so the patch does not do that much.

Hmmm webchick committed a patch re. simpletests. Let's re-test.

I just discovered a wrinkle with per-bundle storage. The semantics for field_attach_update(), as explained in field_sql_storage_field_storage_write(), are:

    // Leave the field untouched if $object comes with no $field_name property.
    // Empty the field if $object->$field_name is NULL or an empty array.

So if $object->$field_name is unset, we need to leave whatever existing data there is in the database for that field for that object. That's easy for per-field storage because we can simply leave that table alone. For per-bundle storage, we currently always delete the existing row for the object and then insert a new one.

To leave an individual field unchanged in a per-bundle table, we either would need to update each set of field columns with a separate query (killing the advantage of per-bundle storage), or else read in the row for the object before deleting it and writing the data back in the single insert query.

Hm. Exactly one of the areas that gets streamlined by PFS.

In D6, we 'delete all then insert new rows' only for multiple fields, because the number of rows affected can vary. It's OK since those fields are in their own PF table so doing so doesn't delete data for other fields.
For single fields, we do either one INSERT on node insert, or one UPDATE (limited to the actual fields we have, preserving the other ones) on node update.
So, AFAIS, PBS storage should not do 'delete all, insert' for its data, only PF can.

Now, the above (INSERT on insert, UPDATE on update) works only if there are marker rows in the PB table for every existing entity, so that the UPDATE does actually have a row to update. There were node creation / field creation / field migration scenarios in CCK that could lead to a node having no entry in a data table. So what CCK actually does for its PB tables is :

if (db_result(db_query("SELECT COUNT(*) FROM {". $table ."} WHERE vid = %d", $node->vid))) {
  content_write_record($table, $record, array('vid'));
}
else {
  content_write_record($table, $record);
}

I cannot say for sure whether these 'tricky' scenarios have disappeared in our D7 context, I'd say we're better off taking the same precautions. I *think* DBTNG has nice ways to do 'update if exists else insert' (er, isn't this a merge query ?)

Addition - AFAICT, PBS taking all fixed-cardinality fields just makes a little difference wrt the above :
when building the UPDATE query, you must pad the multiple fields with NULL values up to their cardinality, so that the previous delta 3 and above do not get preserved if the incoming field only has 2 filled-in values.

That's exactly the explanation I was hoping for, thanks.

I cleaned up pbs.module to match the behavior in#21-22 and fixed a bug in the Field API patch that enables it in the process. New patch attached.

Re-rolled.

New patch, fix the syntax error in field.api.php.

Jeff, did you make any functional changes in your patch? I need to know if I should try to integrate changes from it into my working directory.

All the Field API tests and pbs.module tests passed with the patch I attached when I attached it. Perhaps something else changed since I made the patch which is causing it to break.

Yes, that's correct: the field tests will not pass if pbs.module is enabled because they look at the storage tables directly. I should move the tests that look at the tables directly into field_sql_module.test, possibly replacing the field.test tests with pure API tests.

Actually, I thought having pbs.module enabled outside the test system would be irrelevant and that only the modules enabled during setUp() were actually active during the test. But in any case the field tests will have failures and exceptions if pbs.module is enabled while they are running.

New patch with a bugfix in field_attach_insert and field_attach_update.

Meant to change the status with the latest patch.

So, I have to confess I'm a little behind on this.

I'd welcome a clarification / confirmation of the final goal we're aiming at here.

1) Is pbs.module intended to stay as a separate (core or contrib) module, independent of field_storage.module ?
The code currently seems undecided on this - its .info tags it as 'required', but it has synchronisation code in its hook_install().

We have an architecture for pluggable storage engines - well, supposedly, since a) the actual 'pluggable' property relies on the ongoing work on pluggable handlers and b) no-one actually tried to write a 2nd storage engine, so we kind of lack perspective.

I'm worried that a separate pbs module, that bypasses parts of the load and save workflows from the (potentially non db) engine in charge towards its own (db) tables adds a layer of complexity on top of the existing concepts ('storage engine'), and makes the storage schema and actual location of the data hard to grok ('ok, first, do you have pbs enabled ?').

If we come to a consensus on a storage strategy for pbs.module, then shouldn't we 'just' move it to field_storage_sql.module ? Maybe that's the plan already ?

2) Are we settled on PB tables as primary storage vs secondary (duplicate) storage ?
The current pbs.module and the patch above ($skip_fields) seem to go for the former. The FiC sprint favored the latter, and I'm still personally pretty inclined to it. Your data *never* moves, it's reorganized depending on your actions.
I don't look forward to rehashing the issue yet another time if everyone else is sold, it's just that looking at some code that handles primary bundle tables make me sick in anticipation :-/

re #33 / #34 - it might very well be pbs.module currently being 'required' that messes with tests. I had troubles a fresh drupal install with just caused by pbs module being present in sites/all/modules. Went smoothly after I fixed its .info file to make it not required.

re #37:

This patch adds the capability to field_attach_load/save for modules to duplicate or completely take over the storage of some or all fields, regardless of the field storage engine in use, or in conjunction with the field storage engine in use, or really using whatever strategy they want. Duplicating storage of fields is already possible using the field_attach hooks, and this patch does not change that. This patch makes taking over storage possible by allowing modules to tell the Field Attach API that certain fields should not even be passed to the field storage engine for load and save operations.

pbs.module uses this capability to provide hybrid storage. Eaton proposed another possible use case: group field storage, to improve the efficiency of multiple multiple-value fields that are used in lock step (e.g. an "address" that has multiple sub-fields, and allows multiple addresses per object). Yet another use case could be a module that stores all large blob fields in S3 or something.

The fact that our current field storage engine is per-field with pbs.module in contrib is incidental to this patch. Someone (not me!) could choose to write an integrated hybrid field storage engine and we could choose to make that the default (thus making pbs.module obsolete). But this patch would still be relevant, and that new field storage engine would simply have to support the new form of hook_field_storage_load/save proposed by this patch (basically, it has to accept a list of fields to skip).

reroll

I changed the hook_field_attach_{load,insert,update} signatures to reduce the amount of PHP data slinging, but the patch is basically unchanged.

This patch is lingering on and on. Please, someone, mark it RTBC...

Patch missing ?

Ooops.

Sorry to be a pain.
I'm still worried that it acts *above* the storage engine. As #392494: Fields API needs to allow searching of scalars showed, abstracting out storage to storage engines had a nasty side-effect of black boxing the data. If we add another layer which can take storage in and out of the hands of the stuff that's supposed to handle it (the storage engine), it becomes unclear how the proposed field_attach_query() works.

field_attach_query(), when implemented, will work just like field_attach_load(). It will pass the query to any module that implements hook_field_attach_query(), then pass it to hook_field_storage_query() with a list of fields to skip generated by hook_field_attach_query() implementations. At least, that's what I'm thinking, but I haven't written it yet.

The alternative to this patch is that we make field storage not pluggable any more, and only offer a single hard-coded field storage engine. Sure, we could skip this patch and leave the basic field storage pluggable, but then non-field modules still would not know where the data was stored and could not query SQL directly anyway, so nothing would be gained. And we've already identified several powerful use-cases for the hooks in this patch:

* Per-bundle storage.
* Per-combo-field storage for things like addresses.
* Storing individual fields in alternate locations; e.g. "store all Image field files in a CDN."
* Possibly, helping with field conversion. I haven't thought through this yet, but I'm wondering if eaton's idea would make it possible to convert even large data sets from one field storage mechanism to another incrementally (like search indexing) without taking the site down, only switching to reading from the new storage location when all conversion is finished.

OK, works for me.

bot #7 had a hiccup with gd. it's disabled now.

Given this patch, how important is it that the storage engine is swappable? It sounds like we just defined a much more powerful API that, at least partially, deprecated the previous API? I was hoping the API documentation would help me answer this question, but it didn't. (I need to spend more time looking into this patch so I might be off.)

My sense is that both are useful. The hooks added by this patch let modules override, mirror, shard, or otherwise store a subset of fields in some other way. The field storage module then handles everything that's left. It seems like a handy system to me, and the changes are pretty simple.

Anyway, the patch no longer applies. I'll re-roll it when I can.

Re-rolled, tests passed, code unchanged since eaton marked it RTBC.

Nice.

Please don't blame me, but this

  * Act on field_attach_load.
- * 
- * This hook is invoked after the field module has performed the operation.
  *
- * See field_attach_load() for details and arguments.
- * TODO: Currently, this hook only accepts a single object a time.
+ * This hook allows modules to load data before the Field Storage API,
+ * preventing the field storage module from doing so.

looks like it would conflict with the hook's usage in #367595: Translatable fields?

AFAIK, all of the current _load hooks in Drupal are invoked after the base object has been loaded. This seems to do the opposite. So, maybe a hook_field_attach_pre_load() or hook_field_attach_storage_load() or similar is in order?

Yeah it looks like the actual _load() hook has essentially been removed here.

While we're here, this is just c&ped in the patch, but could fixed anyway:

+  // Fetch avaliable objects from cache.

I agree that the hijacking of hook_field_attach_load() is unfortunate. field_attach_[op] / hook_field_[op] / hook_field_attach_[op] have a fairly consistent relationship across ops, we should keep it that way and create a new hook.

Marking CNW, I also agree that a new hook name sounds better (as opposed to putting hook_field_load() back and renaming that).

No. We are not bike-sheding this critical issue based on hook names or on requirements for other issues (like translatable fields) which do not even have a viable proposed implementation regardless of this patch.

Yes, this patch changes the semantics of hook_field_attach_load, _insert, and _update, but there is no existing code that uses them and arguably they are not even presently correct/useful as committed. For example, in a previous conversation with chx we discussed that the post-load "you can change the loaded field data now" operation should actually be drupal_alter(). That may or may not be right, but is not for this issue.

I'm all for adding whatever hooks are necessary. In a separate issue.

Well I disagree. It's very easy to add new hooks rather than re-purposing existing ones, even if the existing ones are a bit buggy. Simply removing API functions with no route to replace them, in a completely unrelated issue, with absolutely no discussion of this decision in the first 50 followups makes no sense whatsoever.

We have _load() hooks all over the place in core:

hook_load()
hook_node_load()
hook_taxonomy_term_load()
hook_taxonomy_vocabulary_load()
hook_user_load()
hook_file_load()
hook_comment_load()
etc.

All of these allow you to add data to the object returned from the core tables, hook_field_load() is exactly the same in HEAD, even if it's a bit buggy as it stands. Removing it would be a regression.

If you want to force this issue through without proper review, go ahead and try, but there are only six names on this issue, and three have expressed concerns with the hook re-purposing, while two didn't express an opinion, leaving you in a minority of one at the moment as the patch author. Hardly a bikeshed.

I've not followed this issue that closely (as opposed to the discussions around it), and apologise for coming in late - but changing hook_field_load() is precisely the kind of thing which needs to happen in another patch, not this one.

This version puts back the old hook_field_attach_load() and renames the created-by-this-patch hook to hook_field_attach_pre_load(). It also renames hook_field_attach_{insert,update} to hook_field_attach_pre_{insert,update}, which the previous patches already moved to be before the field storage hooks. There is no post-storage insert/update hook. If we come up with a use case later, we can add it later.

New naming is fine with me. I can't think of use cases for the post-storage insert/update either - and not renaming existing hooks means it's a trivial change to add them back rather than having to shuffle everything around a second time.

Thanks Barry.

Re-rolled; the conflict came from changes to a test file that were in this patch and another that got committed earlier today.

The first glaring problem I see here is that very few of the hooks in field.api.php have example code. Getting this in place should be a very high priority, as it's a huge barrier to helping new developers grasp the Field API and its capabilities. Fixing this throughout the file is out of scope for this patch, but at the very least, I'd like to see hook_field_attach_pre_load() have example code, since it's added here. (can this be copy/pasted from pbs.module?)

+ * @param $skip_fields
+ *   An array of whose keys are field names whose data has already
+ *   been loaded and therefore should not be loaded again.

That sentence is very awkward. We discussed in #drupal and bangpoud came up with "An array keyed by names of fields whose data has already been loaded and therefore should not be loaded again." Sounds good to me! Could you change all instances of this param to this?

Dries's comment at #52 hasn't been answered with respect to the API documentation. It's documented in the comments here in this issue, but not in the docs that everyone who is NOT involved in this issue will actually read. ;) Seems like this could be done at the spot where http://api.drupal.org/api/group/field_attach/7 is generated, which is where all of these hooks are currently grouped. The only context people currently get is:

+ * This hook allows modules to load data before the Field Storage API,
+ * optionally preventing the field storage module from doing so.

This doesn't explain why I would might want to invoke this hook in my module. This should also be explained (several great examples were given in this issue's comments.)

Code-wise the rest of the patch looks fine, although we're introducing some whitespace in field.info.inc and there seems to be an unrelated hunk in field.crud.inc that moves module_invoke_all('field_create_instance', $instance); after field_cache_clear(); (why is that, btw?)

Another area I ran into confusion is why there are two nearly identical loops in _field_attach_load(); one to loop through $additions, and one to loop through $loaded. So a comment above those two loops that explains why they work that way would be a good thing; it's inconsistent with how the insert/update functions look.

Also:

+    // hook_field_storage_load() and hook_field_attach_pre_load() must not
+    // alter objects directly but return their additions.

Why? (the old code said this was to allow for caching; the new code removes explanation)

I was around in IRC when webchick reviewed and started to account for her remarks.

- modified the 'An array of whose keys... ' sentences.
- I don't think I get where you would like to see the explanations about use cases for the hook_field_attach_pre_load(). For now I added a few lines in the PHPdoc for the hook() in field.api.php.
- renamed the $loaded variable and added more code comments inside _field_attach_load()
- Fixed a few PHPdoc glitches along the way, and removed the PHPdocs from hook implementations in field_sql_storage.module while I was in the area. The PHPdocs are in field.api.php.

Still open :

- I added code from pbs in hook_field_attach_pre_load() 's sample code. This code is, however, rather complex and I doubt it makes much sense taken out of pbs. Also, it has calls to other pbs_* functions, which I'm not sure is inline with the rest of core's hooks docs.

- do we need hook_field_attach_pre_load() to take and return $additions_pre_load (formerly $loaded) by ref ? This is not consistent with the other load-related hooks that are part of the load workflow.

- Dunno about the module_invoke_all('field_create_instance', $instance); change in field.crud.inc, and whether it's related. I'd think we want to clear the cache *after* we let 3rd party module have their say ?

Leaving to 'needs work', at least for the last point.

Same patch, without the many trailing whitespace fixes that my IDE automatically adds...

I modified yched's patch in #70 in two ways:

* I added some documentation to appear at http://api.drupal.org/api/group/field_attach/7 to explain the motivation for the pre-operation hooks, as Angie requested.

* I *removed* the pbs module code from hook_field_attach_pre_load() in field.api.inc. The code is too confusing as is to be used as an example, especially removed from its support code in pbs module and the corresponding code in pre_insert() and pre_update(). What we need is a suitable example showing all three hooks working together, perhaps with psuedo-code showing "store all image fields in a cloud database."

there seems to be an unrelated hunk in field.crud.inc that moves module_invoke_all('field_create_instance', $instance); after field_cache_clear(); (why is that, btw?)

When someone calls field_create_instance() an instance gets created. If a module implements hook_field_create_instance(), that hook should be entitled to assume that the Field Info API will return accurate information including about the instance that was just created. For that to be true, the cache must already have been cleared. The general motto is: Outside the Field API, the existence of any internal caching must be invisible. I added a comment to that effect.

So, the current code contains a bug in which hook_field_create_instance() is called too soon, and in fact pbs.module broke as a result. Hence, I included the fix here.

No code has changed in this patch since it was RTBC, only comments. I would really like to see the patch committed without being help up for the as-yet-unspecified example code for the pre-operation hooks, so I am setting this back to RTBC. If Angie or Dries thinks this is unacceptable, they can set it back to CNW.

Awesome. Thanks a lot, Barry! The documentation looks much improved, and captures much better the discussion in this issue. While I'm not crazy about the idea of committing a hook without example documentation (too much of that has happened this release... :\) there is logic to addressing all of these missing examples at once in a separate patch. And this patch is very important for moving other field API efforts forward.

Committed to HEAD! Thanks for all the great work, folks!

(deleted, me just being plain blind). sorry