252272 - Allow extremely large attachments to be stored locally

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P2)

Description

[Joel Peshkin]

Tracefiles can often FAR exceed 16M and are only temporarily needed.  Storing
them on the local fielsystem can help.

Comment 1

[Joel Peshkin]

Attachment: Functional patch - still needs work

This patch does work, but it needs further refinement.	Specifically, it needs
to handle the sizes int he attachment table better.

Comment 2

[Joel Peshkin]

Attachment: Patch v2

Actually, getting that to work properly is a lot easier than it looked.

Comment 3

[Joel Peshkin]

Attachment: patch v3

Comment 4

[Joel Peshkin]

Comment on attachment 153826 [details] [diff] [review]
patch v3

This feature is off by default.  It certainly has no place on BMO, but it is
essential for those who need it.

Is this something that can land??

Comment 5

[Gervase Markham [:gerv]]

Not without some comments and a spell-checking session :-)

Also, why does the _user_ get to specify which files are big files? What would
entice them ever to check the checkbox? And what's all this "purging" business?
How does this work with groups?

Gerv

Comment 6

[Jouni Heikniemi]

Without reading the patch (yet), I think I could find some use for this feature.
Data dumps etc. frequently exceed the rational maximum attachment size for a
MySQL blob.

Comment 7

[Joel Peshkin]

The user would check the box if they want to upload a file that would otherwise
not be permitted.  They might do it out of good housekeeping also since they may
be aware that the file will not be important years from now.  That is true of
many trace files that are no longer needed once analyzed.

The reason that we need the user to specify this is we avoid reading the entire
file into a perl variable andtransfer it, a buffer at a time, to the local file.

This is independent of groups.  Everything about managing the attachments
follows the same logic as attachments stored in the DB.

Comment 8

[Gervase Markham [:gerv]]

> The user would check the box if they want to upload a file that would 
> otherwise not be permitted.  

But that wouldn't know that unless they'd tried to upload it first, waited five
minutes for however much data to be sent, and then got an error. Not shockingly
user-friendly. Why not automatically switch to this mode when we go over a
certain size?

> They might do it out of good housekeeping also since they may
> be aware that the file will not be important years from now.  

How can anyone know that? Maybe only 1 in 100 old bugs need further work later
(we thought we fixed it but we didn't, or whatever), but you don't know _which_
one. Given how cheap disk space is these days, is there _ever_ a need to throw
away data attached to old bugs?

Gerv

Comment 9

[Joel Peshkin]

It probably makes sense to have the limits stated on the upload form. (this is
true even without this feature)

Like I said, I dont expect this to be activated on BMO

Comment 10

[Vlad Dascalu]

Comment on attachment 153826 [details] [diff] [review]
patch v3

Needs some comments and a spell-checking session :)

Comment 11

[Joel Peshkin]

Attachment: v4 - With spelling fixes and comments

Comment 12

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 158268 [details] [diff] [review]
v4 - With spelling fixes and comments

This crashes on me when I try to open an attachment I removed on the file
system :(

The remainder are nits...

Do you think it's good to speak of "local storage"? To me, when looking at my
browser, I read that as "on my personal computer". If it's not too technical,
maybe "on the server's file system"? Or simply "outside the database"? Other
ideas?

Generally, I think there should be a space following a comma.

+  # Skip uploading into a local variable if the user want to upload huge

s/want/wants/

Good patch. I think I'd use this functionality :)

Comment 14

[Joel Peshkin]

Attachment: Revised to catch removed attachments

Fixed removed file and fixed permissions of directory so bugzilla user can
remove files without being root.

Comment 15

[Marc Schumann [:Wurblzap]]

Comment on attachment 173955 [details] [diff] [review]
Revised to catch removed attachments

Tested; works.

If I view the "edit" page of a deleted image attachment, I see an embedded
Bugzilla error page in the place where the image would be. I'd like a
placeholder image or a some text without the Bugzilla header, footer and
"Please press Back" message a lot better. But that's probably not that easy, so
I think that's something for a follow-up bug.

I'm really not happy with the usage of the word "local". I still think it's
misleading, letting the user think of the desktop PC instead of the server. If
you don't want to address this now, we can have a follow-up bug on this, too.

The space-after-commas nit is a nit, but it's that kind of nit I expect you to
fix :)
http://www.bugzilla.org/docs/developer.html plus
http://www.perldoc.com/perl5.6/pod/perlstyle.html; please put up a new patch
and carry r+ forward.

Comment 16

[:glob ✱]

shouldn't you be setting binmode on AH?

right now it works as the file is translated when writing to and reading from
disk, however this results in the file on disk being corrupt.

Comment 17

[Joel Peshkin]

hmm... sounds like a win32 thing :-)

Comment 18

[Joel Peshkin]

Attachment: Added binmode to keep win32 happy

Cleaned up comment nits and added binmode

Comment 19

[Marc Schumann [:Wurblzap]]

Comment on attachment 174018 [details] [diff] [review]
Added binmode to keep win32 happy

I'm sorry, it seems I failed to catch a couple of points last time.

The directory name should imho be a variable. It could be put into
Bugzilla/Config.pm at $webdotdir's side.

And I think that data/files/ (or perhaps even data/attachments/) is better than
files/.

>Index: attachment.cgi
>@@ -504,6 +511,22 @@
>     # Return the appropriate HTTP response headers.
>     $filename =~ s/^.*[\/\\]//;
>     my $filesize = length($thedata);
>+    # A zero length attachment in the database means the attachment is 
>+    # stored in a local file
>+    if ($filesize == 0)
>+    {
>+        my $attachid = $::FORM{'id'};
>+        my $hash = ($attachid % 100) + 100;
>+        $hash =~ s/.*(\d\d)$/group.$1/;
>+        open(AH, "files/$hash/attachment.$attachid");
>+        binmode AH;
>+        $filesize = (stat(AH))[7];
>+    }
>+    if ($filesize == 0)
>+    {
>+        ThrowUserError("attachment_removed");
>+    }

If the file has been purged, binmode and stat issue warnings. You should move
ThrowUserError up so that binmode and stat don't happen if open fails, like
open() || ThrowUserError.

>+  # If the file is to be stored locally, stream the file from the webserver
>+  # to the local file without reading it into a local variable.
>+  if ($::FORM{'bigfile'})
>+  {
>+    my $fh = $cgi->upload('data');
>+    my $hash = ($attachid % 100) + 100;
>+    $hash =~ s/.*(\d\d)$/group.$1/;
>+    mkdir 'files',0770;
>+    mkdir "files/$hash",0770;
>+    chmod 0770,"files/$hash";

Why are you trying to create the directory here? Somebody should have run
checksetup.pl, and you're creating it there. It shouldn't even work here, at
least not for systems properly set up, because the webserver shouldn't have
write access to the Bugzilla directory.

And while you're here, please put a blank after the commas here that seem to
have been missed.

>Index: Bugzilla/Attachment.pm
>@@ -92,6 +92,16 @@
>     # Retrieve a list of flags for this attachment.
>     $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
>                                           'is_active' => 1 });
>+
>+    # A zero size indicates that the attachment is stored locally.
>+    if ($a{'datasize'} == 0) {
>+        my $attachid = $a{'attachid'};
>+        my $hash = ($attachid % 100) + 100;
>+        $hash =~ s/.*(\d\d)$/group.$1/;
>+        open(AH,"files/$hash/attachment.$attachid");
>+        $a{'datasize'} |= (stat(AH))[7];
>+        close(AH);
>+    }

The calls to stat and close issue warnings when the file doesn't exist any
more. A failing open() needs to be caught here.
You need to replace |= by =, too.
And a comma after AH is missing :)

Comment 20

[Joel Peshkin]

Attachment: take 3

OK... got all that.  $attachdir is the place

Comment 21

[Marc Schumann [:Wurblzap]]

Comment on attachment 174118 [details] [diff] [review]
take 3

Ok, there's only one thing left :)

>Index: attachment.cgi
>@@ -514,6 +539,11 @@
>     print $thedata;
>+    while (<AH>) {
>+        print $_;
>+    }
>+    close(AH);

We get warnings here when viewing *db-based* attachments ("unopened
filehandle"), so the loop needs to be conditional here.

Comment 22

[Joel Peshkin]

Attachment: take 4 - check for warnings

Comment 23

[Marc Schumann [:Wurblzap]]

Comment on attachment 174577 [details] [diff] [review]
take 4 - check for warnings

That's it. Thank you.

Comment 24

[Joel Peshkin]

checked in

Comment 25

[Max Kanat-Alexander]

Added to the release notes in bug 286274.

Comment 26

[A. Karl Kornel]

Attachment: Docs Patch v1

There were two areas mentioning MySQL config and attachments, I added notes
saying that those do not apply to Big Files.  Also updated the Hints & Tips
section.

Requesting review from documentation@.

Comment 27

[A. Karl Kornel]

Comment on attachment 188987 [details] [diff] [review]
Docs Patch v1

Requesting review from joel to make sure what I actually wrote is correct.

I notably did not cover anything on what information remains in Bugzilla when
the Big File is deleted, how to delete Big Files, who can delete Big Files, how
to enable Big Files, or the fact that Big Files may not work as well as
expected when running Bugzilla over multiple machines accessing one database. 
If requested to do so, I can put that in.

Comment 28

[Joel Peshkin]

Comment on attachment 188987 [details] [diff] [review]
Docs Patch v1

r=joel for content
Someone should make sure the docs build before checkin.

Comment 29

[Colin Ogilvie [:cso]]

Comment on attachment 188987 [details] [diff] [review]
Docs Patch v1

r=me on the fact the docs compile - joel has given r on the contents.

Comment 30

[Colin Ogilvie [:cso]]

I've checked in Patch v1, but I think the comments you made when you attached it
are valid; there should be documentation on how to enable it etc. would be useful.

2.20
Checking in installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <-- 
installation.xml
new revision: 1.98.2.2; previous revision: 1.98.2.1
done
Checking in using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.1; previous revision: 1.33
done


Trunk
Checking in installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <-- 
installation.xml
new revision: 1.100; previous revision: 1.99
done
Checking in using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.34; previous revision: 1.33
done