Separate revisions from node.module

I gave a little love to the update path.

subscribing

Minor: changed the system.install function to system_update_2005().

me too

Needs a complete rewrite after the module split patch. The other benefits sound very nice.

I definitely think this would provide a substantial benefit to a large number of users like me that don't have any need for node revisions on some of my sites. Will help test.

For all the reasons webchick advanced, I absolutely support this request!

This is logically the first step.
Secondly we would integrate diffing.
Third important step would be integrating Save as draft's logic into the new core node_revisions.module, which simply means: The latest revision doesn't necessarily need to be the one that is currently published.
Fourth step: Of Revision Moderation there should be only a few lines left that could be merged into modr8.
Whether we then want to have modr8 in core or not, that's an entirely different issue.

Does that sound like a good plan?

Does that sound like a good plan ?

... awesome !

... but it's probably not gonna happen this way: diffing will be included rightaway, because the other patch is close to rtbc. This means we need to wait until that one is committed and then reconsider how we want to split node.module.
So the best thing we can do right at the moment is to review the "diff in core" patch. The more reviews the better, so we can soon return to this issue here.

This makes complete sense to me, and then adding drafts and simplifying moderation are good plans too.

Oh my,

I've been working all day on a new version of this patch. As you all know, many things have changed in the meanwhile, so I had to start all over again.
But now that most of the work seemed to be done, I was hit by a logic error, which turned out to be a major problem:

If {node_revisions} has never been installed:

We can copy our {node}.nid to {node}.vid.

If {node_revisions} is installed and Node Revisioning is enabled:

We can copy the appropriate {node_revisions}.vid to {node}.vid.

If {node_revisions} is installed, but Node Revisioning has been disabled:

We can't assume the next unused {node}.vid without querying {node_revisions}.

I see the following options:

1. We start without a {node_revisions} table (so the node table needs to be self-sufficient). As soon as Node Revisioning gets installed, we copy over the existing nodes and start using both tables. Now, if Node Revisioning is being disabled, we can double-check if any node has additional revisions. If not, we can automatically uninstall {node_revisions}. But if yes:
   
   1. we reintroduce some kind of sequence variable, so we know the next vid to use in {node}.
   2. instead we could query the highest {node_revisions}.vid and the highest {node}.vid and take the max of both +1 as the next vid.

   In both cases, if later Node Revisioning gets enabled again, we need to copy over our new nodes from {node} to {node_revisions}.

2. We start without a {node_revisions} table, but once Node Revisioning has been installed we keep using the table until a full uninstall. As long as Node Revisioning is only disabled, we keep syncing {node_revisions} on every added, edited or deleted {node} entry. Should Node Revisioning be enabled again, we can just keep on working.
3. Or we keep creating the {node_revisions} table on every install (self-sufficient, though) and keep using it. Only code that is specific to manipulating multiple revisions is split out into a new module. Then we don't have to cope with this, but have the least performance gains for simple websites w/o revisioning.

Either of these ways will work. But we need to take a decision which way we want to go with this patch. Remember, this is about improving performance for small sites that don't need node revisioning. Please throw in your cents ;)

P.S. Only for the records, I enclose my latest patch. It follows the first solution, but doesn't implement a) or b) yet. At the moment this is not worth testing (so I added ".txt" to keep it from being slandered by the testbed ;-).

(1st edit: clarification on solution 2)
(2nd edit: clarification on solution 3)

Does the current patch allow enough to benchmark the difference between 1 and 3? I'd be interested to know how much difference the extra join makes on node/1 and node pages. If it's not a great deal of gain, then it would simplify things a lot to keep it as, is except for moving the code around so that's not loaded unless necessary - both option 1 and 2 are going to take quite a bit of thought and complexity to keep things synced nicely.

I've also been wondering if we could remove the join on the user table - either by denormalising name, and/or moving that into user_nodeapi and only adding the fields if authoring information / user pictures are enabled.

I clarified the alternatives in my post above. Hopefully this is also what catch thought them to be, but after the edits I can't nail him down on his opinion anymore ;-)

We are actually having a fourth alternative:

4. Finally we can leave the data structure as-is, which means that we still have to join {node-revision} on viewing a node. As in (3), only code that is specific to manipulating multiple revisions is split out into a new module.

Thanks catch for guessing with me, but I agree that we need hard facts, i.e. benchmarks. Therefore we need to materialize some (maybe two) of the four alternatives to a working patch, and then benchmark them against each other and the status quo. We can't do this for all of them - this would just be too much wasted work. To help us pick two of the four alternatives (at least for now), I did some "theoretical benchmarks". If they give us a general idea about the performance in comparison, they exactly fulfill what they are meant to.

In theory, things should be as following:

Notes:

• More is better.
• Diff and save_as_draft are already expected to be included afterwards, which should only affect the amount of loaded code. If you disagree, you need to substract 1 (for loads) resp. 0,5 (for updates) from the first two rows, except from the status quo.
• Note that the values are just freely weighed estimates, that might or might not turn out to be close to the truth. Also I'm obviously weighing loads higher than updates.

My personal opinion?
1 and 3 look promising:

• While 1 should bring us maximum performance, it is a bit tricky to implement. Especially the syncing after reenabling Node Revisioning will be quite a task - fortunately we don't need to care too much about this, just to get it benchmarked.
• 2 would be a compromise, if 1 in the end doesn't work out. Ensuring data integrity is no issue, and at least with a completely uninstalled {node_revisions} table even updates should perform pretty well.
• 3 is rather defensive and easy to implement, but should still give us good performance on loads. The slightly worse performance on updates is still acceptable.
• 4 would be a last resort, if we otherwise wouldn't get anything done. In the end it is just some code reorganization which still brings us better modularization, a less cluttered admin UI and some minor performance improvements.

Some more input on this would be great, otherwise my plans would be to start working out alternative 1 to compare its performance to the status quo. This would show how large the difference between good and evil actually is. Then it might be easier to decide on the route to go.

I reworked the last patch to reflect the renaming of tables, the introduction of node_load_multiple() and other changes in core.

However the results of the benchmarks I made are not impressive at all. I tested both / and /node/xyz with and without cache. Also I benchmarked a PHP page running 100 heavy node_load_multiple() queries (with $reset flag on).

Apache Bench shows no significant improvements in overall performance.
The Devel query log indicates an improvement around 1/3ms per node_load_multiple() query. This is virtually nothing compared to the heavy 'cache-get' queries or 'upload' (which should get a batch loader as well).

Obviously the additional join doesn't make any difference, and the performance increase of node_load_multiple() can't be further improved.

This certainly doesn't mean we should not make the {node} table independent from {node_revisions}, if we have other reasons. But - if I got it right - we should not expect a significant performance increase from that.

If somebody could repeat and verify my benchmarks (maybe on a more reliable testing server than mine), we should be ready to decide on our path and move forward.

The enclosed patch separates node_revision.module from node.module. Note that the 'Node revision' module should stay disabled, and saving nodes in the stripped node.module will also fail (because determining the vid has not yet been implemented, see above). However, loading and viewing nodes works fine, which at the moment is all we need for doing benchmarks.

Switching this to CNR, so it has at least a chance to get some review... :)

Not bad - I expected more fails... as said in #32, everything needed for benchmarking works fine.
Some more benchmarks for verification and feedback on the general plan is appreciated.

Nice work Pancho! I do see a small improvement in the benchmarks [you did] with the patch, not to mention how this will make node editing/creation less complicated for a large number of Drupal sites.

That'd be cool! Could you please post the figures and how they were measured?

Also, note that the question is not if we separate out the node revisioning code, but to which extent node.module should be independent from the {node_revision} table.
So either way we go, we're both getting the UI simplification for simple websites and opening up the possibility to add more features to node revisioning (diffing, for example).

It'd be very much worth considering splitting out the loading of revisions from node_load() - or at least doing it using hook_query_alter() or similar. node revisions, and the lowercase queries in user and taxonomy module are the main barriers to re-using code in the new multiple_load() functions - which could probably be abstracted into one or two common helper functions otherwise.

Please note that node_revisions in its current form may be something of the past thanks to the fields in core initiative. Storing the full $node object in a node_archived_revisions table or something like this could perhaps be enough, and would really simplify the handling code.

subscribe

Please keep in mind the importance of node moderation when playing with revision system. It remains a gaping hole in Core. One I sincerely hope is plugged in Drupal 7.

See: #218755: Support revisions in different states

Should the priority be optimizing performance for small sites, or optimizing performance for large sites? It seems to me that larger, more complex sites are more likely to push Drupal's performance limits and hence would see more benefit from optimization. These sites also seem more likely to make use of revisions (especially if dif and moderation are included). Simplifying things and improving performance for sites that don't use revisions is a worthy goal, but shouldn't the adoption of these changes be contingent on their not having a negative impact on performance when revisions are enabled? Just a thought.

Given we have fields in core, shouldn't Body be a separate field? Especially when it's optional anyway.

DamienMcKenna : See #372743: Body and teaser as fields

yched: thanks for referencing that, it's a related change but wasn't mentioned here previously. Cool :)

I haven't checked the status of this for a while - a useful cleanup in itself would be a node_load_revision() function to remove all the $vid special casing from node_load_multiple().

Do we still wait for #120955: Integrate Diff into Core with this?

It was linked here as RTBC last September, but I just came back from rerolling a patch last updated in December, and the discussion there left it unclear if it would happen at all.

My thoughts are to get this in, and then we can take #120955: Integrate Diff into Core further afterwards. I know that Dries has some reservations about Diff in Core, so it might seem that we will have a better win on getting the split done first, which then would make more sense in having Diff integrated to Revisions.

My thought is to postpone #120955: Integrate Diff into Core on this issue, get Revisions split into it's own module which we can then work on improving without playing around with Node module.

And another thought as to why splitting Revisions off could be worth while - long term, we could look to expand Revisions to cover more than just Nodes, possibly?

Soli Deo Gloria

Let's do this once #372743: Body and teaser as fields is in, which will make it a much smaller patch.

#372743: Body and teaser as fields is in, this shouldn't be postponed for that issue's documentation. :)

Now that it is postponed, instead of using diff, I guess a good 'auditing' system to track changes on specific fields could be a good approach. Although the output of diff could be used to view the changes, it is not focused on content auditing, well.. entity (why only content, users would require auditing also) auditing.

See also: #996696: Support revisions in Entity API. And, fago's D8 Entity API improvements - entity-property-fago branch - appears to have this synced. Attached are "egrep -r revision ./" and "egrep -r Revision ./" of the latest snapshot.

Setting back to active because #32 is from 2009.

Yay! EntityNG support revisions but probably it's still possible to decouple this functionality to separate mode.

Let's see how many tests will be broken

@andypost: Nah, that was just grep output. My bad.

For the real progress, see:
* #1723892: Support for revisions for entity save and delete operations
* #1082292: Clean up/refactor revisions

The diff issues in OP are still active.

Not really a duplicate, so reopening as active.

Now that revision storage is so deeply integrated into node storage and handling, the amount of code might not be the central argument.
However, while revision management is something many people expect from a good CMS, quite many others simply don't need it and don't want to cope with the additional complexity.

Comparing with how fine-grained modularity is, for example with field modules, it is obvious we should separate out this functionality to a separate module, even if we end up removing not much more than some UI from node.module.

So here is a patch separating the node revision UI from Node module.
Even UI separation is not quite complete, let alone the deeper API separation, yet I could already move ~800 lines of functional code and ~500 lines of testing code out of Node module.
Given the slightly lower memory footprint, his should already improve performance a bit for basic sites not using revisions. But to really make a difference much more needs to be taken into consideration.

Node (Revision) tests should pass, while some others would probably fail. Regard it as a first step or proof-of-concept.

Related issue that impacts performance massively. Even if a site does not use revisions, they get to use two field tables, one for the field and another one for revisions.
#1279440: Deeply decouple revisions from Entity API and Node module

@giorgio79:
Thanks for pointing me to that one, which I fear won't hit D8 anymore, unless all performance potentials need to be unveiled.
This one here aims for a more shallow separation of at least the revisioning UI, so it has better chances to land in D8. At the same time it would be a first step.

Some movement here #2144631: Add a revisionable key to field definitions

Seems there's no actionable battleplan here, and looks mostly as duplicate of #1279440: Deeply decouple revisions from Entity API and Node module

It'd be great to get the functionality of https://www.drupal.org/project/multiversion into 8.1.x as demoed in https://events.drupal.org/losangeles2015/sessions/we-need-revisions-and-...

Indeed, this effort is superseded by #1279440: Deeply decouple revisions from Entity API and Node module and #2083451: Reconsider the separate field revision data tables and largely irrelevant to D8 as it has evolved since then. Closing this one as a duplicate.