285397 - Untested parts of Bugzilla::DB are broken (when running on PostgreSQL)

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Max Kanat-Alexander]

So, I have a running PostgreSQL dbcompat installation, now (more on that later).

It showed me that certain untested parts of Bugzilla::DB and Bugzilla::DB::Pg
were broken. :-) So, patch coming up.

Comment 1

[Max Kanat-Alexander]

Attachment: Simple fixes

Looks like we forgot to remove the Carp stuff (we don't use Carp) and set some
variables. :-)

Comment 2

[Tomas Kopal]

Comment on attachment 176833 [details] [diff] [review]
Simple fixes

Yeah, I know about this, I just didn't find the time to raise the bug yet,
sorry :-).

Personally, I like having the carp there, as without it, you are basically left
with just red text in html and no idea what went wrong. With carp, you have
descriptive message in your webserver log with location, where it happened.
But if Bugzilla policy is no Carp, I am ok with that :-).

>Index: Bugzilla/DB/Pg.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v
>retrieving revision 1.3
>diff -u -r1.3 Pg.pm
>--- Bugzilla/DB/Pg.pm	8 Mar 2005 23:23:30 -0000	1.3
>+++ Bugzilla/DB/Pg.pm	9 Mar 2005 05:24:34 -0000
>@@ -173,6 +173,7 @@
>         Bugzilla->dbh->do('LOCK TABLE ' . join(', ', keys %write_tables) .
>                           ' IN ROW EXCLUSIVE MODE') if keys %write_tables;
>     }
>+    $self->{private_bz_tables_locked} = 1;
> }

I would move this before the closing brace, it's clearer then what it means.
But the result is the same, so no big deal.

>@@ -193,6 +192,7 @@
>             $self->bz_commit_transaction();
>         }
>     }
>+    $self->{private_bz_tables_locked} = 0;
> }
> 
> 1;

No, you want to set this variable before actually calling rollback or commit,
otherwise you can end up with an endless recursion (tested ;-) ). If rollback
or commit ends up with error, it will call ThrowCodeError, which in turn calls
unlock(abort), which will call rollback, which wil in turn call... You got the
idea ;-).

Comment 3

[Max Kanat-Alexander]

Attachment: v2

OK, fixed it.

The carps aren't necessary; we have data/errorlog

Comment 4

[Tomas Kopal]

Comment on attachment 176854 [details] [diff] [review]
v2

Functional changes are OK now, but if you want to cleanup carp, you need to do
that properly. Both Pg.pm and Mysql.pm contains "use Carp;", Mysql still
contains carp calls.

Comment 5

[Max Kanat-Alexander]

Attachment: v3

Comment 6

[Tomas Kopal]

Comment on attachment 176860 [details] [diff] [review]
v3

Now it rocks :-).

Comment 7

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.29; previous revision: 1.28
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.6; previous revision: 1.5
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.5; previous revision: 1.4
done