Move node revision information for files to a separate table

I agree. NID and VID should be removed, making a new table containing FID, NID, and VID with all three as keys.

Here's a patch with the proposed changes. This is completely untested. I don't know SQL very well, so I'm sure I've done something wrong. I'll hopefully be able to do testing later today.

Updated patch:
- cleaned up errors in database.*
- cleaned up errors in update.inc

I tested adding files, listing and unlisting files, deleting files, deleting revisions, deleting nodes, adding files only to certain revisions, etc. Deleting revisions does not delete the files, but as far as I can tell, that's because of http://drupal.org/node/31323.

Updates after code review from Cvbge (thanks!):
- fixed typo in database.pgsql
- using a select statement in update_148()
- fixed logic in upload_delete()

Keeping up with HEAD.

Updated again. Now, fuzz free!

This seems more like a task than a bug.

This patch also fixes several broken queries in upload.module.

Keeping up with head.

Patch is out of date. Agreed, this functionality would be very useful, the revisions patch was great, but there are lots of new, unforseen issues that need to be dealt with to really make the new revisions system shine.

Anyone willing to tackly this? Would be nice to clean this up before 4.7.

I'll update it within the next couple of days.

Excellent, I'm willing to help out debug/test etc I need this too!

Updated to HEAD. This patch is untested, as there are some problems with uploads in core that need to be worked out. I'll try to get some testing done tomorrow.

Overall code looks good, but until this patch goes in: http://drupal.org/node/37605 gonna be a bit harder to test, as I would deem that one a bit higher priority ;-) But I do very much like this idea...

Updated again to keep up with HEAD. This patch also includes the following changes from the previous patch:

- Tested and corrected the mysql upgrade path. I attempted to correct the upgrade path for pgsql, but it still needs testing.
- Added a 'delete revision' case to upload_nodeapi to fix the upload.module portion of http://drupal.org/node/31323
- Added upload_delete_revision() to handle revision deletion. I tested this a fair amount.

I tested adding new revisions, changing revisions, deleting revisions, and deleting nodes. I could not test adding new files, as that appears to be broken in HEAD. Everything else seemed to work fine.

Actually, there seems to have been a problem when I deleted a node with attachments. I look into it soon.

Updated to HEAD. Also, I fixed the problem with files not being deleted when nodes were deleted. I also improved the performance of upload_delete a bit.

I still can't get files to upload to nodes. I'll do a fresh checkout and see if it's a problem in my patch or something in CVS (like http://drupal.org/node/37605).

File uploads don't work without my patch either, so it looks like that'll have to be fixed before I can go any further.

Keeping up with HEAD. Tested a bit more now that file uploads are working and everything seems to work well. I also fixed a bug in the previous patch where upload_delete() wasn't deleting files from old revisions if they weren't in the current revision. I also fixed a bug with removing files from nodes when revisioning was disabled (and removed an unnecessary query in the process).

Keeping up with HEAD.

Could you exaplain what this part about "get rid of duplicates in files table" in updates.inc is supposed to do? I'm a bit lost here.

Certainly. This patch removes one of the primary key values, VID, and moves some other data to the file_revisions table. To create the new primary key, FID, we have to get rid of duplicate rows.

I didn't understand how revisions work, started asking, and found following issues:

1. can count the same file many times?

function upload_space_used($uid) {
  return db_result(db_query('SELECT SUM(filesize) FROM {files} f INNER JOIN {file_revisions} r ON f.fid =
 r.fid INNER JOIN {node} n ON r.nid = n.nid WHERE n.uid = %d', $uid));
}

2. node_revisions(nid) is used only in following queries, maybe it's not needed in the table?

db_result(db_query('SELECT SUM(filesize) FROM {files} f INNER JOIN {file_revisions} r ON f.fid =
 r.fid INNER JOIN {node} n ON r.nid = n.nid WHERE n.uid = %d', $uid));

db_query("SELECT * FROM {files} f INNER JOIN {file_revisions} r ON f.fid = r.fid WHERE r.nid = %d", $node->nid);
...
db_query('DELETE FROM {file_revisions} WHERE nid = %d', $node->nid);

Changes:
1. database.pgsql: DEFAULT 0, not DEFAULT '0'
2. update.inc: postgresql part

Is it possibile that (fid,vid) in old files table is not unique? It had no PK/UNIQUE...

I could not test the patch, as file uploads do not work currently...

FIle uploads *do* work with head, a patch went in a few days ago that fixed this. I've been using file uploads on CVS on a live site for a few days now, no problems.

I can change the default '0's, but most of the rest of database.pgsql is filled with them. It's possible that fid/vid isn't unique, but it shouldn't happen.

Here's a bit better patch for postgresql update. Also fixes a bug in postgresql update

Indeed, the file uploads work.

I've done some simple test, with the patch applied, and it seems to work.

The only problem is I can't edit not-active node revisions (that's probably has nothing to do with this issue...)

Here's another go at it. Changes from last patch (#11):

- Incorporated Cvbge's changes for pgsql (thanks!).
- Moved nid back to the files table on Cvbge's recommendation. It's not used in the key in file_revisions and doesn't change with revisions, so there was no point for it to be there.
- Corrected the default values in the file and file_revisions table for integers. They should be 0, not '0'. This change will also occur for existing installs.
- Fixed the query in upload_space_used().
- Reordered the logic in upload_save() somewhat. Now there's one foreach loop that handles everything instead of three separate loops.

I'll have to do another round of testing now.

I ran into a problem when changing upload_save(). The upload_nodeapi:validate call at the top of the function only returns new uploads when called from upload_save, not existing uploads. I'm not sure if that's by design or a bug. Note that upload_nodeapi:validate is called from upload_js, upload_nodeapi:update, upload_nodeapi:validate, and _upload_form_alter.

I finally got a chance to do that testing. Here's a new patch, updated to HEAD. This patch also fixes two issues I found:

1) The upgrade patch for mysql was broken
2) Nodes without files couldn't be created

In testing, I also found the following three issue:

1) The list attribute is always (except for #3 below) enabled for new uploads, no matter what the checkbox setting is
2) In JS mode, the list checkbox is checked by default for new uploads. In non-JS mode, it's unchecked by default
3) The list attribute is not set for new uploads if they weren't Attached before the node is Submitted

These issues exist *without* this patch as well, thus they should not stop this patch from hitting Core!

Keeping up with HEAD.

Updated after some comments on IRC from killes. Fixed some style issues and removed a query change I did, as it removed a node access check.

Keeping up with HEAD.

This is the last patch standing in the way of continued work on http://drupal.org/node/25756.

Furthermore, this patch is the first step in allowing comments/users/etc. to have uploads. After this patch goes in, we can add a "type" column and change "nid" to "id" in the files table and the DB will be ready. In the type column, we put things like "node", "comment", or "user". In the "id" column, we put the nid, cid, or uid, respectively.

Patch applies cleanly and I can attach different files to different revisions and they each show up. However, when trying to download files on revisions, I get access denied (as uid=1).

This is with downloads set to 'private'.

Just to follow up, if I change modes to 'public' files are correctly downloaded.

However, discovered another problem, if I create some revisions and attach files, then later make a new revision with no files, then later try and revert back an older revision with files attached, the new revision that is created (or copied) doesn't have it's files anymore.

The last problem you mentioned also exists in HEAD, but I can fix it. m3avrck also mentioned the following issue on IRC: "when you create that new revision with one that has a file attached, the file is still there, however it no longer remembers its 'list' being checked"

Here's a new patch with the following changes for the last:

1) Addressed the problem m3avrck brought up about setting old revisions active
2) Changed some double quotes to single quotes

The problem with private downloads also happens in current HEAD and is unrelated to this patch. I could not reproduce the other problem m3avrck mentioned.

Everything is working with the latest patch! Did not run into any problems at all. I think before I might have had a bad node with some data, fresh install this time, everything is working great. Tested all sorts of revisions, files, combinations, everything worked as expected!

I say +1 and this is RTC but I won't set it, I'd like to have another set of eyes glance over as well. Additionally, this update should go in before 4.7, as this complements the new revisions system quite well, and opens up a host of new possibilities for modules and patches.

Keeping up with HEAD. I also fixed an error when deleting a node.

Needs to be rerolled. updates.inc changed.

Keeping up with HEAD.

I'll commit it when Gerhards tells me so. He gets to be responsible for this issue. :)

I've applied the patch and ran the update. Worked fine.

When I looked at the tables afterwards I noticed that there is no index on nid in the files table. Maybe there should be one.

Also, then I tried to upload a file, it did work. The file is uploaded, the info is stored in the database, but when I edit the node again, no file is shown.

This might be dues to the general brokenness of uploads, but I don't think so.

Setting to "needs work".

m3avrck and I have been unable to reproduce the node edit problem. I did see an issue with the list checkbox[1], but that also exists in current CVS.

FWIW, I tested with and creating a new node revision with public downloads.

[1] The list checkbox never seems to stay checked when the node is submitted if a new upload is attached. It's checked when submitting, but then the file doesn't show. Editing the node, enabling list, and submitting fixes it.

Here's an updated patch that fixes a broken query that killes found. I'll have to do some more testing when I'm not logged in as UID 1, as I haven't done that at all.

I've found two problems in limited testing:

1) List is saved to the database as the FID instead of a BOOL.
2) Sometimes (I haven't figured out when), an row containing no fid, description, or list (i.e. just a vid) will be written to file_revisions

I'm low on time right now and would appreciate help in tracking down these issues so this patch can land ASAP.

Updated again to HEAD. The list ID saving problem is also in HEAD and should be fixed in a separate patch. I cannot reproduce the blank database row issue. I'll be happy to try to track it down if someone can tell me how to reproduce it. Otherwise, I hope this patch can be committed.

I added files, created revisions, rolled back, deleted files and all worked well. nice.

I did see the blank row in file_revisions. i tried to reproduce it after seeing once but i was unable. here is what i did

started from scratch. applied patch and update.php

1. created a story with 1 attachment
2. edited same story and added an attachment. created new revision
3. rolled back a revision, so node only has 1 attachment

at this point, i noticed a fid=3 row with no other data in file_revisions table. that record might have been there since step, i don't know. i hope this helps. even if that row is not fixed, i think this is ready to commit.

thanks for sticking with this patch.

Moshe: OK, I was able to reproduce it. It's just some bogus data being added, not a missing file. Now to figure out what's causing it....

I've been able to reliably reproduce the problem, though I don't know how to fix it. Here's how to reproduce it:

1) Edit a node
2) Enable "Create new revision"
3) Enter a file to attach
4) Press Submit without attaching first

There will now be a new entry in file_revisions for the new vid with 0 for fid and list.

As far as I can tell, this is caused by an extra 'upload_N' in $_SESSION inserted by upload_nodeapi:validate. This problem is also present in HEAD, though not visible because of a check for is_numeric() on line 414 in upload_save(). The problem seems to originate in the following code in upload_nodeapi:validate (starting on line 250 in HEAD):

          $key = 'upload_'. count($_SESSION['file_uploads']);
          $file->source = $key;
          $file->list = 1;
          $file = file_save_upload($file);
          $node->files[$key] = $file;

Or, it could be the nonsensical file_check_upload('upload') on line 210 in HEAD.

I'll add a check for is_numeric(), as in HEAD. Keep in mind that this is a work-around and doesn't fix the real problem. And I don't have the expertise to fix the real problem.

Attached is a new patch, updated to HEAD, which includes the is_numeric() change.

The list problem mentioned in comment #49 is already filed as http://drupal.org/node/42358. The upload_nodeapi:validate problem described in comment #53 is filed as http://drupal.org/node/48092.

i tested again, and code reviewed.

Keeping up with HEAD.

Committed to HEAD. Thanks.

Is there possibly an error in the update function? The update does not show up on update.php.

i tried it on four of my (test) drupal installs and it works perfectly...

Killes: What do you mean that it doesn't show up? You run the update script and it says you are up-to-date? That could happen if you've tested a patch that uses the same update number.

I've updated another database and it didn't have the problem. No idea what went wrong with the other db.

Maybe I'm misunderstanding the this, but isn't the following code a bit backwards?

+      if ($node->remove[$key]) {
+        db_query('DELETE FROM {file_revisions} WHERE fid = %d AND vid = %d', $key, $node->vid);
+        // Only delete a file if it isn't used by any revision
+        $count = db_result(db_query('SELECT COUNT(fid) FROM {file_revisions} WHERE fid = %d', $key));
+        if ($count < 1) {
+          db_query('DELETE FROM {files} WHERE fid = %d', $key);
+          file_delete($file->filepath);
+        }

It DELETES FROM {file_revisions} all fids that are $key.

However, right below, it then trys to get the count of fid that is $key.

If you already deleted them all, then this will always be 0 and then the file is always removed, when in fact, maybe it shouldn't.

Or am I misunderstanding something here?

Ah just after posting I noticed the vid part. So that first one is deleting the file as part of that revision and if that file isn't used in any *other* (missing this keyword) then delete.

Ok makes sense.

Ah just after posting I noticed the vid part. So that first one is deleting the file as part of that revision and if that file isn't used in any *other* (missing this keyword) then delete.

Ok makes sense.