Closed (works as designed)
Project:
IP-based Determination of a Visitor's Country
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
12 Oct 2012 at 10:32 UTC
Updated:
12 Sep 2013 at 02:29 UTC
Jump to comment: Most recent file
Comments
Comment #1
tcmug commentedPatch here:
Comment #3
tcmug commentedApparently I screwed up the patch, heres another go:
Comment #4
tcmug commentedAnd again, this time marking for review.
Comment #5
gregglesThis seems reasonable to me. @tcmug - do you have an explain plan from before and after?
Comment #6
gregglesHere's an explain plan for the select query before/after:
MariaDB [cap]> explain SELECT country FROM ip2country WHERE (1262949420 >= ip_range_first AND 1262949420 <= ip_range_last) LIMIT 1;
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
| 1 | SIMPLE | ip2country | ALL | NULL | NULL | NULL | NULL | 1 | Using where |
+------+-------------+------------+------+---------------+------+---------+------+------+-------------+
1 row in set (0.00 sec)
MariaDB [cap]> explain SELECT country FROM ip2country WHERE (1262949420 >= ip_range_first AND 1262949420 <= ip_range_last) LIMIT 1;
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows |
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+
| 1 | SIMPLE | ip2country | range | ip | ip | 8 | NULL | 1 | Using index condition |
+------+-------------+------------+-------+---------------+------+---------+------+------+-----------------------+
The number of rows returned stays the same, but the method of finding them changes.
Comment #7
gregglesI did a bit of lightweight testing and didn't see any benefit from this change. An index will make the insert process slower at the db level, but in my experience that process is actually bottlenecked on php and it only runs infrequently.
So....without some benchmarking that shows this as a definite improvement I don't see a reason to make the change.
Comment #8
tr commentedI see a similar explain for MySQL.
I don't think the IP lookup causes any significant performance bottleneck. My tests show it to take less than a millisecond. That's added only to the user login time, not to every page load, so I don't think this poses any real problem.
My concern with performance is with the time it takes to update the database. Recent changes to this module have made it so the update can occur in the background without impacting the functionality of the module at all, so the update time is now less of a concern. (It used to be that the module wouldn't function properly while the database was being updated, because the updating process first dropped the existing data and THEN added the new data, so if it took a long time to update the data, the module would be unusable for a long time. That's no longer the case.)
The lookup time is a one-time-only penalty, and I don't think that it's worth changing the code unless you can demonstrate significant performance improvements for the change. That is not the case here.
Comment #9
gregglesThat depends on your use case, actually. The way I use the module is that every visit to the site checks this lookup table.
Overall, though, I agree with your assessment.