add an index to fielddefs on (custom,type,sortkey);

RESOLVED WONTFIX

Status

()

Bugzilla
Database
--
enhancement
RESOLVED WONTFIX
6 years ago
6 years ago

People

(Reporter: glob, Assigned: dkl)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
in bug 824998 sheeri has determined that the fielddefs table would benefit from an index on (custom,type,sortkey).
(Assignee)

Comment 1

6 years ago
Created attachment 697934 [details] [diff] [review]
Patch to update unique index on fielddefs table (v1)
Assignee: database → dkl
Status: NEW → ASSIGNED
Attachment #697934 - Flags: review?(glob)

Comment 2

6 years ago
Comment on attachment 697934 [details] [diff] [review]
Patch to update unique index on fielddefs table (v1)

>+            fielddefs_name_idx => {FIELDS => [qw(name type sortkey)],

We want (*custom* type sortkey), not name.
Attachment #697934 - Flags: review?(glob) → review-
(Assignee)

Comment 3

6 years ago
(In reply to Frédéric Buclin from comment #2)
> Comment on attachment 697934 [details] [diff] [review]
> Patch to update unique index on fielddefs table (v1)
> 
> >+            fielddefs_name_idx => {FIELDS => [qw(name type sortkey)],
> 
> We want (*custom* type sortkey), not name.

Ugh. Yeah completely missed that. New patch coming.
(Assignee)

Comment 4

6 years ago
Created attachment 698015 [details] [diff] [review]
Patch to add unique index to fielddefs table (v2)
Attachment #697934 - Attachment is obsolete: true
Attachment #698015 - Flags: review?(LpSolit)

Comment 5

6 years ago
Comment on attachment 698015 [details] [diff] [review]
Patch to add unique index to fielddefs table (v2)

>+            fielddefs_custom_idx  => {FIELDS => [qw(custom type sortkey)],
>+                                      TYPE => 'UNIQUE'},

Not UNIQUE! You can have two custom fields of the same type (e.g. Free Text) with the same sortkey. I'm surprised checksetup.pl didn't crash in your testing. :)
Attachment #698015 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 698098 [details] [diff] [review]
Patch to add new index to fielddefs table (v3)
Attachment #698015 - Attachment is obsolete: true
Attachment #698098 - Flags: review?(LpSolit)

Comment 7

6 years ago
I scanned our codebase, and I cannot find any place where queries of the form

 WHERE custom = 1 AND type = X

as described in bug 824998 are triggered. I added debug code into Bugzilla::Object::_do_list_select() to intercept queries generated by it, but none of these queries are of the form above. There are tons of queries of the form:

 WHERE visibility_field_id = X

and

 WHERE value_field_id = X

though. Those are generated by [% legal_value.controlled_values %] and [% field.controls_visibility_of %] in bug/field-events.js.tmpl.

I don't want to add a new index if we cannot determine which code will benefit from it.

Comment 8

6 years ago
(In reply to Frédéric Buclin from comment #7)
> I scanned our codebase, and I cannot find any place where queries of the form
> 
>  WHERE custom = 1 AND type = X
> 
> as described in bug 824998 are triggered.


Ah, I found it! This comes from Bugzilla::Bug::AUTOLOAD in Bugzilla 4.0:

      $self->{_multi_selects} ||= [Bugzilla->get_fields(
          {custom => 1, type => FIELD_TYPE_MULTI_SELECT })];

But AUTOLOAD is gone since Bugzilla 4.2, see bug 600123. That was the single place doing such queries. So there is no benefit in adding this new index in Bugzilla 4.2 and newer. Suggesting WONTFIX.

Comment 9

6 years ago
Comment on attachment 698098 [details] [diff] [review]
Patch to add new index to fielddefs table (v3)

This index is now irrelevant in Bugzilla 4.2 and newer, see my previous comment.
Attachment #698098 - Flags: review?(LpSolit)
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
great detective work....glad it's fixed in the new version!
You need to log in before you can comment on or make changes to this bug.