305333 - Move attachments.thedata to its own table

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

Two major reasons to do this...
1) When the attachments table gets huge, any queries involving it get extremely
slow even when the attachments.thedata field is not involved.

2) In sites using shadow databases, it is a shame to keep multiple copies of the
attachments data around.  (If a site chooses not to replicate the attachment
data table, then it has to choose between disabling atatchment content searches
or performing those searches against the master)

Comment 1

[Joel Peshkin]

Attachment: Move attachment data to a seperate table

I'm planning to test this on a copy of my huge site tomorrow.

I've already tested upgrading, attaching, viewing, editing, and searching on a
test site.

Comment 2

[Joel Peshkin]

Comment on attachment 193318 [details] [diff] [review]
Move attachment data to a seperate table

OK... 2 problems

1) Postgres doesn't like the simple insert syntax in checksetup.  I tried the
same syntax as is used in attachment.cgi and I got ... 
checksetup.pl: DBD::Pg::st execute failed: ERROR:  relation "attach_data" does
not exist [for Statement "INSERT INTO attach_data
[Sun Aug 21 07:38:54 2005] checksetup.pl: 

2) On my huge database, the attachment migration got an out of memory error.

Comment 3

[Joel Peshkin]

OK...  I have fixes for both problems.  I just need to merge them.

Comment 4

[Joel Peshkin]

Attachment: Revised - make both databases happy

Comment 5

[Joel Peshkin]

Attachment: Same as previosu, but without extra clutter

Comment 6

[Frédéric Buclin]

Comment on attachment 193781 [details] [diff] [review]
Same as previosu, but without extra clutter

>Index: checksetup.pl

>-AddFDef("attachments.thedata", "Attachment data", 0);
>+#AddFDef("attachments.thedata", "Attachment data", 0);

Nit: I think you should remove it completely or replace it by:

>+$dbh->do("DELETE FROM fielddefs WHERE name='attachments.thedata'");
>+AddFDef("attach_data.thedata", "Attachment data", 0);


> # Create Administrator  --ADMIN--
> ###########################################################################
> 
>+# 2005-08-19 - [e-mail] - Bug 305333
>+$dbh->do("DELETE FROM fielddefs WHERE name='attachments.thedata'");
>+AddFDef("attach_data.thedata", "Attachment data", 0);
>+if ($dbh->bz_column_info("attachments", "thedata")) {

This should be moved into the ---TABLE--- section.



>Index: Bugzilla/DB/Schema.pm

>+    attach_data => {
>+        FIELDS => [
>+            id    => {TYPE => 'INT3', NOTNULL => 1,
>+                             PRIMARYKEY => 1},
>+            thedata      => {TYPE => 'LONGBLOB', NOTNULL => 1},

Nit: could you align '=>' arrows?

id	=> {TYPE ...
thedata => {TYPE ...



Files missing in your patch:

Bugzilla/Search/Quicksearch.pm:

83:		   "attachmentdata" => "attachments.thedata",
84:		   "attachdata" => "attachments.thedata",


template/en/default/pages/quicksearchhack.html.tmpl:

258:  <td>Attachment Data <i>(&ldquo;attachments.thedata&rdquo;)</i></td>


contrib/bug_email.pl:

167:	    my $sql = "insert into attachments (bug_id, creation_ts,
description, mimetype, ispatch, filename, thedata, submitter_id) values (";


contrib/jb2bz.py:

251:			    "filename=%s, thedata=%s, submitter_id=%s",


contrib/cmdline/query.conf:

46:attachments.thedata	   substring	   "attachdata"


contrib/gnatsparse/gnatsparse.py:

454:		print >>outfile, "  bug_id, filename, description, mimetype,
ispatch, submitter_id, thedata) values ("

Comment 7

[Joel Peshkin]

Attachment: cleaned up review coments

OK, covered all the comments.

I made sure the contrib/ code still compiles, but it is not possible to test
it.
I did check out the fancy quicksearch stuff (I never knew that was there!)
I did confirm that the last_insert_id can be placed in an insert statement. 
The contrib stuff is not db-portable anyway.

Comment 8

[Frédéric Buclin]

Comment on attachment 193893 [details] [diff] [review]
cleaned up review coments

>Index: Bugzilla/Search.pm

>+         "^attach_data\.thedata," => sub {

In query.cgi, "Attachment data" "changed after" "2005-08-25" doesn't work:

 Can't use attach_data_0.thedata as a field name.

Maybe my query is invalid, so I will let joel fix that in another bug. What I
wanted was all attachments newer than 2005-08-25.



>Index: contrib/gnatsparse/gnatsparse.py

>-            print >>outfile, "%s,%s,%s, %s,0, %s,%s);" %(self.bug_id, SqlQuote(name), SqlQuote(name), SqlQuote (ftype), who, SqlQuote(zlib.compress(data)))
>+            print >>outfile, "%s,%s,%s, %s,0, %s,%s);" %(self.bug_id, SqlQuote(name), SqlQuote(name), SqlQuote (ftype), who)

Nit: Now that SqlQuote(zlib.compress(data)) has been removed, the last %s
should be removed as well. Please fix that on checkin.


Your patch works well, except the problem I got with my query. r=LpSolit

Comment 9

[Joel Peshkin]

Note: I'll have a subsequent fix for the Search bug by the end of the day under
another bug.

Comment 10

[Max Kanat-Alexander]

Comment on attachment 193893 [details] [diff] [review]
cleaned up review coments

Sorry, tiny little thing, but it's important for certain parts of the Schema
code:

INDEXES must be *absent* if it's empty.

Comment 11

[Max Kanat-Alexander]

Oh, also, you should use $dbh->bz_last_insert_id instead of LAST_INSERT_ID(),
where possible.

Comment 12

[Joel Peshkin]

comment 10 seems like something that can be fixed on checkin.

comment 11 does not apply since the only place LAST_INSERT_ID appears is in
contrib/ code that doesn't use any of the new modules, is not cross-db
compatible, and - in most cases - is written in python.

Comment 13

[Dave Miller [:justdave]]

Contrib stuff isn't really supported.  If we can fix it, it's nice, but it's not
a requirement for checkin.  If we know we broke it, we should file a bug saying
so and try to CC the original contributor.

I prefer to get the final patch posted prior to checkin just so we have an
accurate and working patch on the bug.  But feel free to carry r+ forward and go
ahead and check it in for that one change.

Comment 14

[Joel Peshkin]

Attachment: Patch to be checked in

Carrying r+ forward per justdave's comment

Comment 15

[Joel Peshkin]

Checked in with the one issue fixed on checkin... actual patch is attached as
attachment 193982 [details] [diff] [review]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.94; previous revision: 1.93
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.433; previous revision: 1.432
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.24; previous revision: 1.23
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.109; previous revision: 1.108
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.36; previous revision: 1.35
done
Checking in Bugzilla/Search/Quicksearch.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v  <--  Quicks
earch.pm
new revision: 1.2; previous revision: 1.1
done
Checking in contrib/bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.29; previous revision: 1.28
done
Checking in contrib/jb2bz.py;
/cvsroot/mozilla/webtools/bugzilla/contrib/jb2bz.py,v  <--  jb2bz.py
new revision: 1.5; previous revision: 1.4
done
Checking in contrib/cmdline/query.conf;
/cvsroot/mozilla/webtools/bugzilla/contrib/cmdline/query.conf,v  <--  query.conf

new revision: 1.3; previous revision: 1.2
done
Checking in contrib/gnatsparse/gnatsparse.py;
/cvsroot/mozilla/webtools/bugzilla/contrib/gnatsparse/gnatsparse.py,v  <--  gnat
sparse.py
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/pages/quicksearchhack.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/quicksearchhack.htm
l.tmpl,v  <--  quicksearchhack.html.tmpl
new revision: 1.2; previous revision: 1.1
done

Comment 16

[Frédéric Buclin]

Comment on attachment 193982 [details] [diff] [review]
Patch to be checked in

r=LpSolit

Maybe too late, but at least the patch has been "officially" reviewed ;)

Comment 17

[Max Kanat-Alexander]

Added to the Bugzilla 2.22 Release Notes in bug 322960.