Closed Bug 141951 Opened 22 years ago Closed 16 years ago

Set the max_packet_size for attachments (and bugs_fulltext) when connecting to MySQL

Categories

(Bugzilla :: Installation & Upgrading, defect, P1)

2.10

Tracking

()

RESOLVED FIXED
Bugzilla 3.2

People

(Reporter: CodeMachine, Assigned: mkanat)

References

Details

Attachments

(1 file, 1 obsolete file)

If the max attachment size in the system parameters is larger than the maximum
size MySQL allows, obscure MySQL error messages can get spewed out to the user
if their attachment is bigger than the MySQL size.

We could possibly use the SQL "SHOW VARIABLES" and read off the
"max_allowed_packet" variables.  From there we could force the administration
page to make the max attachment size smaller than this.

I figure there's probably some overhead in the packet besides the attachment, so
it would actually have to be some appropriately smaller size.
Priority: -- → P4
Target Milestone: --- → Bugzilla 2.20
Component: Administration → attachment and request management
Relevant information can be found at:
    http://www.mysql.com/doc/en/Packet_too_large.html
This has come up enough times to actually annoy me.
Assignee: justdave → kiko
*** Bug 263908 has been marked as a duplicate of this bug. ***
*** Bug 275821 has been marked as a duplicate of this bug. ***
*** Bug 285116 has been marked as a duplicate of this bug. ***
This would be really easy now that we have the Bugzilla::DB::Mysql module.

You can put the check inside Bugzilla::DB::Mysql::new, and then it will also get
checked inside of checksetup.
*** Bug 307704 has been marked as a duplicate of this bug. ***
*** Bug 329532 has been marked as a duplicate of this bug. ***
*** Bug 331643 has been marked as a duplicate of this bug. ***
*** Bug 339357 has been marked as a duplicate of this bug. ***
QA Contact: mattyt-bugzilla → default-qa
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
ACK. This should be done within the database module. May be the DB module should provide an function to get such db backend limitations. Other RDBMs don't have such low limitations (ie. postgresql allows 1 GB per row)

BTW: large attachements could be stored as blobs.
I (and other people, according to the dupes) encountered this same problem, but instead of spewing SQL errors, the upload _appears_ to work fine, and gives the message 'Attachment #0  to Bug #612 Created', which is of course untrue as attachment #0 cannot exist.  It also successfully obsoletes other uploads and lodges the comment, linked to bug #0, which is clearly incorrect behaviour if the upload failed!

The max_allowed_packet MySQL variable indeed solves the problem, and I agree that the DB module should handle this somehow, but in the meantime, how about catching this error?  Currently I have a bug with 5 comments referring to the same 'attachment #0', where a user has repeatedly attempted to upload a large file and been baffled as to why it didn't work.

I am using 2.20.2, so apologies if this error is being handled better in later versions.
kiko hasn't touched this in 2 years, and I suspect Max is in a better position to do something about it nowadays.  Per comment 6 this probably belongs in Installation/Upgrading since the check should be done in checksetup.pl and/or editparams.cgi
Assignee: kiko → installation
Component: Attachments & Requests → Installation & Upgrading
I would suggest that the check is also done on the upload page, as it is possible for this value to change after installation - that would make it more bullet-proof.
It may also be possible to change max_allowed_packet with a SET SESSION in Bugzilla::DB::Mysql::new(). I'll have to look into that.
The number of dups filed indicates to me that this is a higher-priority support issue than it is currently filed as, so I'm upgrading the priority and severity.
Assignee: installation → mkanat
Severity: minor → normal
Priority: P4 → P1
(In reply to comment #0)
> If the max attachment size in the system parameters is larger than the maximum
> size MySQL allows, obscure MySQL error messages can get spewed out to the user
> if their attachment is bigger than the MySQL size.
> We could possibly use the SQL "SHOW VARIABLES" and read off the
> "max_allowed_packet" variables.  From there we could force the administration
> page to make the max attachment size smaller than this.
> I figure there's probably some overhead in the packet besides the attachment, so
> it would actually have to be some appropriately smaller size.

yes

(In reply to comment #1)
> Relevant information can be found at:
>     http://www.mysql.com/doc/en/Packet_too_large.html

(In reply to comment #0)
> If the max attachment size in the system parameters is larger than the maximum
> size MySQL allows, obscure MySQL error messages can get spewed out to the user
> if their attachment is bigger than the MySQL size.
> We could possibly use the SQL "SHOW VARIABLES" and read off the
> "max_allowed_packet" variables.  From there we could force the administration
> page to make the max attachment size smaller than this.
> I figure there's probably some overhead in the packet besides the attachment, so
> it would actually have to be some appropriately smaller size.

(In reply to comment #0)
> If the max attachment size in the system parameters is larger than the maximum
> size MySQL allows, obscure MySQL error messages can get spewed out to the user
> if their attachment is bigger than the MySQL size.
> We could possibly use the SQL "SHOW VARIABLES" and read off the
> "max_allowed_packet" variables.  From there we could force the administration
> page to make the max attachment size smaller than this.
> I figure there's probably some overhead in the packet besides the attachment, so
> it would actually have to be some appropriately smaller size.

very good

The 2.22 branch is restricted to security bugs -> 3.0
Target Milestone: Bugzilla 2.22 → Bugzilla 3.0
Note that we also need this now for bugs_fulltext, and it's not very hard to fix. (If your comments all combined are larger than max_allowed_packet, they'll get truncated or there will be an error when you update a bug.)
Severity: normal → major
Summary: Make sure max attachment size is small enough to not cause MySQL errors. → Set the max_packet_size for attachments (and bugs_fulltext) when connecting to MySQL
Flags: blocking3.2+
Will this check be run during checksetup and edit_params?  Probably the best two places to do it.
Here is how to get the value:
    my ($max_pack_size) = @{$self->selectcol_arrayref(
        q{SHOW VARIABLES LIKE 'max\_allowed\_packet'}, {Columns=>[2]})};

Looks like the best place to do this is in the middle of the "update_params"
sub.
http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/Config.pm#113
It's going to be *set*, not checked.
Attached patch v1 (obsolete) — Splinter Review
Attachment #344540 - Flags: review?(LpSolit)
Comment on attachment 344540 [details] [diff] [review]
v1

>Index: Bugzilla/DB/Mysql.pm

>+    my $min_max_allowed_packet = MAX_COMMENTS * MAX_COMMENT_LENGTH;
>+    my $max_allowed_packet = max($min_max_allowed_packet, 
>+                                 Bugzilla->params->{'maxattachmentsize'});
>+    $self->do("SET SESSION max_allowed_packet = $max_allowed_packet");

One thing bothers me with your patch: if max_allowed_packet is already set to a larger value in my.cnf, you completely ignore it. I can imagine some cases where this could be a problem (e.g. in bugs with several tens or hundreds of long comments such as stack trace). Why not taking the max between $max_allowed_packet and "SHOW VARIABLES LIKE 'max_allowed_packet'"?
Attachment #344540 - Flags: review?(LpSolit) → review-
Attached patch v2Splinter Review
Yeah, fair enough. :-) Here we go.
Attachment #344540 - Attachment is obsolete: true
Attachment #344830 - Flags: review?(LpSolit)
Comment on attachment 344830 [details] [diff] [review]
v2

Looks good. Note that your patch doesn't apply cleanly on the 3.2 branch.
Attachment #344830 - Flags: review?(LpSolit) → review+
OK, I'll fix the patch for 3.2 when I check it in.
Flags: approval3.2+
Flags: approval+
tip:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.266; previous revision: 1.265
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.99; previous revision: 1.98
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.67; previous revision: 1.66
done
Checking in docs/en/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/installation.xml,v  <--  installation.xml
new revision: 1.162; previous revision: 1.161
done

3.2:

Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.16; previous revision: 1.241.2.15
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.92.2.4; previous revision: 1.92.2.3
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.60.2.4; previous revision: 1.60.2.3
done
Checking in docs/en/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/en/xml/installation.xml,v  <--  installation.xml
new revision: 1.157.2.4; previous revision: 1.157.2.3
done
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: relnote
Resolution: --- → FIXED
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Added to release notes for 3.2 in bug 463244.
Keywords: relnote
Blocks: 463380
Is it possible that this setting wasn't set when upgrading from 3.0.6 to 3.2?  Because I got this error and had to modify my /etc/my.cnf

Also, the release notes don't explain what needs to be redone if restoring from a dump; wouldn't checksetup.pl fix this automagically?
(In reply to comment #32)
> Is it possible that this setting wasn't set when upgrading from 3.0.6 to 3.2? 
> Because I got this error and had to modify my /etc/my.cnf

  I don't think so, no, unless your maxattachmentsize parameter is actually smaller than some of the attachments currently in your DB.

> Also, the release notes don't explain what needs to be redone if restoring from
> a dump; wouldn't checksetup.pl fix this automagically?

  Ah, yeah, that's something that we should probably explain somewhere, because you do have to do that now. You can file a bug for that, if you want.
(In reply to comment #33)
> (In reply to comment #32)
> > Is it possible that this setting wasn't set when upgrading from 3.0.6 to 3.2? 
> > Because I got this error and had to modify my /etc/my.cnf
> 
>   I don't think so, no, unless your maxattachmentsize parameter is actually
> smaller than some of the attachments currently in your DB.

No, but it was smaller than the value in my data/params. :-/  I thought this bug was about making them match so we wouldn't get this error?

> > Also, the release notes don't explain what needs to be redone if restoring from
> > a dump; wouldn't checksetup.pl fix this automagically?
> 
>   Ah, yeah, that's something that we should probably explain somewhere, because
> you do have to do that now. You can file a bug for that, if you want.

Added bug 476905
Details:
  Bugzilla 3.2
  /etc/my.conf had max_packet_size = 1M
  maxattachmentsize was set to 2000

  I got the infamous mysql error message.  I expected it to either say the attachment was too big or to adjust the max_packet_size to 2000 for the SQL connection.
(In reply to comment #34)
> No, but it was smaller than the value in my data/params. :-/  I thought this
> bug was about making them match so we wouldn't get this error?

  It makes them match when it connects, so it's only effective for Bugzilla code. If you have somehow customized your code, it may not be effective for your customizations.
(In reply to comment #36)
>   It makes them match when it connects, so it's only effective for Bugzilla
> code. If you have somehow customized your code, it may not be effective for
> your customizations.

Hmm...  Nothing should be customized in the process.cgi or attachment uploading stuff...  I have customizations on the edit templates... but it goes to the default attachment upload template first.

Maybe it's broken or it doesn't have permission to set max_packet_size or something?

Should I file a new bug, then?

I can recreate the situation on off hours, if need be.
(In reply to comment #37)
> Should I file a new bug, then?

  You could, but it's almost certainly a local problem, since I tested this fairly thoroughly when I wrote the patch, I believe. However, if we're not throwing an error message when we can possibly do so (say, that something is denying us access to setting this configuration var), then that would be bug-worthy.
Is there someway I can tell if something is denying access to the configuration var?
Blocks: 480001
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: