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

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Bradley Baetz (:bbaetz)]

Attachment: Patch

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)

Comment 1

[Frédéric Buclin]

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.

Comment 2

[Max Kanat-Alexander]

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.

Comment 3

[Bradley Baetz (:bbaetz)]

(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 4

[Max Kanat-Alexander]

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.

Comment 5

[Koosha KM]

Attachment: patch

Uploading a new patch after 3 years.

Comment 6

[Frédéric Buclin]

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.

Comment 7

[Koosha KM]

Sorry. I was drowsy at the time of writing!

Comment 8

[Koosha KM]

Attachment: patch - v1.1

Tested.

Comment 9

[Frédéric Buclin]

Comment on attachment 639028 [details] [diff] [review]
patch - v1.1

Looks good and seems to work fine. r=LpSolit

Comment 10

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/User/Setting.pm
Committed revision 8310.