Closed Bug 498890 Opened 12 years ago Closed 9 years ago

Bugzilla::User::Setting doesn't need to sort DB results

Categories

(Bugzilla :: User Accounts, defect)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Bugzilla 4.4

People

(Reporter: bbaetz, Assigned: koosha.khajeh)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Bugzilla::User::Setting uses ORDER BY for its queries, before stuffing the results into an (unordered) hash. There's no point in that.

As well, to get a user's defaults just do a left join (which we are doing anyway) rather than two queries and passes.

Also only grab $dbh when we are going to use it.

(None of this shows up in profiles directly; its just some stuff I noticed while tracking down some of the actual perf issues)
Attachment #383701 - Flags: review?(mkanat)
Comment on attachment 383701 [details] [diff] [review]
Patch

> sub new {

>-    my $dbh = Bugzilla->dbh;

>     if (scalar @_ == 0) {
>+        my $dbh = Bugzilla->dbh;

Please revert that. It's usually a good idea to keep $dbh defined at the top of the subroutine, so that we can reuse it later if needed. Having it defined here doesn't hurt *at all*.


> sub get_all_settings {

>     my $sth = $dbh->prepare(

While you are doing some cleanup here, could you please replace these prepare(), execute(), fetch() calls by a single selectall_arrayref() call as we do everywhere else?


> sub get_defaults {

>     my $sth = $dbh->prepare(q{SELECT name, default_value, is_enabled, subclass

Same here.
Target Milestone: --- → Bugzilla 3.6
Attachment #383701 - Flags: review?(mkanat) → review-
Comment on attachment 383701 [details] [diff] [review]
Patch

>Index: Bugzilla/User/Setting.pm
>-    my $dbh = Bugzilla->dbh;
>-

  No particular reason to do that.

> sub get_all_settings {
>     my ($user_id) = @_;
>-    my $settings = get_defaults($user_id); # first get the defaults
>     my $sth = $dbh->prepare(
>            q{SELECT name, default_value, is_enabled, setting_value, subclass
>                FROM setting
>           LEFT JOIN profile_setting
>                  ON setting.name = profile_setting.setting_name
>-              WHERE profile_setting.user_id = ?
>-           ORDER BY name});
>+                    AND profile_setting.user_id = ?});

  No reason to convert the WHERE into an ON condition.
(In reply to comment #2)

> > sub get_all_settings {
> >     my ($user_id) = @_;
> >-    my $settings = get_defaults($user_id); # first get the defaults
> >     my $sth = $dbh->prepare(
> >            q{SELECT name, default_value, is_enabled, setting_value, subclass
> >                FROM setting
> >           LEFT JOIN profile_setting
> >                  ON setting.name = profile_setting.setting_name
> >-              WHERE profile_setting.user_id = ?
> >-           ORDER BY name});
> >+                    AND profile_setting.user_id = ?});
> 
>   No reason to convert the WHERE into an ON condition.

Err, that the point of the patch. With the constraint as a WHERE, it only finds user-modified settings, so we have to call get_default_settings first, which creates a Bugzilla::User::Setting object for each one, and then we override that for the user settings (ie its the same as an INNER JOIN). With this patch the one query will just have NULL for the outer joins, so we can create default and user settings in the one loop

(::new could use a similar change but AFAICS noone ever uses that with a user_id passed in but not the whole set of values...)
Comment on attachment 383701 [details] [diff] [review]
Patch

Okay. In that case, just address the other comments from myself and LpSolit and we should be good to go.
Target Milestone: Bugzilla 3.6 → Bugzilla 3.8
Target Milestone: Bugzilla 4.0 → Bugzilla 4.2
Target Milestone: Bugzilla 4.2 → Bugzilla 5.0
Attached patch patch (obsolete) — Splinter Review
Uploading a new patch after 3 years.
Assignee: bbaetz → koosha.khajeh
Attachment #383701 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #638651 - Flags: review?(glob)
Attachment #638651 - Flags: review?(LpSolit)
Attachment #638651 - Flags: review?(glob)
Comment on attachment 638651 [details] [diff] [review]
patch

>+    my $rows = $dbh->selectall_arrayref(
>            q{SELECT name, default_value, is_enabled, setting_value, subclass
>                FROM setting
>           LEFT JOIN profile_setting
>                  ON setting.name = profile_setting.setting_name
>+                AND profile_setting.user_id = ?});

Looks like this patch hasn't been tested. You define a placeholder but doesn't pass any value for it.
Attachment #638651 - Flags: review?(LpSolit) → review-
Sorry. I was drowsy at the time of writing!
Attached patch patch - v1.1Splinter Review
Tested.
Attachment #638651 - Attachment is obsolete: true
Attachment #639028 - Flags: review?(LpSolit)
Comment on attachment 639028 [details] [diff] [review]
patch - v1.1

Looks good and seems to work fine. r=LpSolit
Attachment #639028 - Flags: review?(LpSolit) → review+
Flags: approval+
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/User/Setting.pm
Committed revision 8310.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.