User->groups may have a hazard when shadow DBs are in use

RESOLVED FIXED in Bugzilla 2.18

Status

()

defect
P3
normal
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: bugreport, Assigned: bugreport)

Tracking

2.17.7
Bugzilla 2.18
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
718 bytes, patch
jouni
: review+
Details | Diff | Splinter Review
Assignee

Description

16 years ago
Scenario A:
It appears that a query or collection of series data could fail to get current
group information in the following scenario....

1) Group information in the user_group_map is stale (perhaps the user has never
logged in, like a pseudo_user used for series data collection)
2) Replication is delayed
3) As user logs in, group information in master is refreshed, but not update in
the slave
4) The query runs, pulling the user's group list from the slave and misses bugs.

Scenario B:
1) collectstats switches to shadow db
2) collectstats calls Search.pm with a stale user's userid
3) Search.pm calls User->groups, which rederives user's groups and writes to
_dbh wchich currently points to the shadow db.
4) Failure???

I suspect the right thing to do is to have User.pm ALWAYS use the main db.
Assignee

Comment 1

16 years ago
Even though nobody has reported an actual failure to this, I am convinced that
we should make this change.  

Any objections?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.18
I don't understand A) - what is is pseudo_user used for series data collection?
I didn't have such a concept when I wrote the code.

I don't see missing bugs in the series count due to a stale shadow as a problem
- that's just what happens when you have a shadow.

B) sounds more worrying - although I'd say it's an architectural problem that
asking what a user's groups are causes database writes... but I doubt that can
be changed.

Gerv


Assignee

Comment 4

15 years ago
Posted patch The patch (obsolete) — Splinter Review
I really wish that I could just access the main db's dbh instead of calling the
switch functions.
Assignee

Updated

15 years ago
Attachment #146689 - Flags: review?

Comment 5

15 years ago
Comment on attachment 146689 [details] [diff] [review]
The patch

>     if ($result) {
>+        my $noshadow;
>+        unless ($noshadow = Bugzilla->dbwritesallowed()) {
>+            Bugzilla->switch_to_main();
>+        }
>         $self->derive_groups($tables_locked_for_derive_groups);
>+        unless ($noshadow) {
>+            Bugzilla->switch_to_shadow();
>+        }

1. I think those should be switch_to_main_db and ..._shadow_db? (note the _db
part)

2. "noshadow" is confusing; negation in a variable name tends to do that.
"is_main_db" is much better, for example.

3. Does this fix scenario A? Groups are derived correctly on the main db, but
what is replication is delayed, and rest of in_group queries get done from the
slave server?
Attachment #146689 - Flags: review? → review-
Assignee

Comment 6

15 years ago
Posted patch v2Splinter Review
This addresses the 2 code changes in Joini's review.  It addresses only
scenario B consistent with the point Gerv made.
Attachment #146689 - Attachment is obsolete: true
Assignee

Updated

15 years ago
Attachment #148455 - Flags: review?

Comment 7

15 years ago
Comment on attachment 148455 [details] [diff] [review]
v2

I can't really test this (I don't have a shadowing configuration available),
but as far as I can tell, it should be ok. 

r=jouni
Attachment #148455 - Flags: review? → review+

Comment 8

15 years ago
-> patch author
Assignee: justdave → bugreport
Flags: approval?
Flags: approval? → approval+
Assignee

Comment 9

15 years ago
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.19; previous revision: 1.18
done
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.