Optimize this SQL query

Question

I've got a SQL query within a foreach loop. Sometimes there can be many, and I mean a lot of queries to do, depending on several criteria, up to 78 queries potentially.

Now, I know that premature optimisation is root cause of all evil, but I don't want to see 78 queries - it's just not healthy.

Here's the code:

$crumbs = explode(",", $user['data']['depts']);

foreach ($crumbs as &$value) {
    $data = $db->query("SELECT id FROM tbl_depts WHERE id = '" . $value . "'");
    $crumb = $data->fetch_assoc();
    $dsn = $db->query("SELECT msg, datetime FROM tbl_motd WHERE deptid = '" . $value . "'");
    $motd = $dsn->fetch_assoc();
    if ($motd['msg'] != "") {
        <?php echo $motd['msg']; ?>
    }
}

Can I make it any better?

Aside from the optimizations below, you might want to consider making it a stored procedure instead of a straight select to improve performance. — nickytonline
– nickytonline, Commented Jun 27, 2011 at 2:09
be aware that this code is vulnerable to sql injection attacks. — Daniel A. White
– Daniel A. White, Commented Jun 27, 2011 at 11:53
Hello Daniel, why is this the case? The data is only getting an array, which would have been sanitised before putting in. Aldo, $user['data'] and its children are also sanitised before this code. — bear
– bear, Commented Jun 27, 2011 at 11:55

Nemoden · Accepted Answer · 2011-06-27 01:33:13Z

1

Use IN MySQL operator to search over a set of values for id:

$ids = '"' . implode('", "',$crumbs) . '"';
$query1 = "SELECT id FROM tbl_depts WHERE id IN (" . $ids . ")";
$query2 = "SELECT msg, datetime FROM tbl_motd WHERE deptid IN (" . $ids . ")";

And so you will not need to retrieve all data you need using foreach loop, so you will have only 2 queries instead of 78.

Example: I have a table named table with 10 records which ids are: 1,2,3,4,5,6,7,8,9,10 (auto-incremented). I know I need records with ids 1,5,8. My query will be:

$sql = "SELECT * FROM `table` WHERE id in (1,5,8);";

And I don't understand why do you need to use & operator in foreach loop if you don't modify the $crubms arrays values.

edited Jun 27, 2011 at 1:33

answered Jun 27, 2011 at 1:26

Nemoden

9,0866 gold badges45 silver badges66 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

bear Over a year ago

So I would replace the two queries above with this, but rewrite the code itself?

bear Over a year ago

It's just habit that I put & operator in.

Nemoden Over a year ago

@Shamil, Yes. You will have to rewrite the code since it uses another approach.

Nemoden Over a year ago

@Shamil it's a bad habit then. You might occasionally modify any array in your program and it will be hard to find out where an error is hidden.

Mike Neumegen Over a year ago

Using the & operator will make the program run slightly faster as the program doesn't need to allocate memory to store each $value in.

|

David Rodrigues · Accepted Answer · 2011-06-27 01:29:02Z

1

I think this is want you want.

SELECT msg, datetime
FROM tbl_depts td
INNER JOIN tbl_motd tm ON td.id = tm.deptid

edited Jun 27, 2011 at 1:29

David Rodrigues

12.6k17 gold badges68 silver badges96 bronze badges

answered Jun 27, 2011 at 1:10

Craig Suchanec

10.9k4 gold badges34 silver badges39 bronze badges

2 Comments

bear Over a year ago

which one should I replace it with?

Nemoden Over a year ago

I did not really looked into the logic of queries, but I think you've forgotten WHERE somewhere. I think OPs query result is restricted by a set of particular ids. Should be INNER JOIN tbl_motd tm ON td.id = tm.deptid WHERE td.id in (<set of ids>), probably?

Collectives™ on Stack Overflow

Optimize this SQL query

2 Answers 2

6 Comments

2 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

2 Comments

Linked

Related