811280 - Add a caching mechanism to Bugzilla::Object to avoid querying the database repeatedly for the same information

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[:glob ✱]

Bugzilla::Bug->new() is called multiple times when displaying a bug.
each call results in a round trip to the database to fetch the bug's row.
this is inefficient.

i picked a bug at random from my test database, and it resulted in 13 calls to Bugzilla::Bug->new($bug_id), where $bug_id is the is of the bug i was viewing.

the breakdown is:

1 from show_bug.cgi
1 for each attachment (via Attachment->flag_types)
1 for each get_bug_link (this bug happened to have a lot of self-referential comments)
2 for each bug_format_comment (splinter extension)


we should only load a bug once from the database, then reuse the same object.

Comment 1

[:glob ✱]

Attachment: patch v1

tested with landfill's database, bug 9855, mod_cgi

without patch: average 1286ms
   with patch: average 808ms

Comment 2

[Frédéric Buclin]

Comment on attachment 681024 [details] [diff] [review]
patch v1

You cannot cache bug objects this way. On bug creation, $self->{error} is set based on user privs. Same for $self->{choices}, $self->{tags}, $self->{actual_time}, etc... which are all user-dependent. Once a bug object is cached, it can be shared, and you may access data from someone else if the user accessing the data changes (think bugmails). This would open the door for many new security bugs.

Comment 3

[:glob ✱]

this is a bug, not an enhancement.

Comment 4

[:glob ✱]

(In reply to Frédéric Buclin from comment #2)
> You cannot cache bug objects this way. On bug creation, $self->{error} is
> set based on user privs. Same for $self->{choices}, $self->{tags},
> $self->{actual_time}, etc... which are all user-dependent. Once a bug object
> is cached, it can be shared, and you may access data from someone else if
> the user accessing the data changes (think bugmails). This would open the
> door for many new security bugs.

as far as i can tell, when Bugzilla->new() is called, Bugzilla->user is always the current user.  $user.can_see_bug() is used to determine visibility, including during bugmail generation.  Bugzilla.set_user() isn't called that much at all.

regardless, i'll provide an updated patch which includes the user's id as part of the cache's key.

Comment 5

[:glob ✱]

Attachment: patch v2

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 681073 [details] [diff] [review]
patch v2

Review of attachment 681073 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 7

[Frédéric Buclin]

Comment on attachment 681073 [details] [diff] [review]
patch v2

Has this patch been tested? You can no longer edit bugs. All changes are ignored.

Comment 8

[Frédéric Buclin]

Also, this patch doesn't let us recreate a bug object using the same bug ID, which is a problem in process_bug.cgi:

        if ($action eq 'same_bug') {
            # $bug->update() does not update the internal structure of
            # the bug sufficiently to display the bug with the new values.
            # (That is, if we just passed in the old Bug object, we'd get
            # a lot of old values displayed.)
            $bug = new Bugzilla::Bug($bug->id);
            $vars->{'bug'} = $bug;
        }

We need to be able to invalidate the cache, else the code above won't work anymore.

Comment 9

[:glob ✱]

(In reply to Frédéric Buclin from comment #7)
> Has this patch been tested? You can no longer edit bugs. All changes are
> ignored.

yes, i tested this bug :|

looks like the bug is displayed with the updated values after you update it, but the database isn't actually updated.

Comment 10

[Frédéric Buclin]

Same problem in attachment.cgi:

  # We cannot reuse the $bug object as delta_ts has eventually been updated
  # since the object was created.
  $vars->{'bugs'} = [new Bugzilla::Bug($bugid)];

And that's only the two places I have in mind. I didn't check all places where we (re)create a bug object. Your fix is prone to regressions.

Comment 11

[:glob ✱]

Attachment: patch v3

while i still believe the default behavour for bugzilla should be to avoid re-creating objects from the database if we already have the data, this patch follows lpsolit's suggested path.

this patch extends bugzilla::object which allows the caller to indicate they would be happy with a cached object.  eg.
  $self->{product_obj} ||=
    new Bugzilla::Product({ id => $self->{product_id}, cache => 1 });

it also updates all places where we'd benefit from a cached object while viewing a bug or attachment.

Comment 12

[Reed Loden [:reed]]

(In reply to Byron Jones ‹:glob› from comment #11)
> while i still believe the default behavour for bugzilla should be to avoid
> re-creating objects from the database if we already have the data

+1... we should cache by default and allow objects to do cache => 0 if they absolutely need to bypass the cache.

Comment 13

[Dave Miller [:justdave]]

(In reply to Reed Loden [:reed] from comment #12)
> +1... we should cache by default and allow objects to do cache => 0 if they
> absolutely need to bypass the cache.

we would likely need to heavily audit the usage if we implement this way...  caching by default is asking for data leakage if one place is attempting to filter on what the user's allowed to see and another isn't.

Comment 14

[:glob ✱]

(In reply to Dave Miller from comment #13)
> we would likely need to heavily audit the usage if we implement this way... 
> caching by default is asking for data leakage if one place is attempting to
> filter on what the user's allowed to see and another isn't.

agreed, however bear in mind we're just caching the individual objects, and for bugs the cache key includes the user's id.  by and large the visibility of data is governed not by the objects themselves, rather by the consumers, so they shouldn't be impacted by caching.

Comment 15

[Dave Miller [:justdave]]

Good to know we're being that smart with the cache objects :)

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 683001 [details] [diff] [review]
patch v3

Review of attachment 683001 [details] [diff] [review]:
-----------------------------------------------------------------

I do not see where you are including the user id with the cache_key value which was discussed in previous comments as being needed.

::: Bugzilla/Object.pm
@@ +117,4 @@
>      return $object;
>  }
>  
> +# Provides a mechanism for objects to be cached in the request_cahce

request_cache

Comment 17

[:glob ✱]

Attachment: patch v4

odd; i definitely remember writing that code :|
no matter, here an updated patch which includes the missing sub.

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 683586 [details] [diff] [review]
patch v4

Review of attachment 683586 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

Comment 19

[Frédéric Buclin]

It's too late for 4.4, and my comments in this bug show that this code is prone to regressions. Security is also something which needs to be investigated carefully as it may result in leaks at unexpected places. This means much more testing than what I want to do post-RC.

Comment 20

[:glob ✱]

(In reply to Frédéric Buclin from comment #19)
> It's too late for 4.4, and my comments in this bug show that this code is
> prone to regressions.

this statement isn't accurate -- your prior comments relate only to earlier patch versions, which enable caching by default.

at your request, the latest patch does caching on demand, which has *significant* less risk with regards to regressions.

Comment 23

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified attachment.cgi
modified show_bug.cgi
modified Bugzilla/Attachment.pm
modified Bugzilla/Bug.pm
modified Bugzilla/Comment.pm
modified Bugzilla/Object.pm
modified Bugzilla/Product.pm
modified Bugzilla/Template.pm
Committed revision 8483.

Comment 24

[Frédéric Buclin]

This was already added to relnotes for 5.0rc1. Forgot to report this here.