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

Status: RESOLVED FIXED

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

Description

[Joel Peshkin]

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.

Comment 1

[Joel Peshkin]

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

Any objections?

Comment 2

[Dave Miller [:justdave]]

makes sense to me.

Comment 3

[Gervase Markham [:gerv]]

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

Comment 4

[Joel Peshkin]

Attachment: The patch

I really wish that I could just access the main db's dbh instead of calling the
switch functions.

Comment 5

[Jouni Heikniemi]

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?

Comment 6

[Joel Peshkin]

Attachment: v2

This addresses the 2 code changes in Joini's review.  It addresses only
scenario B consistent with the point Gerv made.

Comment 7

[Jouni Heikniemi]

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

Comment 8

[Jouni Heikniemi]

-> patch author

Comment 9

[Joel Peshkin]

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.19; previous revision: 1.18
done