Move attachments.thedata to its own table

RESOLVED FIXED in Bugzilla 2.22

Status

()

P2
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bugreport, Assigned: bugreport)

Tracking

2.21
Bugzilla 2.22
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

13 years ago
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)
(Assignee)

Comment 1

13 years ago
Created attachment 193318 [details] [diff] [review]
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.
Assignee: attach-and-request → bugreport
Status: NEW → ASSIGNED
Attachment #193318 - Flags: review?
(Assignee)

Comment 2

13 years ago
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.
Attachment #193318 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #193318 - Attachment is obsolete: true
(Assignee)

Comment 3

13 years ago
OK...  I have fixes for both problems.  I just need to merge them.
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.22
(Assignee)

Comment 4

13 years ago
Created attachment 193363 [details] [diff] [review]
Revised - make both databases happy
Attachment #193363 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #193363 - Flags: review? → review?(LpSolit)
(Assignee)

Comment 5

13 years ago
Created attachment 193781 [details] [diff] [review]
Same as previosu, but without extra clutter
Attachment #193363 - Attachment is obsolete: true
Attachment #193781 - Flags: review?(LpSolit)
(Assignee)

Updated

13 years ago
Attachment #193363 - Flags: review?(LpSolit)

Comment 6

13 years ago
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 - bugreport@peshkin.net - 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 ("
Attachment #193781 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 7

13 years ago
Created attachment 193893 [details] [diff] [review]
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.
Attachment #193781 - Attachment is obsolete: true
Attachment #193893 - Flags: review?(LpSolit)

Comment 8

13 years ago
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
Attachment #193893 - Flags: review?(LpSolit) → review+

Updated

13 years ago
Flags: approval?
(Assignee)

Comment 9

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

Updated

13 years ago
Blocks: 302669
Flags: approval? → approval+

Comment 10

13 years ago
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.
Attachment #193893 - Flags: review-

Comment 11

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

Comment 12

13 years ago
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.
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.
(Assignee)

Comment 14

13 years ago
Created attachment 193982 [details] [diff] [review]
Patch to be checked in

Carrying r+ forward per justdave's comment
Attachment #193893 - Attachment is obsolete: true
(Assignee)

Comment 15

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Keywords: relnote

Comment 16

13 years ago
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 ;)
Attachment #193982 - Flags: review+

Comment 17

13 years ago
Added to the Bugzilla 2.22 Release Notes in bug 322960.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.