530270 - Whining fails if mail queueing is enabled

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Dave Miller [:justdave]]

whine.pl is still using the shadow database at the point where it tries to send mail.  This causes it to try to insert into the slave when adding to the queue, and the insert fails.

Inserting a send_mail job into the Job Queue failed with the following
error: Failed to execute INSERT INTO ts_job (funcid, arg, uniqkey,
insert_time, run_after, grabbed_until, priority, coalesce) VALUES (?,
?, ?, ?, ?, ?, ?, ?) with funcid, arg, uniqkey, insert_time, run_after,
grabbed_until, priority, coalesce: DBD::mysql::st execute failed: The
MySQL server is running with the --read-only option so it cannot
execute this statement at
/usr/lib/perl5/vendor_perl/5.8.8/Data/ObjectDriver/Driver/DBI.pm line
380.

The proper place to fix this is probably in the queue management code.  The queue insert routines should never be talking to the slave database.

Comment 1

[Max Kanat-Alexander]

Yeah, agreed with your suggested resolution. We should have an on_main_database routine that takes a block and performs the code using the main database.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Create Bugzilla->main_dbh, which returns the main DB handle without switching to it.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 413823 [details] [diff] [review]
patch, v1

Let's call it dbh_main, consistently. (It has two names in this patch, main_dbh and dbh_main.)

Also, I was thinking that instead, we'd create a function called on_main_db(&) that takes a block and makes Bugzilla->dbh be the main db for that block. I think that might be more useful in the general case, because that would also make any subroutines called use the main dbh.

Comment 4

[Bradley Baetz (:bbaetz)]

Please add a comment to JobQueue.pm explaining this.

Also, this is going to make the QueueRunner run on the main db too, but I don't think that that's a problem - certainly won't make any difference for the current job we run

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> (From update of attachment 413823 [details] [diff] [review])
> Let's call it dbh_main, consistently. (It has two names in this patch, main_dbh
> and dbh_main.)

Haha, yes, I wrote it quickly. :)


> Also, I was thinking that instead, we'd create a function called on_main_db

This wouldn't be really useful as we already have switch_to_main_db() for that. I will just update my patch to fix the function name.

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

Comment 7

[Max Kanat-Alexander]

(In reply to comment #5)
> This wouldn't be really useful as we already have switch_to_main_db() for that.

  It's true, but that doesn't do this:

  1) Are we running on the shadow? If so, switch to the main and remember that we were running on the shadow.
  2) At the end of the block, switch back to using the shadow, only if we were on the shadow before.

  It's not a *lot* of code, but it would simplify using the main dbh in a lot of places.

  For this bug, though, I agree, main_dbh is all we need.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 413934 [details] [diff] [review]
patch, v2

>Index: Bugzilla/JobQueue.pm
>+    $class->request_cache->{dbh} = $class->dbh_main;
>+    return $class->{dbh_main};

  That needs to be $class->dbh_main, on the last line there.

  Otherwise this looks fine.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> >+    $class->request_cache->{dbh} = $class->dbh_main;
> >+    return $class->{dbh_main};
> 
>   That needs to be $class->dbh_main, on the last line there.

This doesn't *need* to be, as all it would do would be to return ->{dbh_main}, which I do explicitly here.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> This doesn't *need* to be, as all it would do would be to return ->{dbh_main},
> which I do explicitly here.

  No, it does. There is no such thing as $class->{dbh_main}. $class is a string.

Comment 11

[Frédéric Buclin]

(In reply to comment #9)
> This doesn't *need* to be, as all it would do would be to return ->{dbh_main},
> which I do explicitly here.

Ah right, I meant $class->request_cache->{dbh_main}. I will use $class->dbh_main, then.

Comment 12

[Frédéric Buclin]

tip:

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.84; previous revision: 1.83
done
Checking in Bugzilla/JobQueue.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v  <--  JobQueue.pm
new revision: 1.4; previous revision: 1.3
done


3.4.4:

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.73.2.3; previous revision: 1.73.2.2
done
Checking in Bugzilla/JobQueue.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v  <--  JobQueue.pm
new revision: 1.1.2.1; previous revision: 1.1
done