PHP DB code: clean up the logic, and allow for > 1 readonly replica

[davidpanderson]

Also always use mysqli interface

[brevilo]

Also always use mysqli interface

Please, use atomic commits so that one can review separate topics separately.

[brevilo]

Apart from the above I like the approach. Flexible while not overly complex.

[davidpanderson]

I removed the 'use db_name' query (not needed with mysqli) and made the default 'localhost' explicit.

[brevilo]

@bema-aei should ultimately review this as well (I can't add him).

[bema-aei]

This works fro us in principle, with the change I mentioned above. I updated my original branch with your changes, with two important changes:

• including a change that affects the whole web code ("use mysqli everywhere") in another commit with very limited scope and not even mentioning it in the commit message is just asking for trouble. It's difficult to review and difficult to test. Worst of all, if someone of some project later finds it to cause trouble for him, he is unlikely to ever find the commit that breaks his project. So I split your commit in two (leaving the author intact).
• I added the commit I mentioned above that is required to make this work for us. It allows to address multiple DB hosts in sequence without changing too much code.
• I also changed the debug output to suit my needs, then reverted it. You can re-add it by reverting the 'Revert' commit if needed.

Please have a look at that branch.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 10.53%. Comparing base (f2be5af) to head (da1727c). Report is 161 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5697      +/-   ##
============================================
- Coverage     10.53%   10.53%   -0.01%     
  Complexity     1068     1068              
============================================
  Files           279      279              
  Lines         35874    35891      +17     
  Branches       8412     8417       +5     
============================================
  Hits           3780     3780              
- Misses        31700    31717      +17     
  Partials        394      394              

see 8 files with indirect coverage changes

[AenBleidd]

@bema-aei, @brevilo, are you ok with this PR? Does it need anything else (like testing on your side) or we can move forward and merge it?

[AenBleidd]

@bema-aei, @brevilo, could you please take a look at this PR and test it? Thank you in advance.

[bema-aei]

First of all I like the idea of allowing an arbitrary number of replicas. And thank you for implementing a close() function.

This code in get() is still unchanged, i.e. if $instance is set, get() will return the same instance even if $dbnum is different.

At least this doesn't correspond to what I proposed in my branch. Is the "user" of this code required to close one connection / instance before opening another? If so, it should be noted somewhere, I didn't expect that.

[bema-aei]

Also too I there is an uncaught exception if the DB connection is refused for some reason. I get:

PHP Fatal error:  Uncaught mysqli_sql_exception: Connection refused in /srv/BOINC/live-webcode-test/html/inc/db_conn.inc:37
Stack trace:
#0 /srv/BOINC/live-webcode-test/html/inc/db_conn.inc(37): mysqli->__construct()
#1 /srv/BOINC/live-webcode-test/html/inc/boinc_db.inc(65): DbConn->init_conn()
#2 /srv/BOINC/live-webcode-test/html/inc/boinc_db.inc(117): BoincDb::get_aux()
#3 /srv/BOINC/live-webcode-test/html/ops/eah_server_status.php(681): BoincDb::get()
#4 /srv/BOINC/live-webcode-test/html/ops/eah_server_status.php(1108): seconds_behind_master()
#5 {main}
  thrown in /srv/BOINC/live-webcode-test/html/inc/db_conn.inc on line 37

[davidpanderson]

• I added the check for different dbnum in get()
• Yes, you can only use on DB at a time
• An exception would be thrown if had previously called mysqli_report(MYSQLI_REPORT_STRICT); presumably because you want to handle exceptions. But in any case I added a try/catch.

[bema-aei]

Thanks, this works for us technically. We do use a recent MariaDB, which has some defaults different from MySQL, hence the exception.

However, for reasons I pointed out already I can't really approve the first commit which combines different features w/o even mentioning in the commit message, so I can't recommend to merge this branch as it is.

[brevilo]

We do use a recent MariaDB, which has some defaults different from MySQL, hence the exception.

How does a PHP module's (mysqli) config relate to the MySQL flavor used as the DB backend?

[bema-aei]

We do use a recent MariaDB, which has some defaults different from MySQL, hence the exception.

How does a PHP module's (mysqli) config relate to the MySQL flavor used as the DB backend?

Oh. Sorry, was mislead. Indeed, that's the mysqli config.