286501 - Summarize time fails with "Can't bind reference" error

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Tomas Kopal]

While testing Postgres port, I tried the 'summarize time' link for a bug (with
no dependencies). I got error 'Can't bind a reference (ARRAY(0x1fd247c)) at
C:/Perl/site/lib/DBI.pm line 1577.'.
I don't know if this is Postgres specific, I can't test it on MySQL ATM, but it
looks like perl problem, not DB.
Patch follows.

Comment 1

[Tomas Kopal]

Attachment: V1

Added dereferencing of the returned array reference. Also added one missing
column to groupby for postgres (if you would prefer this as a separate
bug/patch, let me know, I will remove it).
Also, the same Bug.pm method (EmitDependList) is used inside of Bug.pm, can
someone with better knowledge of Bug.pm internals check if the same problem
apply there?

Comment 2

[Tomas Kopal]

Attachment: V1.1

Oooops, sorry, forget to diff unified :-).

Comment 3

[Tomas Kopal]

Hmmm, as I am thinking about it, is this safe, when the function is calling
itself recursively? Will the referenced values be dereferenced in the loop prior
to recursion, or will the next recursion overwrite the values for the first one?
Maybe we need to copy the array first?
Sorry, I am far from being an expert on perl references :-).

Comment 4

[Max Kanat-Alexander]

EmitDependList was not exported from Bugzilla::Bug on purpose -- it was intended
to be used only within Bugzilla::Bug. Sorry that's not clear -- there are no POD
docs for Bugzilla::Bug.

Nobody outside of Bugzilla::Bug should be using EmitDependList. Instead they
should be using ->blocked or ->dependson on a Bug object.

summarize_time should probably be using Bug objects anyhow, I'd imagine.

However, for now the fix looks correct. But what's up with that group_by change
below it?

Comment 5

[Max Kanat-Alexander]

Also, as part of this patch, please add a comment above EmitDependList that says
it's private and is not intended for use outside of Bugzilla::Bug.

Comment 6

[Max Kanat-Alexander]

Oh, and to answer your question -- no, the recursion is not a problem.
EmitDependList returns a different arrayref every time.

Comment 7

[Tomas Kopal]

(In reply to comment #4)
> However, for now the fix looks correct.

Well, while we are at that, why not fix it properly? :-). I will try to change
it to use the Bug object. Do you think that the overhead of constructing bug
objects recursively just to find out dependecies is not prohibitive?

> But what's up with that group_by change
> below it?

As I said, I found it when testing Postgres port. One column was missing in the
group_by and postgres was complainig. If you don't like it here, I can easily
create a new bug for it :-).

Comment 8

[Tomas Kopal]

Attachment: V2

New version, using Bug object to get the list of dependsons. Also added comment
to the Bug.pm as requested.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 177685 [details] [diff] [review]
V2

Yes, creating a Bug object inside of a recursiev function is definitely too
heavy. :-) Instead, the function should have a bunch of Bug objects passed in
to it.

The *correct* way to fix this (and not something we want to do while frozen) is
to make *all* of summarize_time use Bug objects instead of bug id's.

Comment 10

[Max Kanat-Alexander]

And the even *more* correct way to fix this is bug 283876.

Comment 11

[Max Kanat-Alexander]

Oops, I meant bug 286509.

Comment 12

[Tomas Kopal]

(In reply to comment #9)
> (From update of attachment 177685 [details] [diff] [review] [edit])
> Yes, creating a Bug object inside of a recursiev function is definitely too
> heavy. :-) Instead, the function should have a bunch of Bug objects passed in
> to it.
> 
> The *correct* way to fix this (and not something we want to do while frozen) is
> to make *all* of summarize_time use Bug objects instead of bug id's.
> 

I am afraid I don't understand what you mean. The function I am trying to fix is
traversing the tree of dependencies and gathering their ids for later use in a
SELECT.
Yes, we can convert the whole summarize_time to use Bug class, but that will not
remove the need to create instance of the class for every single bug in the
dependency tree. It may be a little faster as some bug parameters will not have
to be queried from the DB in summary_time (as they were queried at bug
construction time), but you still don't have the information you need in Bug
instance, so you'll need to do the final queries anyway. And IMHO it's always
faster to do one query with a list of ids, than to do a query for every bug
separately.
If the construction is too heavy, we can't use Bug class in sumarize_time, or we
need to make it lighter. Maybe by making *all* bug properties lazily evaluated?

But I agree this is not a fix for freeze time. I will revert to the previous
solution.

Comment 13

[Tomas Kopal]

Attachment: V3

Reverted to use EmitDependList.

Comment 14

[Max Kanat-Alexander]

Yeah, you're right. I have to come up with some way to handle this situation in
a performant fashion.

Also, you know... that function look like it will infinite loop on dependency
lists...

Comment 15

[Tomas Kopal]

(In reply to comment #14)
> Also, you know... that function look like it will infinite loop on dependency
> lists...

Dependency lists? Do you mean dependecy loops? I tought they are not allowed to
exists, that the code checks for circular deps when you are entering the dependency?
If not, then this will definitelly freeze and probably crash on memory
exhaustion (for mod_perl, that could be even a security problem).

Comment 16

[Max Kanat-Alexander]

Comment on attachment 177690 [details] [diff] [review]
V3

Cool.

We also need to file a bug about how that function will die on a dependency
loop, as far as I can see.

Comment 17

[Max Kanat-Alexander]

Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <-- 
summarize_time.cginew revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.70; previous revision: 1.69
done

Comment 18

[Christian Reis]

(In reply to comment #4)
> summarize_time should probably be using Bug objects anyhow, I'd imagine.

The issue was that generating this report using Bug objects, for large bug trees
(thousands of bugs), made the report unusably slow.

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> The issue was that generating this report using Bug objects, for large bug trees
> (thousands of bugs), made the report unusably slow.

  That should no longer be a big issue, now that I've sped up the creation of
Bug objects considerably.

Comment 20

[Christian Reis]

I'm not so sure, but I don't have the cycles to check if that's true or not at
the moment.