16

I have on my PHP file a function that check if an IP is banned or not. For some reason my site is very slow and the problem is when I check if the IP is banned or not.

(I remove the code that checks and my site was fast again)

Here's my code:

// index.php - everything redirects to this file in the .htaccess
<?php
include('config.php');
if(isIpBanned($_SERVER['REMOTE_ADDR'])) {
 die('access denied');
}
// rest of the code

here's my function

// config.php
<?php
function isIpBanned($db, $ip) { // $db is declared correctly
 $goodIP = $db->getRecord("SELECT is_banned FROM security.ip WHERE ip = '$ip'"); // this function works and return 1 or 0
 return (bool)$goodIP;
}

This query takes about 2 seconds to 3 seconds to run. Why? I don't have left join or other tables.

Thanks

Gino Sullivan
  • 2,203
  • 18
  • 29

2 Answers2

26
  1. Put a (unique?) index on the IP column
  2. Use the correct datatype by converting the textual representation to a "native" one (an ipv4 fits in a INT UNSIGNED, an ipv6 in 2 BIGINT UNSIGNED): this will make your tables smaller, and will require less I/O during scans

and, as a side note, even if $_SERVER["REMOTE_ADDR"] should be safe, NEVER FORGET TO ESCAPE THE DATA IN SQL QUERIES!

CAFxX
  • 28,060
  • 6
  • 41
  • 66
  • 1.1.1.1 is not an interger, how can you have INT as the data type? – Gino Sullivan Nov 02 '11 at 11:16
  • @Gino lookup the `ip2long` and `long2ip` PHP functions or the `INET_ATON()` and `INET_NTOA()` MySQL functions – CAFxX Nov 02 '11 at 11:17
  • INT is faster than CHAR(20) for lookup? – Gino Sullivan Nov 02 '11 at 11:20
  • @GinoSullivan it doesn't matter. you have to focus on indexing. – Your Common Sense Nov 02 '11 at 11:39
  • 1
    @Col.Shrapnel well after some digging, I found using a varchar/char is not recommended for lookups, INT is better. Also, I found using a range of IPs and not looking for each individual IPs (1.1.1.1 to 1.1.1.255 - for example but instead > upperboundIP INT number). So easier to maintain personally. -CodeCaster has a good solution but using 2 tables for IP is not, I think, recommended – Tech4Wilco Nov 02 '11 at 15:10
  • @Tech better, better okay. do all micro-optimizations you wish **but create an SQL index first,** as it's the only real solution. – Your Common Sense Nov 02 '11 at 15:22
  • @Col.Shrapnel I agree the REAL solution is the index no doubt about that – Tech4Wilco Nov 02 '11 at 15:39
  • 2
    @Col.Shrapnel that's why using proper indexing is point #1 on my list... The second is just an addition (but, in my experience, sometimes this kind of data type issues will cause massive slowdowns; see e.g. http://stackoverflow.com/questions/4777070/hamming-distance-on-binary-strings-in-sql) – CAFxX Nov 02 '11 at 16:58
2

Put an index on the ip column.

There's literally tons of information out on the web on query analysing and improveemnt so I'm not gonna repeat that for you, but an index will most definetely help.

I guess security.ip is a very large table, so the lookup becomes slow.

The drawback of an index: writing becomes somewhat slower, so if you write a lot to that table, you could try to offload the banning part to a new table, say banned_ips.

CodeCaster
  • 147,647
  • 23
  • 218
  • 272