insert id manually

[php_begins]

i was working on modifying a code written by a previous user.

i have field called adid in the ads table which should have been a primary key and autoincremented.

But since we already have lot of data across different sites we cannot modify the table structure.

The problem is i want to check if the following code is adding adid manually.is there some problem with this code?

 

 

 

   $get_ad_id = mysql_query("SELECT adid FROM ads ORDER BY adid DESC LIMIT 1", $con) or die("Error Getting AD ID: ".mysql_error());
    while($get_ad_id_results = mysql_fetch_assoc($get_ad_id)) { $adid=$get_ad_id_results['adid']; }
    if($adid!="") { $adid=$adid+1; }
    else { $adid="1"; }
    if($bannertype=="120x60" || $bannertype=="468x60")
     {
      $New_URL = $_POST['URL'];
      $active='Y';
      $insert_image_banner_query="INSERT INTO ads (adid, URL, image_path, date, userid, active, size) VALUES ('".$adid."','$New_URL', '$newname', '$Date', '$loggedinuserid','".$active."','".$bannertype."')";
      mysql_query($insert_image_banner_query, $con) or die("Inserting Image Ad Failed: ".mysql_error());
      $update_banner_cache="YES";
     }

[QuickOldCar]

$adid if not empty will make the new $adid a value of $adid +1, which to me there could always be that adid somewhere

 

I would probably do it differently.

select the highest adid in your databases, ..then add +1 to the highest one and set that as your new $adid.

 

Also I would never make the adid a value of 1, you could have multiple 1's throughout.

 

if($adid!="") {
$adid=$adid+1;
} else {
$adid="1";
}

 

something in the lines of this

if($adid!="") {
$idquery = "SELECT MAX(adid) FROM ads";
$idrow = mysql_fetch_row($idquery);
$adid = $idrow[0] +1;
}

[Psycho]

You can change the field to be an auto-increment field without causing problems. Just determine the highest ID in use and set teh start counter above that.

[QuickOldCar]

I agree mjdamato

 

In any case, I forgot to add the mysql query, so fixed it

 

if($adid!="") {
$idquery = mysql_query("SELECT MAX(adid) FROM ads");
$idrow = mysql_fetch_row($idquery);
$adid = $idrow[0] +1;
}

[php_begins]

Thank you quickoldcar..

MJ, ill keep that in mind

[QuickOldCar]

I think there is a mistake.

 

It was making a new adid only if was not blank, it should make one if it is blank

 

if($adid == "") {
$idquery = mysql_query("SELECT MAX(adid) FROM ads");
$idrow = mysql_fetch_row($idquery);
$adid = $idrow[0] +1;
}

[SergeiSS]

Another possibility is to create INSERT trigger and set a new value for adid in this trigger. But then I don't know how to get it back from MySQL into PHP... In Postgre it's easy.

The benefit is that you do it just in one place but not in many different scripts.

[Psycho]

Using something like this is a very bad idea.

if($adid == "") {
$idquery = mysql_query("SELECT MAX(adid) FROM ads");
$idrow = mysql_fetch_row($idquery);
$adid = $idrow[0] +1;
}

 

1) It is inefficient. DB queries are expensive transactions. Running two queries just to insert a record is not wise.

2) MySQL will do a much better job of managing IDs than you or I can

3) You leave yourself open for creating database inconsistencies that will corrupt your database.

 

In the unlikely event that two records are submitted at the same time, you could end up with records that have duplicate IDs.

 

As stated previously, change the field to be an auto-increment primary key field and set the start value to be higher than the current highest ID. Then do not include the ID field in the insert clauses and let MySQL create it for you.

 

Besides, if you did need to get the number that is one more than the current max value (which you should still not do in this instance) a more appropriate solution would be

if($adid == "")
{
    $query = "SELECT MAX(adid)+1 FROM ads";
    $result = mysql_query($query);
    $adid = mysql_result($result, 0);
}