PHP backup log table from MySQL

Question

I have been working on a little project to tidy up my log table which has exceeded the shared host provider limits:) So I have done some script that is working except one thing at the end: optimise DB.

This script will run in cron so I hope security should not be issue.

My questions are:

1. the code is working, however I would like to learn how to do it better, so If you could point out thing that I shouldn't do and I did I would appreciate
2. if someone could tell me why my optimise DB doesn't work I would also thanks.
3. Performance!!! It will run on a table which has 1million rows and I am worried that his script wont perform fast enough. Any idea how to work this out?

I know I should use PDO (next step - as I am not familiar with it yet) but as a first step I am proud that I could put together something that is working.

<?php
include 'opendb.php';

//functions
function test($string){
  echo "<p>".$string."</p>";
}

function db_rows($db,$ord){
  $dbquery="SELECT azon FROM $db ORDER BY azon $ord LIMIT 1";
  $dbresult=mysql_query($dbquery);
  $row = mysql_fetch_array($dbresult);
  $dbrow = $row['azon'];
  return $dbrow;
}
// end of functions

//config information...
$acttable  = 'foo';
$today = date("yW_Hi");
$newdb = 'test_'.$today;

$firstact= db_rows($acttable,"asc");
$lastact= db_rows($acttable,"desc");
$upto=$firstact+25000;

test($lastact);
test($firstact);


if ($lastact-$firstact>50000) {

//create a new table 
$newdbsql=" CREATE TABLE  $newdb ( `azon` bigint(20) NOT NULL AUTO_INCREMENT,  `mikor` datetime NOT NULL,  `felhazon` int(11) NOT NULL,  `felhnev` varchar(255) NOT NULL,  `muvelet` varchar(20) NOT NULL,  `sql` varchar(255) NOT NULL,  `tabla` varchar(255) NOT NULL,  `mezok` varchar(255) NOT NULL,  `ertekek` text NOT NULL,  `feltetel` varchar(255) NOT NULL,  `ip` varchar(255) NOT NULL,  `bongeszo` varchar(255) NOT NULL,  PRIMARY KEY (`azon`),  KEY `mikor` (`mikor`),  KEY `felhazon` (`felhazon`),  KEY `felhnev` (`felhnev`),   KEY `muvelet` (`muvelet`),  KEY `tabla` (`tabla`),  KEY `feltetel` (`feltetel`)) ENGINE=MyISAM  DEFAULT CHARSET=utf8 AUTO_INCREMENT=3524772 ;" ;
test($newresult);
$newresult = mysql_query($newdbsql);


// copy all the data
$query = "INSERT INTO $newdb SELECT * FROM $acttable WHERE $acttable.azon < $upto";
test($query);
$result = mysql_query($query);

// so what has happened...
if ($result) {
   test("ok");
  $delquery = "DELETE FROM $acttable WHERE $acttable.azon < $upto";
  test($delquery);
  $delresult = mysql_query($delquery);
  if ($delresult) {
     test("Deleted rows - OK");
  } else {
     test("failed to delete...");
     // then tidy up everything:)
    $res = mysql_query('SHOW TABLE STATUS WHERE Data_free / Data_length > 0.1 AND Data_free >  102400');
    while($optrow = mysql_fetch_assoc($res)) {
             mysql_query('OPTIMIZE TABLE ' . $optrow['Name']);
         }
      }
   }
   else {
      test("failed to copy...");
   }

}
else {
  test("no work needs to be done.");
}


// close db
mysql_close($conn);
?>

Answer 1

1. Your Select statement.

   SELECT azon FROM $db ORDER BY azon ASC/DESC LIMIT 1
   

   you could simplify the statement like this:

   SELECT MAX(azon)/MIN(azon) AS 'azon' FROM $db
   

   There are some discussions about the gain of performance by using MIN/MAX instead of ORDER BY + LIMIT. I personally experienced that MIN/MAX is faster, especially on tables with many rows.

2. You can simply copy a table structure by using

   CREATE TABLE `$newdb` LIKE `$acttable`
   

   but you'd have to set the AUTO_INCREMENT manually after this, otherwise it would be set to 1.

   ALTER TABLE `$newdb` AUTO_INCREMENT=3524772
   

   (I don't see the purpose on this, maybe it's not even nessecary?)

3. Your optimize.

   • I'm quite irritated about the way to want to query this. You're calling that statement inside the else-condition -> after your delete has failed. It should be called inside the if-condition (or I missunderstood the goal).
     
   • You should consider using InnoDB instead of MyISAM (in general), because there would be no need (and possibility) of running an OPTIMIZE.

4. There should be no performance issue on the php-side as long as you do not iterate through the many rows. You may want to increase the max_execution_time-directive to prevent the script from stopping, but I consider this is not nessecary.

   On MySQL-side, i really suggest changing to InnoDB (you may want to read this).

---

Conclusion:
Besides the little changes of the statements and the use of OPTIMIZE inside your if-condition, there are no required improvements for exactly this script in my opinion. I strongly recommend to change the database engine when you make your changes for using PDO. This of course means more effort, as there are many variables which you can or should set to the InnoDB engine, but I think it is worth it. In fact, there aren't real advantages on using MyISAM, less then ever in performance.

Answer 2

I'm not sure, but from what I gather you are trying to access the first and last item in the $acttable with the following code?

$firstact= db_rows($acttable,"asc");
$lastact= db_rows($acttable,"desc");

If so, then this is the wrong way to go about it. You are essentially building the same array twice. Build it once, then use array_shift() and array_pop() respectively to get the first and last items from the resulting database row. Or if you need to reverse it and use entire array, use array_reverse().

Where are these magic numbers coming from 25000, 50000, 102400? What are they? Define these as constants to avoid confusion. For example:

define( "LIMIT",            25000 );
define( "MAX_ACT",     50000 );
define( "SIZE",             102400 );

Consider formatting your $newdbsql so that it is easier to read. Right now it is just one jumbled line. PHP doesn't care about white space, and to the best of my knowledge, neither does MySQL.

After you create the $newdbsql statement, you immediately try to print the results with test() before even applying it. This will throw up a warning because that variable is undefined. I copied the problem bit below. Since you seem to use the test() function quite frequently after querying a database, I would consider creating a mysql_query() wrapper that does this for you to avoid mistakes like this in the future. This applies the "Don't Repeat Yourself" (DRY) Principle. Which can probably be applied elsewhere in this script. I'm not sure if you are keeping the test() function in for cron, or if this is literally just for troubleshooting, so this may not be necessary.

//Problem Code
test($newresult);
$newresult = mysql_query($newdbsql);  
//Suggested Solution
function query( $sql ) {
    $result = mysql_query( $sql );
    test( $result );
    return $result;
}

The only efficiency problem I see is the number of database queries you are making. I don't know if its possible to lessen the amount, but I would try, as that's going to be a big factor.

Also, make sure your style remains consistent throughout. So far I've seen a few different styles in this code. This makes me think this was copy-pasted together.

Answer 3

Two minor notes:

1. The test function does not really test anything. I'd consider renaming it to log or something similar.

2. I'd move the logic into a function and use guard clauses instead of if-else branches. See: Flattening Arrow Code