Get all company branches using stored procedure

Question

Am creating an application for a transportation company that will store information about their drivers, the cars they drive and to what branches and districts they belong to.

I have a branch table in my database with the following data

branch_id   branch_name active  district_id
1           Naboa Road  1       76
2           Main Street 1       35
3           Bukoto      1       43
4           Najeera     1       110

and i have created this view to query it

CREATE OR REPLACE VIEW branch_view AS
SELECT b.branch_id,b.branch_name, b.active, d.district_id,
    CONCAT(d.district_name, ' ( ', d.district_region,' )') AS location
FROM branch AS b JOIN district AS d 
ON b.district_id = d.district_id

Now i have also created a stored procedure that returns either all data from that view or some of the data depending on the arguments given. Here is the stored procedure

DELIMITER $$
CREATE PROCEDURE get_branches( 
    branch_name_param VARCHAR(30), start_row INT, num_rows INT, OUT sql_param TINYTEXT 
)
BEGIN
    DECLARE select_clause VARCHAR(300);
    DECLARE where_clause  VARCHAR(300);

    SET select_clause = "SELECT branch_id, branch_name, location FROM branch_view";
    IF branch_name_param IS NULL THEN
        SET where_clause = " WHERE active ";
    ELSE
        SET branch_name_param = TRIM( BOTH ' ' FROM branch_name_param );
        SET where_clause = CONCAT( " WHERE branch_name LIKE '%", branch_name_param ,"%' " );
    END IF;

    SET select_clause = CONCAT( select_clause, where_clause );
    -- a small tweak to get total number of rows before limit clause is used
    SET sql_param = CONCAT( "SELECT COUNT(*) AS total_rows FROM branch_view", where_clause );

    SET select_clause = CONCAT( select_clause, " ORDER BY branch_name " );

    IF start_row IS NOT NULL THEN
        SET select_clause = CONCAT( select_clause, " LIMIT ", start_row, ", ", num_rows );
    END IF;

    SET @dynamic_sql = select_clause;

    PREPARE get_branches_stmt FROM @dynamic_sql;
    EXECUTE get_branches_stmt;
    DEALLOCATE PREPARE get_branches_stmt;
END$$
DELIMITER ;

The thing about this stored procedure though is that i get an sql statement through the fourth argument that i pass it, which i can use to execute later in my php application to find out how many rows would have been returned would there have not been a limit clause applied. Before you can kill and say why didn't i use SQL_CALC_FOUND_ROWS and FOUND_ROWS() information function to help me return information needed, i actually did that and found_rows function kept returning an incorrect total number of rows hence the above solution. Here is a my php code in case you need some closure, it takes a pdo connection object as it's first parameter, and the next parameters are to used in pagination on the application side that is the start_row parameter indicating which row to start with and num_rows parameter indicates how many rows to return and finally the where array parameter which contain key value pairs of my search in this case the branch name

/* Function to get all branches */
function get_branches( PDO $conn, $start_row, $num_rows, array $where = null ) {
    $sql = "CALL get_branches( :branch_name, :start_row, :num_rows, @sql_p )";
    try {
        $st = $conn->prepare($sql);
        $st->bindParam( ':branch_name', $where['name'], PDO::PARAM_STR | PDO::PARAM_NULL );
        $st->bindParam( ':start_row', $start_row, PDO::PARAM_INT | PDO::PARAM_NULL );
        $st->bindParam( ':num_rows', $num_rows, PDO::PARAM_INT | PDO::PARAM_NULL );
        $st->execute();
        $branches = array();
        foreach ( $st->fetchAll(PDO::FETCH_ASSOC) as $row ) {
            $branches[] = $row;
        }
        $st = null;
        // Get sql query returned by stored procedure
        $st = $conn->query("SELECT @sql_p AS `sql`");
        $row = $st->fetch(PDO::FETCH_ASSOC);
        // Execute sql query returned by stored procedure
        $st = $conn->query($row['sql']);
        $total = $st->fetch(PDO::FETCH_ASSOC);
        $st = null;
        return array( $branches, $total['total_rows'] );
    } catch (PDOException $e) {
        die( "<p>Query failed: " . $e->getMessage() . "</p>" );
    }
}

What i wanted to find out is, will this affect the performance of the database and my application and how can i improve it, if need be? Thank you

Answer 1

First,

I don't know if you are open to moving away from stored procedures. I really don't see a reason for it here, as all you are doing with the stored procedure is dynamically building a query. Could you not just do this in PHP so your application logic is not split across two areas?

To me, the only cases where I would want to consider stored procedures are:

• When you have a significant amount of query processing, such that if one were to put the logic in the application, it would result in a lot of back and forth data transfer between application and database. For example, say you were working with a significantly large table and you need to distill that data down to a smaller data set using some sort of algorithm. You may not want to bring the whole data set into the application to work with as this would be worse from a performance standpoint than perhaps working with memory tables in the DB.
• One might also use stored procedures in cases where the database handles requests from a variety of different applications and you need to provide a common abstraction for getting at data in the underlying tables without exposing the tables themselves. One could argue that this is the type of design you see less and less of in this day and age where service oriented architecture now generally serves as such an abstraction layer.

Neither of these is the use case here. The stored procedure is giving you no benefit at all. You could just as easily have that logic in your application and not "hide" it away from your application developer.

Second,

Don't use views in MySQL. They perform horribly.

Finally,

I have inserted code review comments into your PHP code. They are within multi-line /* */ comments below.

/*
Good job here of passing the PDO dependency to this method.
When you are trying to enforce an array type on $where parameter,
why would you have null as default value? Either make empty array
the default or don't provide a default value here at all if you want
null to be an error condition.
*/
function get_branches( PDO $conn, $start_row, $num_rows, array $where = null ) {
/*
You should validate $start_row, $num_rows, and $where values are as expected
and perhaps throw InvalidArgumentException if they are not.  I am guessing
$start_row and $num_rows should be validated as positive integer values. And
I am guessing that $where needs to be validated as not null
*/
/*
Here I would suggest adding your dynamic query building code in lieu of using stored procedure
*/
    $sql = "CALL get_branches( :branch_name, :start_row, :num_rows, @sql_p )";
    try {
/*
How about a more meaningful variable name?  Even $stmt would be better.
Nobody is handing out points for having short variable names.
*/
        $st = $conn->prepare($sql);
/*
You are just assuming prepare works here, which may be OK if you are working
in PDO exception mode, but still could be problematic doing happy path only
if you need to do something in code to handle errors besides just typical
exception handling.

You have not validated that $where['name'] even exists before working with it.
Why send in $where as an array parameter to this function,
when you only need the 'name' value?
Why not just send that value?
*/
        $st->bindParam( ':branch_name', $where['name'], PDO::PARAM_STR | PDO::PARAM_NULL );
        $st->bindParam( ':start_row', $start_row, PDO::PARAM_INT | PDO::PARAM_NULL );
        $st->bindParam( ':num_rows', $num_rows, PDO::PARAM_INT | PDO::PARAM_NULL );
        $st->execute();
        $branches = array();
/*
fetchAll will get the entire result set. So not sure why there is
a loop here.
This should probably be:
$branches = $st->fetchAll(PDO::FETCH_ASSOC);
*/
        foreach ( $st->fetchAll(PDO::FETCH_ASSOC) as $row ) {
            $branches[] = $row;
        }
/*
This line not needed
*/
        $st = null;
/*
Again this makes no sense, you are going to call a stored procedure that
returns a query to run against the database?  Very odd.

This whole section only considers happy path.
*/
       // Get sql query returned by stored procedure
        $st = $conn->query("SELECT @sql_p AS `sql`");
        $row = $st->fetch(PDO::FETCH_ASSOC);
        // Execute sql query returned by stored procedure
        $st = $conn->query($row['sql']);
        $total = $st->fetch(PDO::FETCH_ASSOC);
/*
This line not needed
*/
        $st = null;
/*
Why do you need to calculate total rows here?  Wouldn't that just be
the same as the size of the $branches array?
*/
        return array( $branches, $total['total_rows'] );
    } catch (PDOException $e) {
/*
Why die here?  Seems like a waste to catch an Exception only to die.
Should you rethrow exception to caller (or not catch at all) so it
can handle display of error to user somewhere up the call stack?
Why is this function responsible for displaying errors to users?
It should only do one single thing and succeed/fail as appropriate
so calling code (or something further up the call stack) can deal
with how to display errors to user.

Log the error, don't output directly to user from this function.
*/
        die( "<p>Query failed: " . $e->getMessage() . "</p>" );
    }
}