_sync_fulltext always updates bugs_fulltext.short_desc, even if it wasn't changed

RESOLVED FIXED in Bugzilla 4.4

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

({perf})

unspecified
Bugzilla 4.4
Bug Flags:
approval +
approval4.4 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
_sync_fulltext always updates bugs_fulltext.short_desc, even if it wasn't changed.

this is problematic on mysql because the bugs_fulltext table is MyISAM, which has very broad locks (no row-level locks).

by updating the short_desc field each time a comment is added (even if it hasn't changed), we're taking a broad lock for no reason, which can cause locking problems on high volume sites.
(Assignee)

Comment 1

6 years ago
Created attachment 687661 [details] [diff] [review]
patch v1

this patch reworks sync_fulltext to ensure we update the bugs_fulltext table only once for each update (currently the code performs two separate actions).
Assignee: create-and-change → glob
Status: NEW → ASSIGNED
Attachment #687661 - Flags: review?(LpSolit)
(Assignee)

Comment 2

6 years ago
Created attachment 688250 [details] [diff] [review]
patch v2

i missed a few calls to _sync_fulltext, which were being made outside the package.
Attachment #687661 - Attachment is obsolete: true
Attachment #687661 - Flags: review?(LpSolit)
Attachment #688250 - Flags: review?(LpSolit)

Comment 3

6 years ago
Comment on attachment 688250 [details] [diff] [review]
patch v2

>=== modified file 'Bugzilla/Bug.pm'

>+    $self->_sync_fulltext(
>+        update_short_desc => $changes->{short_desc},
>+        update_comments   => $self->{added_comments} || $self->{comment_isprivate}
>+    ) if $self->{added_comments} || $changes->{short_desc}
>+         || $self->{comment_isprivate};

Please remove the |if ...| part. Not only this looks ugly, but _sync_fulltext() won't do anything if both update_short_desc and update_comments are false. So this test is useless. r=LpSolit with this fix on checkin.
Attachment #688250 - Flags: review?(LpSolit) → review+

Comment 4

6 years ago
Looks safe for 4.4.
Flags: approval4.4+
Flags: approval+
Keywords: perf
Target Milestone: --- → Bugzilla 4.4
(Assignee)

Comment 5

6 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified importxml.pl
modified sanitycheck.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Migrate.pm
Committed revision 8542.

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bugzilla/4.4/
modified importxml.pl
modified sanitycheck.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Migrate.pm
Committed revision 8495.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.