Put revisions in their own table

I too think the serialized approach is less than desirable, but here's an alternative. This would likely take some considerable rework in core and contrib, but the following is how we handle similar types of situations in our databases at work. It is more elegant that a separate table, and avoids the (almost exact) duplication of a table. Instead of separate tables, keep all revisions of nodes in the node table as follows:

* add field: active (0/1 or Y/N)
* add field: revision
* every revision of a node is stored in the node table; however, only one revision can be active at any given time
* nid can no longer be unique -- primary/unique key becomes (nid, active)
* any time a node is loaded, updated (without revision), etc., the active version is used.

Thoughts?

I don't necessarily think that duplicating a table's structure is _bad_. It just seems to be wasteful and a pain to maintain. (Every change to the node table is made twice...easy to do, but also easy to miss perhaps.)

As for performance, as long as nid and the active indicator are indexed, there shouldn't be any performance loss. Also, archiving an old version when making a new revision will be much simpler: just change the active indicator rather than copying an entire node to another table (and ensuring everything gets copied...again a potential maintainance issue).

To be honest, I would just like to see the serialized data go away, regarless of what approach is taken.

I'm interested in using Drupal for a large scale wiki-type project. In order to do this, I need revisions to be in their own table.

Attached is a patch to do just that. Most of the changes are pretty self explanatory. Spreading out node data across two tables meant that I had to add database functions to do locking/transactions. Without this, race conditions in which the database becomes corrupted are possible.

Oh yeah... The patch is a diff against Drupal CVS

I think this is the proper way to do things. No columns are duplicated, there is no serialized data, and only the fields that are logically revised are stored. Nothing jumped out at me as a way to have my node modules be able to keep a table of revisions of additional fields. I'm guessing this could be done within the confines of _insert and _update.

Assuming the upgrade path works and modules can extend it I give it a +1.

It figures that just as I finish a big patch, another patch comes along and breaks it. Oh well, it should be a pretty easy to fix. I'll work on it.

Fixing the upgrade path to keep revisions should be fairly painless.

I found another issue that needs to be fixed before this patch gets merged. There format of a node needs to be stored for each revision. Otherwise, for modules that store a format for the nodes, such as page and book, if you write one revision in PHP and the next in HTML, the PHP revision will be displayed as HTML. This is part of a larger issue of how node modules should store revisions of additional fields. I think each module that wants to do this should create another table with (nid, revid) as the primary key. Just as when they want to add fields to a node they create another table with nid as the primary key.

As far as performance goes, for sites that make heavy use of revisions, an extra join on primary keys is going to be a lot faster than grabbing all of the revisions from that database everytime. We would need to run benchmarks to determine is the overall difference in speed is for an average site is a gain or a loss. I'm guessing it's very minor either way.

Here's an updated patch against CVS that puts revisions in their own table, provides an upgrade path, and fixes the format related bugs in the last patch.

Hopefully, this can make it into CVS as soon as the freeze is over.

Interesting patch ... drumm's question is still outstanding. how do modules store revisions of their fields? Are they expected to manage this on their own? Thats not how it works today.

As an aside, i am seeing profile_ fields in my node.revisions column. One could argue that those need not be saved. They pertain to the node author, not to the node itself.

Having modules be responsible for storing revisions of their own fields is a side-effect of storing revision data in tables. There's really no way around it. However, revisions generally don't make sense for node types that don't have PHP/HTML content, such as polls. I think it's going to be a pretty rare scenario for a new node type to want another field to change per-revision, so it's a pretty good trade-off.

Storing fields that shouldn't be part of revisions, such as the profile_ fields, is a side-effect of storing revisions as serialized objects. Applying this patch will free up that wasted space. :)

I agree, there should be an API change to make specifying support for revisions easier. In the interests of keeping patches small and keeping to one change per patch, I think the API change should be a separate issue.

A sort of ad-hoc API to decide whether or not a module supports revisions by default already exists. Instead of having a hook, modules set the default value of the "Create new revision" field in the edit form. The admin can change this option in admin/node/configure/defaults. This patch doesn't change that.

Revisions are broken for node types that have their own database structure, like polls, even when storing them as serialized objects. This patch doesn't change that, either.

I'm guessing that someone is going to have to demonstrate that this patch performs as well as current drupal before it gets comitted. i think this patch is a few benchmarks from being comitted.

I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance.

I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second.

The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/

An updated patch that should apply to CVS is attached.

I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance.

I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second.

The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/

An updated patch that should apply to CVS is attached.

I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance.

I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second.

The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/

An updated patch that should apply to CVS is attached.

please overthink the revision system default workflow as well. don't look at the revision system as an isolated system but as a part of the whole workflow system!

if you combine revisions with the moderation queue the most logic default workflow would be like that:

auth user creates node (revision #0)
admin approves the node (status = 1, moderation = 0)
=> node publicly available
auth user finds typo and changes node (revision #1, status = 0, moderation = 1)
-------
what happens at that point at the moment is, that the node is not accessible anymore at all until the new revision is approved by admin. of course the new revision should not go online until reviewed and approved, this is absolutely correct, but there is no reason to not take the old revision offline, since it was already approved and should therefore be online until the new revision is approved. it is not practical if a node disappears only because the author corrected a typo.
-------
admin approves the node (status = 1, moderation = 0)

eventhough I first thought a plain boolean active field would not be capable of providing that functionality if finally came to the conclusion, that it can. The only thing to do is to not set that bit, when a new revision is created, but when it is approved (in case moderation is activated under default workflow). Every revision should have its own moderation, status and active field and on approval they are set like this (status=1, moderation=0, active=Y).

When you wanna rollback to an old revision, you can chose between all revisions that already have the moderation bit set back to 0 again and the published to 1. There should be an extra permission for rollback!

another concern that I have about the default workflow is, that users can't see the content, they have just created, when moderation is enabled. Eventhough, there is a big fat "submission accepted" presented after submissions, unexperienced users tend to question the information those stupid tincans give them, if they can't find their content afterwards. Many users are really lazy bastards and they don't even read the status messages. The best feedback about whether his story was submitted successfully or not of course is, if he can find the story somewhere on the site, maybe with a status message on top of it, mentioning, that the content is currently not publicly available since it has not been approved yet. there should be a my content section under my account, like somebody is trying to do with the workspace module I guess.

so my suggestion is to make (status=0, moderation=1) still available for the creator under a my content section somewhere!

I agree. The current workflow for moderation queues and revisions needs to change, but this patch isn't the place for it. The patch is already too big, and it only does the backend stuff.

Instead of adding more to this patch and making it take even longer to get into core, would you mind creating a new issue for your UI suggestions, so the those changes can be added as a separate patch?

Thanks,
Nick

This patch is _much_ needed so I'd love to see someone revive it. In order for this patch to be accepted, the following needs to be done:

1. Update this patch to CVS HEAD.
2. Rename revid to vid.
3. Rename node_rev to node_revisions.
4. Rename node_rev.changed to node_revisions.timestamp.
5. Rename $rnode to $revision.
6. Fix the coding style to match Drupal's: proper spacing, single quotes where possible, proper variable names.
7. Benchmark this patch with a large database with enough revisions. I'd be happy to benchmark this on my local copy of the drupal.org database.
8. The book.log field should probably move to the node_revisions table. This can be done in a separate patch.
9. Investigate whether transactions are well-supported.

Gerhard: your patch does not apply.

Some more comments:

• db_begin_transaction() and db_end_transaction() do not belong in database.inc, but in database.mysql.inc and database.pgsql.inc respectively.
• The node module calls node_revisionsision_list() which is not defined. (Fxed that on my local copy.)
• Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's table locking patch?
• The upgrade path assigns the wrong user ID to each revision.
• The upgrade path assigns the wrong date to each revision (that or a node's revision page shows the wrong usernames/dates).
• The coding style needs a bit of work, but we can worry about that later.

If you need any help getting those things fixed, just let me know.

How this relates to Jeremy's node locking patch:

There was lots of discussion, and node locking was decided against because from an end user point of view you never want a node to be locked. He's now advocating for a much simpler patch that warns users if their changes will overwrite someone elses. That patch still has a race condition, which might be fixed using db_begin_transaction().

http://drupal.org/node/6025

It didn't fix the aforementioned bugs. See attached screenshot.

Anyone to help review/test this?

Submitting book pages doesn't work on your test site. It puts the entire content of the preview inside the body textarea. I wrote a sentence in the body and the log, and pressing preview put several lines of HTML containing both sentences in the body textarea on the preview page, plus the book page wouldn't submit.

-R

0 results here. I started using Drupal with version 4.4, though.

No results here on the query:

select nid,type from node where type like '%/%';

On a database that was put in place prior to Drupal 4 and is now running on 4.5.2.

By the way, I just remembered that Drupal.org has some blogs lingering on in the database even though blog.module is not enabled. Perhaps this is causing troubles?

Hi - I posted comment #62. The changes to node_save appear to be needed because recent patches (both 21 and 22) remove the field 'title' from table 'node'. So without the changes to node_save, node.module is broken and generates errors.

Joshua

since HEAD is open again, perhaps it is a good time to revisit this patch.

once this is committed, lets address - http://drupal.org/node/11071 "node_validate does not respect group editing"

I'll commit this patch later this week! If you haven't checked this patch already, I urge you to test/check it out because it will have significant impact on existing code and modules!

Also, what do people think about the n.title being duplicated?

I won't lose any sleep because of duplicated titles...

That's a big patch. I've only started looking through it. I noticed one little typo, affecting updates. A patch to your last patch is attached.

I'm running with the revision patch on my dev server now happily. I like the concept.

What happens if you click 'stop' on your browser in the middle of a MySQL "transaction"? I assume that kills the connection to MySQL, and the lock is freed? But this then leaves changes only partially applied?

What exactly does locking/unlocking the tables buy us in MySQL? I don't see anywhere that we detect if an apply fails part way through, and thus roll back...? What am I missing?

Jeremy: many of us are worried about the performance ramifications this patch introduces. Early experiments showed a small performance improvement (while a performance regression might be expected). More performance reports from large sites like kerneltrap.org will certainly help this patch. Mind to do a quick performance comparision and to report back with some numbers? Thanks.

Dries: I'm not running HEAD on kerneltrap, so this really isn't a possibility. Furthermore, until I understand why we're locking tables, I don't like it. The idea of revisions in their own tables is great. The idea of locking tables to get (without any obvious benefit) there really worries me.

Got one error during the upgrade path:

ALTER TABLE {book} ADD PRIMARY KEY vid (vid)
FAILED

Actually, I got 2850 errors during the upgrade.

Some of these:
sprintf() [function.sprintf]: Too few arguments in drupal-cvs/includes/database.inc on line 154.

Some of these:
Query was empty query: in drupal-cvs/includes/database.mysql.inc on line 66.

And this:
Unknown table 'n' in field list query: SELECT n.nid, n.vid FROM node INNER JOIN files f ON n.nid = f.nid in drupal-cvs/includes/database.mysql.inc on line 66.

:-)

Or this:

user error: Unknown column 'log' in 'field list'
query: SELECT parent, weight, log FROM book WHERE nid = 1 in drupal-cvs/includes/database.mysql.inc on line 66

The time required to generate my main page went from 902 ms (before upgrade) to 2139 ms (after upgrade).

The time required to generate a forum listing (?q=forum/x) went from 1872 ms (before upgrade) to 2874 ms (after upgrade).

Maybe this is because my database is not consistent as the result of the upgrade errors (yet I don't see any errors on the pages I benchmarked).

I double-checked and the numbers don't seem to lie. I'll test some more after work on another machine to make sure it is not platform-specific.

I did another round of tests on _another_ machine and it is not looking good:

                              Before upgrade        After upgrade

?q= (main page)               218 ms/request         340 ms/request
?q=forum (forum overview)     754 ms/request        1520 ms/request
?q=about (book page)          375 ms/request        5400 ms/request

The upgrade process itself gave me a number of 'query was empty' and 'sprintf(): too few arguments' reports. Everything seems to work fine though.

Looking at the ?q=about page, I see that the following query is executed twice _and_ that each time, it take more than 2 seconds to complete:

SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY b.weight, n.title; 
--8<-- snip --8<---
351 rows in set (2.39 sec)

If you investigate this some more, you see that there is no index on vid?

EXPLAIN SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY b.weight, n.title;
+-------+------+--------------------------------+------+---------+------+-------+---------------------------------+
| table | type | possible_keys                  | key  | key_len | ref  | rows  | Extra                           |
+-------+------+--------------------------------+------+---------+------+-------+---------------------------------+
| b     | ALL  | NULL                           | NULL |    NULL | NULL |   369 | Using temporary; Using filesort |
| n     | ALL  | node_moderate,node_status_type | NULL |    NULL | NULL | 15294 | Using where                     |
+-------+------+--------------------------------+------+---------+------+-------+---------------------------------+

mysql> SHOW INDEX FROM node;
+-------+------------+---------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| Table | Non_unique | Key_name            | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment |
+-------+------------+---------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| node  |          0 | PRIMARY             |            1 | nid         | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | uid                 |            1 | uid         | A         |        4078 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_title_type     |            1 | title       | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_title_type     |            2 | type        | A         |       20392 |        4 | NULL   |      | BTREE      |         |
| node  |          1 | node_moderate       |            1 | moderate    | A         |           2 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_promote_status |            1 | promote     | A         |           2 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_promote_status |            2 | status      | A         |           4 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_type           |            1 | type        | A         |           8 |       10 | NULL   |      | BTREE      |         |
| node  |          1 | created             |            1 | created     | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_created        |            1 | created     | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_changed        |            1 | changed     | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_status_type    |            1 | status      | A         |           2 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_status_type    |            2 | type        | A         |          13 |     NULL | NULL   |      | BTREE      |         |
| node  |          1 | node_status_type    |            3 | nid         | A         |       20392 |     NULL | NULL   |      | BTREE      |         |
+-------+------------+---------------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
14 rows in set (0.00 sec)

mysql> SHOW INDEX FROM book;
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| Table | Non_unique | Key_name    | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| book  |          1 | book_parent |            1 | parent      | A         |          92 |     NULL | NULL   |      | BTREE      |         |
| book  |          1 | nid         |            1 | nid         | A         |         369 |     NULL | NULL   |      | BTREE      |         |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
2 rows in set (0.00 sec)

The book module does not appear to have a primary key? Sounds like a bad idea so I added one:

mysql> ALTER TABLE book ADD PRIMARY KEY nid (nid);
Query OK, 369 rows affected (0.02 sec)
Records: 369  Duplicates: 0  Warnings: 0

Next, I wanted to make the vid column a unique key in all node tables:

mysql> ALTER TABLE node ADD UNIQUE vid (vid);
Query OK, 20392 rows affected (0.81 sec)
Records: 20392  Duplicates: 0  Warnings: 0

mysql> ALTER TABLE book ADD UNIQUE vid (vid);
ERROR 1062: Duplicate entry '0' for key 2

mysql> ALTER TABLE forum ADD UNIQUE vid (vid);
Query OK, 10806 rows affected (0.10 sec)
Records: 10806  Duplicates: 0  Warnings: 0

As you can see, it fails for the book table which makes me believe there is some inconsistent data ... I set out to fix that:

mysql> SELECT nid, COUNT(nid) AS vids FROM book GROUP BY vid HAVING vids > 1;
+-----+------+
| nid | vids |
+-----+------+
| 871 |    2 |
+-----+------+
1 row in set (0.00 sec)

mysql> SELECT title FROM node WHERE nid = 871;
Empty set (0.00 sec)

mysql> DELETE FROM book WHERE nid = 871;
Query OK, 1 row affected (0.00 sec)

mysql> ALTER TABLE book ADD UNIQUE vid (vid);
Query OK, 368 rows affected (0.01 sec)
Records: 368  Duplicates: 0  Warnings: 0

Looks like everything is well now. Ran some new benchmarks:

                            Before upgrade      After upgrade      With indices
?q= (main page)             218 ms/request       340 ms/request     336 ms/request
?q=forum (forum overview)   754 ms/request      1520 ms/request    1531 ms/request
?q=about (book page)        375 ms/request      5400 ms/request     475 ms/request

Unfortunately, we're still slower than the original code.

It wouldn't hurt if more people would benchmark this patch. The patch's current performance worries me.

Did you check your watchdog messages after upgrading the drupal.org database? Depending on your settings, errors might only be shown in the watchdog. I'll look into the remaining glitches as time permits.

Thanks for your persistence in keeping this patch up-to-date. :)

Killes:

I'm also interested in seeing this hit core. What about adding something to legacy.module to do it?

This is a sensible approach. Maybe this is the _only_ sensible approach.

I have a little problem though: while the conversion is running somehow both revision handlers should be available.

can we please accept and commit this patch? We can iron out any issues later. This patch is just far too big and complex to be perfect-at-once.

@Ber: no, we can't commit this patch blindly.

Data loss is a much bigger problem than a syntax error or other code glitch. We can break Drupal but we can't break their data.

I've spent quite a bit of time testing the previous version of this patch and noticed significant performance degradation.

Tell me, Ber, why can't you test this patch first?

I was not referring to not testing it. If it is just the upgrade path that proves to be cumbersome, then why can we not fix it afterwards, I.e. when everyone has had a good chance to look at it.

Testing such a huge patch requires a lot of work. something no-one just does in his spare hours. We had discussions before to deal in a slightly different way with big changes; to commit them quicker and to leave the ironing out of any left overs for the community. I hooked into that discussion here. For two reasons. One is that Gerhard has spent numerous hours on maintaining this patch. The other one is that the community can be of help with ironing out issues in such a large change, much better than that Gerhard can do on his own.
And yes, dataloss is very bad, but no-one should loose data, when he/she followed the instructions (backing up)...

Ber: applying this patch takes 15 seconds. Whether I apply this patch for you, or whether you apply it yourself, it will hardly reduce the 'testing cost'. The problem is that data loss can be subtle; it might go unnoticed for a couple days. Make no mistake, I'd like to see this patch committed ASAP, but it warrants some testing. Let's test/benchmark it and commit it.

The upgrade script already takes a long time to execute and does not provide feedback to the user about how it is doing. I plan on making the upgrade script able to spread the updates across multiple page views and give user feedback showing that progress is being made. This will hopefully make the speed of this update a moot point.

I am working on this for my dayjob, CivicSpace, so it should get done "real soon now", but it should be expected to take awhile (smallish number of weeks). Please make comprimises if necessary.

Even if the upgrade issues are resolved, we have to figure out why this patch is slowing down drupal (according to benchmarks posted here).

We can't proceed without some additional benchmarking. I already benchmarked it on two machines (see comments above), and I'll benchmark it again after August 10. The performance impact of this patch worries me so if two other people could test and benchmark this patch extensively, that would come a long way. August 10 is close to the code freeze so some help is necessary.

If Gerhard/killes reviewed/tested one of your patches, it is time to return the favor ...

After looking at the patch I'm not really surprised this slows down everything. I thought that the reason why we wanted revisions in their own table was in the first place to have a simpler -and faster- node table. But this patch:
- Adds fields and complexity not only to the node table but also to all the main tables for data
- Needs additional joins just to retrieve single nodes (which is done many times for a typical main page)
- Does away with encapsulation of the revision functionality requiring other modules to handle revissions related data.

So please, please, please:

- Make revisions table only needed when actually using revisions
- Keep that old nice thing of hiding revisions system from other modules
- And what's so bad with serializing data anyway? I agree that usually there are better ways to store data. But this is one of the cases where serialization does make sense. Not the current huge serialized field, but only a new table with one serialized node per row.
- In general, do it much simpler. This is way too complex and it removes more functionality than it adds.

In just four words: keep it simple, please.

It has been exactly one year since I first submitted this patch.

You know what that means: old patch party in IRC! Virtual beers on me!

Hi again.
I had a talk with killes -I have to say he enlightned me about this- and also did some testing. Sorry but I dont have a good set up to provide benchmarks.

Here are my conclusions -and also some explanation for the varied results abt performance:

- This is faster for simple node listings
- This is slower for full node/load
....thus the performance increase/decrease for a given page should depend on the relation between the number of simple node listings (usually blocks) and the number of nodes displayed in the main page.

I've also experienced some trouble with attachments -not re-created when creating a new revision- and book relations, same.

I'm not sure I like this new "feature" of creating a new revision every time you rollback an old one -in Drupal 4.6 this seems not to happen.

Besides these details, I think this patch is good enough -and quite a powerful approach actually- though I still feel it's bit too complex when simply dealing with nodes -without revisions-, which is what we do most of the time, and complexity will be even higher with contrib modules using still more tables, not to mention flexinode -versioning info will have to be scattered everywhere in the db.

So this is not a +1 nor -1. On one side I think this is powerful. On the other side I feel this is too complex and really, it's more than I need.

I would be happy with a 'revision' table having: nid, vid, data(serialized), that you can forget about when not using revisions.

On a much more serious note that my last comment, unless I'm missing something, you've introduced a subtle bug into this patch since I left it.

A and B are both updating revision 5 of a node.

A calls db_next_id() and gets a revision id of 6
B calls db_next_id() and gets a revision id of 7
B updates the node's revision to 7
B adds revision 7 to the revision table
A updates the node's revision to 6
A adds revision 6 to the revision table

Now, you have a situation where the current revision (6) isn't actually the last revision(7). For some uses, that's not a big deal, but in the case of wikis, which is what I originally hoped to use this patch for, it creates problems.

Sorry, I dont know what I did the other day but I cannot reproduce it either :-)

I applied this new patch. Problems I'm having now (Testing with "stories")

- Book outline is gone after creating a new revision
- I cannot delete attachments (not sure it is related to your patch or to something else)
- Usage of the log message: Is that mensage intended for revisions or only for book outlines? Currently the field is displayed only for outlines but shown in the revision tab

Btw, the db update failed for me in patch 32, so I did it manually. I didnt do a db update again after rolling back v32 and applying v33 -should I?

I could apply the patch on my localhost, and did not seen any errors nor disfunctioning drupal features while creating, revising, editing or deleting nodes..

So, +1 for this patch qua implementation it makes sense. Unfortunatly I chaven't got a clue howto benchmark, so that's for another person.

@Gerhard:
I really think you are a great person by rewriting this patch over and over again, untill Dries decides it is good enough to get in to the trunk.
I doubt about the fact if there are more persons in our community who are like you.. People who would revise their patch 37 times, and even after that still open for discussion..

Gerhard, I think you are a great example of how a real coder should do it..
I think we can all learn from you..

function book_update($node) {
-  db_query("UPDATE {book} SET parent = %d, weight = %d, log = '%s' WHERE nid = %d", $node->parent, $node->weight, $node->log, $node->nid);
+  if ($node->is_new || $node->revision) {
+    db_query("INSERT INTO {book} (nid, vid, parent, weight) VALUES (%d, %d, %d, %d, '%s')", $node->nid, $node->vid, $node->parent, $node->weight);
+  }
+  else {
+    db_query("UPDATE {book} SET parent = %d, weight = %d WHERE vid = %d", $node->parent, $node->weight, $node->vid);
+  }
 }

Why 'is_new' and 'revision'? Isn't 'is_new' redundant?

I loaded a fresh copy of the drupal.org database on my test system. How do I test this? Where is the upgrade script/path?

This is probably a little off topic but I can't find info elsewhere and this may affect revisions changes.

It appears as though all revisions of a node are accessible to all users (inc. anonymous users) using ?revision=x. Surely only the 'current' revision should be accessible by default, and there should be a permission that can be assigned to roles to view non-current revisions?

I don't want regular users looking at non-current revisions! A revision may have been made for various private reasons. Only admin should be able to do that.

On the latest snapshopt of the drupal.org database I get this:

ALTER TABLE {book} ADD PRIMARY KEY vid (vid)
FAILED
ALTER TABLE {forum} ADD PRIMARY KEY vid (vid)
FAILED
ALTER TABLE {node_revisions} ADD KEY nid (nid)
FAILED
ALTER TABLE {node_revisions} ADD KEY uid (uid)
FAILED
ALTER TABLE {node_revisions} ADD PRIMARY KEY vid (vid)
FAILED
CREATE TABLE {old_revisions} SELECT nid, revisions FROM {node} WHERE revisions != ''
OK
ALTER TABLE {book} DROP log
OK
ALTER TABLE {node} DROP teaser
OK
ALTER TABLE {node} DROP body
OK
ALTER TABLE {node} DROP format
OK
ALTER TABLE {node} DROP revisions
OK

Hi,

tested the patch a bit:

1. in database.pgsql in the table files
a) after removing PRIMARY KEY (fid) you need to remove also "," from the line above
b) you CREATE INDEX files_fid_idx ON node(fid); CREATE INDEX files_vid_idx ON node(vid); on wrong tables: not node but files

2. in newly created table node_revisions
a) in mysql version there is no DEFAULT, in postgres there is DEFAUL 0, maybe mysql version should be the same as postgres?
b) there's no log longtext NOT NULL default '' in pgsql version
c) you have translated format int(4) NOT NULL default '0' to format smallint NOT NULL default '0', it also should be integer and not smallint
d) you create index: CREATE INDEX node_revisions_title_idx ON node_revisions(title,type);, but there is no type column in the table, and also there is no similar index in mysql version

Attached patch should fix the errors

pgsql needs a sequence to be created, fixed in this patch.

An error when trying to view an old revision, e.g. in ?q=node/3/revision/1:

warning: pg_query(): Query failed: ERROR:  column "c.format" must appear in the GROUP BY clause or be used in an aggregate function in /var/www/dt/d/includes/database.pgsql.inc on line 69.

user error: 
query: SELECT c.cid, c.pid, c.nid, c.subject, c.comment, c.format, c.timestamp, c.name, c.mail, c.homepage, u.uid, u.name AS registered_name, u.picture, u.data, c.score, c.users FROM comments c INNER JOIN users u ON c.uid = u.uid WHERE c.cid = 0 AND c.status = 0 GROUP BY c.cid, c.pid, c.nid, c.subject, c.comment, c.timestamp, c.name, c.mail, u.picture, c.homepage, u.uid, u.name, u.picture, u.data, c.score, c.users in /var/www/dt/d/includes/database.pgsql.inc on line 86.

It's comment.module line 796, adding c.format to group by statement fixed this problem.

That's all for now...

Forgot to attach the patch for sequence

Cvbge: you attached the same patch twice?

Nope, the last one have CREATE SEQUENCE statement.

Here's my idea on how to support both mysql and pgsql databases. db_add_column() and db_change_column() come from http://drupal.org/node/29082. I've tested them then and they worked well. Now also added 2-3 new functions that are not yet tested, but are quite simple so they should work more or less. I can test it tommorow if I know this has any chanse getting in...

The patch is against HEAD+_43.

killes: as you requested, tested this patch on the most recent version of HEAD, with mysqld in ANSI mode and about 300 nodes (none of which had any old revisions prior to the patch and update). update script worked flawlessly and i encountered no problems w/ the site after installation. i also created a few new revisions--the revisions tab works properly, displaying only when there are new revisions for a node. the 'view' and 'set active' operations in the revisions tab also worked perfectly. let me know if there's anything else i can do to help!

A question: in database.* schema the table node_revision has PRIMARY KEY (nid, vid). But in updates.inc you do ADD PRIMARY KEY (nid). Does that mean that the table will have only nid as primary key?

It looks like after CREATE TABLE ... SELECT column attributes like NOT NULL and DEFAUL are lost. If that was not intentional I suggest to use CREATE TABLE + INSERT INTO ... SELECT.

Sorry, I was wrong in my last post.

Updated version on my patch, now it's complete for mysql, and almost complete for postgresql.

I've tested this with a simple script that is shown below - update_146() with my patch executes *exactly* the same sql statesments that original (ok, I have not tested the line that depends on db_fetch_object).

The script to show what sql statesments are executed in update_146():

include "database/updates.inc";
include "includes/common.inc";
function drupal_pg_version() { return "704"; }
function db_next_id($blah) { return "false vid"; }
function db_fetch_object($blah) { return FALSE; }
function db_query($query) {
  $args = func_get_args();
//   $query = db_prefix_tables($query); // Removed for debug version
  if (count($args) > 1) {
    if (is_array($args[1])) {
      $args = array_merge(array($query), $args[1]);
    }
    $args = array_map('db_escape_string', $args);
    $args[0] = $query;
    $query = call_user_func_array('sprintf', $args);
  }
  return _db_query($query);
}
function _db_query($query) { echo "$query\n"; }
$db_type = 'mysql';

update_146();

Please provide a patch that does not introduce new database functions/APIs.

Why?
Instead and writing the same code again and again it's better to write functions.

Anyway, I don't think patch will be pgsql compatibile before freeze, so if mysql part is ok then commit it.

the committed revisions patch (revisions_43.patch) doesn't update blog.module. now blog feeds throw an error

user error: Unknown column 'n.teaser' in 'field list'
query: SELECT n.nid, n.title, n.teaser, n.created, u.name, u.uid FROM node n INNER JOIN users u ON n.uid = u.uid WHERE n.type = 'blog' AND u.uid = 1 AND n.status = 1 ORDER BY n.created DESC LIMIT 0, 15

there are 2 occurrences of "$result = db_query_range(db_rewrite_sql("SELECT n.nid, n.title, n.teaser,...)" in blog.module which i guess have to be fixed. i would much appreciate if someone familiar with the revision changes could look at this (i had half an hour and couldn't get it). thanks a lot!

Committed to HEAD. Thanks.

Postgres' db_* stuff moved back to http://drupal.org/node/29082