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

RESOLVED FIXED in Bugzilla 2.20

Status

()

P1
major
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 1

14 years ago
Posted patch Simple fixes (obsolete) — Splinter Review
Looks like we forgot to remove the Carp stuff (we don't use Carp) and set some
variables. :-)
Attachment #176833 - Flags: review?(Tomas.Kopal)

Comment 2

14 years ago
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 ;-).
Attachment #176833 - Flags: review?(Tomas.Kopal) → review-
(Assignee)

Comment 3

14 years ago
Posted patch v2 (obsolete) — Splinter Review
OK, fixed it.

The carps aren't necessary; we have data/errorlog
Attachment #176833 - Attachment is obsolete: true
Attachment #176854 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #176854 - Flags: review? → review?(Tomas.Kopal)

Comment 4

14 years ago
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.
Attachment #176854 - Flags: review?(Tomas.Kopal) → review-
(Assignee)

Comment 5

14 years ago
Posted patch v3Splinter Review
Attachment #176854 - Attachment is obsolete: true
Attachment #176860 - Flags: review?(Tomas.Kopal)

Comment 6

14 years ago
Comment on attachment 176860 [details] [diff] [review]
v3

Now it rocks :-).
Attachment #176860 - Flags: review?(Tomas.Kopal) → review+
(Assignee)

Updated

14 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 7

14 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.