1
\$\begingroup\$

Here is my script which takes 50k records from a CSV and does CRUD operation with them. Unfortunately there are a lot of issues with the performance.

CSV fields are: msisdn, resellerid, product

Following are the steps performed:

  • Read the CSV.
  • Create Temp table.
  • Select top 1000 records and mark the status with random string to lock it.
  • Execute these records, meanwhile another request comes to this file then following locked status the next 1000 batch is undertaken.
  • It reads the locked status records and make the API call. If the call is positive then it creates the record of the customer account, else continue for next record's API call .

The code is working fine if we upload a CSV of records 1k, 2k, up to 7k.

When we upload like 10k and above the same records which are in locked status are still called twice thrice and many times, leading to unwanted API calls which needs to be avoided. Sometimes when the API call is successful even then customer account is not getting created.

I am hereby pasting the code so that highly skilled PHP and MySQL experts can look into the code and suggest their improvements.

<?php
/*********************************************************************************
 ** The contents of this file are subject to the crm CRM Public License Version 1.0
 * ("License"); You may not use this file except in compliance with the License
 * The Original Code is:  crm CRM Open Source
 * The Initial Developer of the Original Code is crm.
 * Portions created by crm are Copyright (C) crm.
 * All Rights Reserved.
 *
 ********************************************************************************/
ini_set('max_execution_time', 30000);
set_time_limit(0);

global $adb, $log;
require_once('include/utils/utils.php');
require_once('include/database/PearDatabase.php');
include_once 'includes/main/WebUI.php';

$result   = $adb->pquery("SELECT bulkcustomerid,billingarea,apilogid FROM crm_bulkcustomer where `statusrecord` = 'inprogress'");
$noOfRows = $adb->num_rows($result);

if (!$noOfRows) {
    $result   = $adb->pquery("SELECT bulkcustomerid,billingarea,apilogid FROM crm_bulkcustomer where `statusrecord` = 'new'");
    $noOfRows = $adb->num_rows($result);
}
if (!$noOfRows) {
    // add inner join with crm_crmentity for created date interval day for one day.
    //$result     = $adb->pquery("SELECT bulkcustomerid,billingarea,apilogid FROM crm_bulkcustomer where `statusrecord` = 'closedwithfailure'");
    $result     = $adb->pquery("SELECT bulkcustomerid,billingarea,apilogid FROM crm_bulkcustomer  inner join crm_crmentity on 
                                crm_bulkcustomer.bulkcustomerid=crm_crmentity.crmid where `statusrecord` = 'closedwithfailure'");
    $noOfRows   = $adb->num_rows($result);
    $isClosedWf = TRUE;
}
$process_data = 1000;

$log->debug('TOTAL NUMBEROFCSVS ' . $noOfRows);

$length       = 9;
$randomString = substr(str_shuffle("0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"), 0, $length);

if ($noOfRows > 0) { // for number of csv files .
    for ($i = 0; $i < $noOfRows; $i++) {
        $bulkcustomerid = $adb->query_result($result, $i, 'bulkcustomerid');
        $apilogid       = $adb->query_result($result, $i, 'apilogid'); // update the records for processing 
        $billingarea       = $adb->query_result($result, $i, 'billingarea');  
        if (!$apilogid) {
            $apilogid = 0;
        }

        $adb->query("UPDATE crm_bulkcustomer SET `statusrecord` = 'inprogress' where `bulkcustomerid` = '$bulkcustomerid'");

        if ($isClosedWf) {
            $log->debug('IN CLOSED WITH FAILURE PART');
            //sleep(6800);
            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '$randomString' where STATUS = '0' OR STATUS REGEXP '[a-z]' ORDER BY id ASC LIMIT $process_data");

            $servicetype = "Prepaid";
            $log->debug('CRON TASK - After validating Call Create Customer API for Record ID ' . $bulkcustomerid);
            $log->debug('select msisdn,package from temp_csv_$bulkcustomerid where status=' . $randomString . ' limit' . $process_data . ' FOR UPDATE');
            //$transaction = "START TRANSACTION;";
            //$adb->pquery($transaction, array());
            $result2   = $adb->query("select msisdn,package from temp_csv_$bulkcustomerid where status='$randomString' limit $process_data");
            $noOfRows2 = $adb->num_rows($result2);

            $log->debug('Total row for this table is:- ' . $noOfRows2);
            if ($noOfRows2 > 0) {
                for ($j = 0; $j < $noOfRows2; $j++) {
                    $msisdnnum = $adb->query_result($result2, $j, 'msisdn');
                    $package   = $adb->query_result($result2, $j, 'package');
                   /***** commented MSISDN 
                    $log->debug('CRON TASK - Call Create Customer API for Table ID temp_csv_' . $bulkcustomerid . 'and mobile number is' . $msisdnnum);
                    $log->debug("for IF part BulkCustomer_SearchSim_Action::search_msisdn($msisdnnum,$apilogid)");
                    $result1 = BulkCustomer_SearchSim_Action::search_msisdn($msisdnnum, $apilogid, $bulkcustomerid);
                    $message = $result1['message'];
                    $code    = $result1['code'];
                    if ($code == 0) {
                        $log->debug('MESSAGE IS--for code 0 ' . $message . 'code is--' . $code);
                        $log->debug('Sucess UPDATE crm_bulkcustomer SET statusrecord = closed where bulkcustomerid=' . $bulkcustomerid);
                        ******/
                        $bulkresult = BulkCustomer_CreateCustomer_Action::Create_Customer($msisdnnum, $bulkcustomerid, $package, $servicetype,$billingarea,$apilogid);
                        $log->debug('CUSTOMER WITH ALL ACCOUNTS CREATED SUCCESSFULLY FOR MOBILENO=' . $msisdnnum . 'and current j value is=' . $j . 'and no of records to parse are' . $noOfRows2);
                        $bulkcode    = $bulkresult['code'];
                        $bulkmessage = $bulkresult['message'];

                        if ($bulkcode == '0') {
                            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '1',remarks='$bulkmessage' where `msisdn` = '$msisdnnum'");
                        } else {
                            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '-6',remarks='$bulkmessage' where `msisdn` = '$msisdnnum'");
                        }
                   /****** search MSISDN } else {
                        $log->debug('MESSAGE IS-- for code -1 ' . $message . 'code is--' . $code);
                        $log->debug('Failure UPDATE temp_csv_' . $bulkcustomerid . 'SET status = Failed where msisdn =' . $msisdnnum);
                        $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '-9',remarks='$message' where `msisdn` = '$msisdnnum'");
                    }
                    search MSISDN *****/
                    $log->debug('j value is for loop' . $j);
                }
            }else{
              $log->debug('No data to parse');
              $adb->query("UPDATE `crm_bulkcustomer` SET `statusrecord` = 'closed' where `bulkcustomerid` = '$bulkcustomerid'");
            }

        } else { // for locking the columns with -7
            $log->debug('IN PROGESS OR NEW PART');
            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '$randomString' where `status` = '0' ORDER BY id ASC LIMIT $process_data");
            $servicetype = "prepaid";
            $log->debug('CRON TASK - After validating Call Create Customer API for Record ID ' . $bulkcustomerid);
            $log->debug('select msisdn,package from temp_csv_$bulkcustomerid where status=' . $randomString . ' limit' . $process_data . 'FOR UPDATE');
            //$transaction = "START TRANSACTION;";
            //$adb->pquery($transaction, array());
            $result2   = $adb->query("select msisdn,package from temp_csv_$bulkcustomerid where status='$randomString' limit $process_data");
            $noOfRows2 = $adb->num_rows($result2);
            $log->debug('Total row for this table is:- ' . $noOfRows2);
            if ($noOfRows2 > 0) {
                for ($j = 0; $j < $noOfRows2; $j++) {
                    $msisdnnum = $adb->query_result($result2, $j, 'msisdn');
                    $package   = $adb->query_result($result2, $j, 'package');
                    /***** commented MSISDN 
                    $log->debug("ELSE BulkCustomer_SearchSim_Action::search_msisdn($msisdnnum,$apilogid)");
                    $result1 = BulkCustomer_SearchSim_Action::search_msisdn($msisdnnum, $apilogid, $bulkcustomerid);
                    $message = $result1['message'];
                    $code    = $result1['code'];
                    if ($code == 0) {
                        $log->debug('MESSAGE IS--for code 0 ' . $message . 'code is--' . $code);
                        $log->debug('Sucess UPDATE crm_bulkcustomer SET statusrecord = closed where bulkcustomerid=' . $bulkcustomerid);
                        ******/
                        $bulkresult = BulkCustomer_CreateCustomer_Action::Create_Customer($msisdnnum, $bulkcustomerid, $package, $servicetype,$billingarea,$apilogid);
                        $log->debug('CUSTOMER WITH ALL ACCOUNTS CREATED SUCCESSFULLY FOR MOBILENO=' . $msisdnnum . 'and current j value is=' . $j . 'and no of records to parse are' . $noOfRows2);
                        $bulkcode    = $bulkresult['code'];
                        $bulkmessage = $bulkresult['message'];

                        if ($bulkcode == '0') {
                            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '1',remarks='$bulkmessage' where `msisdn` = '$msisdnnum'");
                        } else {
                            $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '-6',remarks='$bulkmessage' where `msisdn` = '$msisdnnum'");
                        }
                  /******  } else {
                        $log->debug('Failure UPDATE temp_csv_' . $bulkcustomerid . 'SET status = Failed where msisdn =' . $msisdnnum);
                        $adb->query("UPDATE temp_csv_$bulkcustomerid SET `status` = '-9',remarks='$message' where `msisdn` = '$msisdnnum'");
                    }
                     comments ends for MSISDN ****/
                    $log->debug('j value is for loop' . $j);
                }
            }else{
              $log->debug('No data to parse inprogress or new part');
              $adb->query("UPDATE `crm_bulkcustomer` SET `statusrecord` = 'closedwithfailure' where `bulkcustomerid` = '$bulkcustomerid'");
            }
            //commit
        } // for else part
        // close the csv file 
        // get count of sucess and failure.
        $sucesscount = $adb->pquery("SELECT * FROM temp_csv_$bulkcustomerid where `status` = '1'"); // fetch record for new
        $noofsucess  = $adb->num_rows($sucesscount);
        $adb->query("UPDATE `crm_bulkcustomer` SET `successcount` = '$noofsucess' where `bulkcustomerid` = '$bulkcustomerid'");

        // failure count
        $failurecount       = $adb->pquery("SELECT * FROM temp_csv_$bulkcustomerid where status!='1'"); // fetch record for fails
        $nooffailure        = $adb->num_rows($failurecount);
        $reprocesscount     = $adb->pquery("SELECT * FROM temp_csv_$bulkcustomerid where  STATUS = '0' OR STATUS REGEXP '[a-z]'");
        // fetch record for fails
        //$noofreprocesscount = 0;
        $noofreprocesscount = $adb->num_rows($reprocesscount);
        $log->debug("UPDATE `crm_bulkcustomer` SET `failurecount` = '$nooffailure' where `bulkcustomerid` = '$bulkcustomerid'");
        if ($nooffailure == '0') {
            $adb->query("UPDATE `crm_bulkcustomer` SET `statusrecord` = 'closed' where `bulkcustomerid` = '$bulkcustomerid'");
            $adb->query("UPDATE `crm_bulkcustomer` SET `failurecount` = '' where `bulkcustomerid` = '$bulkcustomerid'");
        } elseif ($noofreprocesscount) {
            // check if it is being closed before all records are executed 
            $adb->query("UPDATE `crm_bulkcustomer` SET `statusrecord` = 'inprogress' where `bulkcustomerid` = '$bulkcustomerid'");
        } else {
            $adb->query("UPDATE `crm_bulkcustomer` SET `failurecount` = '$nooffailure' where `bulkcustomerid` = '$bulkcustomerid'");
            $adb->query("UPDATE `crm_bulkcustomer` SET `statusrecord` = 'closedwithfailure' where `bulkcustomerid` = '$bulkcustomerid'");

            $filename = "$bulkcustomerid" . "-csv";
            $file     = fopen("tempcsv/$filename.csv", "w");
            $val      = array(
                'MSISDN',
                'ResellerID',
                'Package',
                'Remarks'
            );
            fputcsv($file, $val, ';', ' ');
            $csvresult = $adb->query("select msisdn,package,resellerid,remarks from temp_csv_$bulkcustomerid where status!='1' ");
            $csvrows   = $adb->num_rows($csvresult);
            $log->debug('Total row for this table in CSV PART is:- ' . $csvrows);
            if ($csvrows > 0) {
                for ($m = 0; $m < $csvrows; $m++) {
                    $msisdnnum  = $adb->query_result($csvresult, $m, 'msisdn');
                    $package    = $adb->query_result($csvresult, $m, 'package');
                    $resellerid = $adb->query_result($csvresult, $m, 'resellerid');
                    $remarks    = $adb->query_result($csvresult, $m, 'remarks');
                    $remarks    = preg_replace('/\s+/', '--', $remarks);
                    $arrayval   = array(
                        "$msisdnnum",
                        "$resellerid",
                        "$package",
                        "$remarks"
                    );
                    fputcsv($file, $arrayval, ';', ' ');

                }
            }
        }
    }

} else {
    $log->debug('No status with NEW or Pending of CSV RECORDS found');
}
?>
\$\endgroup\$
2
\$\begingroup\$

Initial thoughts on this code:

  • one long and very convoluted chunk. It badly needs refactoring into smaller and more easily understood pieces.

  • various commented out lines, which obscure the logic.

  • SQL statements littered through the code leading to lots of replication and little reuse.

  • debugging code that prints out what will seem to be the query executed, but which is actually a different string: debugging should print the actual string used, not one that is hopefully the same. (See lines 168 vs 171 / 178)

Suggestions:

  • Turn the code into around 10 functions, each of which does one thing well. For example, a function that returns the number of customers, or one that writes an array as (your sort of) csv file.

  • As mentioned in other review answers, use exceptions to report exceptional conditions, which will make the "normal case" logic clearer. Don't forget to catch the exceptions though!

  • Always check what called functions return. For example, in the first lines the code determines a noOfRows value, and error checks it by "(!noOfRows)". Was that zero rows - i.e. there were no customers - or was it that the database wasn't running? Using discrete functions makes it easier to ensure you always do these checks and don't get lazy.

  • Always: one source of truth. Never duplicate logic or data unless there is truly no choice.

  • Use a code editor that supports tools like "php code sniffer" and use it.

Performance:

  • This is not a "debug my code' site, so I'll simply say: it is likely your script is having memory troubles, possibly because of data not being garbage collected. Splitting code into functions will assist php to get this right (because a function is a scope block).
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.