FR 2.2.1 rlm_sqlippool / mysql 5.5.31 - deadlocks
Hello, I have recently switched from ippool to sqlippool. I decided to use Calling-Station-Id as a pool-key: pool-key = "%{Calling-Station-Id}" related queries in sqlippool.conf: allocate-find = "SELECT framedipaddress FROM ${ippool_table} WHERE pool_name = '%{control:Pool-Name}' AND expiry_time IS NULL ORDER BY RAND() LIMIT 1 FOR UPDATE" allocate-update = "UPDATE ${ippool_table} SET nasipaddress = '%{NAS-IP-Address}', pool_key = '${pool-key}', expiry_time = NOW() + INTERVAL ${lease-duration} SECOND WHERE framedipaddress = '%I' AND expiry_time IS NULL" allocate-clear = "UPDATE ${ippool_table} SET nasipaddress = '', pool_key = 0, expiry_time = NULL WHERE expiry_time <= NOW() - INTERVAL 1 SECOND" and radippool table structure: CREATE TABLE `radippool` ( `id` int(11) NOT NULL AUTO_INCREMENT, `pool_name` varchar(30) NOT NULL, `framedipaddress` varchar(15) NOT NULL DEFAULT '', `nasipaddress` varchar(15) NOT NULL DEFAULT '', `expiry_time` datetime DEFAULT NULL, `pool_key` varchar(30) NOT NULL, PRIMARY KEY (`id`), KEY `radippool_poolname_expire` (`pool_name`,`expiry_time`), KEY `framedipaddress` (`framedipaddress`), KEY `radippool_nasip_poolkey_ipaddress` (`nasipaddress`,`pool_key`,`framedipaddress`) ) ENGINE=InnoDB AUTO_INCREMENT=65025 DEFAULT CHARSET=latin1; At the first glance everything seemed to be fine, however after some time, I realized that there were a number of deadlocks every now and then. Attached you can sample outputs from "SHOW ENGINE INNODB STATUS" I have tried to lower the transaction isolation level to READ-COMMITED, but it didn't help. Investigating the source code for rlm_sqlippool brought me to an idea that moving "CLEAR" query to the distinct transaction can help, because I can't really see any good reason to keep it in the same transaction with "FIND". I have made required minor changes and gave it a try: for two days the server is running without a single deadlock. $ diff orig/rlm_sqlippool.c rlm_sqlippool.c 586a587,598
* COMMIT */ sqlippool_command(data->allocate_commit, sqlsocket, instance, request, (char *) NULL, 0);
/* * BEGIN */ sqlippool_command(data->allocate_begin, sqlsocket, data, request, (char *) NULL, 0);
/*
$ uname -a Linux radius 3.2.0-4-amd64 #1 SMP Debian 3.2.51-1 x86_64 GNU/Linux $ mysql --version mysql Ver 14.14 Distrib 5.5.31, for debian-linux-gnu (x86_64) using readline 6.2 Best Regards, -- George Chelidze Software Developer Magticom Ltd.
George Chelidze wrote:
Investigating the source code for rlm_sqlippool brought me to an idea that moving "CLEAR" query to the distinct transaction can help, because I can't really see any good reason to keep it in the same transaction with "FIND". I have made required minor changes and gave it a try: for two days the server is running without a single deadlock.
Looks good. I'll commit a patch. It should also be doing the CLEAR only once per second. It doesn't make sense to do it on every FIND. That's just unnecessary overhead. Alan DeKok.
On 25/11/13 14:31, Alan DeKok wrote:
George Chelidze wrote:
Investigating the source code for rlm_sqlippool brought me to an idea that moving "CLEAR" query to the distinct transaction can help, because I can't really see any good reason to keep it in the same transaction with "FIND". I have made required minor changes and gave it a try: for two days the server is running without a single deadlock.
Looks good. I'll commit a patch.
It's probably worth making the 2nd transaction conditional on the "clear" query being defined. Personally I think this is a MySQL problem, not a FR/rlm_sql_ippool one.
Phil Mayers wrote:
It's probably worth making the 2nd transaction conditional on the "clear" query being defined.
It's required to be set. The module fails to initialize if it's empty.
Personally I think this is a MySQL problem, not a FR/rlm_sql_ippool one.
Quite possibly. In any case, the fix won't hurt a real database, and it will help MySQL. Alan DeKok.
On 2013-11-25 18:31, Alan DeKok wrote:
Looks good. I'll commit a patch.
It should also be doing the CLEAR only once per second. It doesn't make sense to do it on every FIND. That's just unnecessary overhead.
Alan DeKok.
Hello Alan, Feeling stupid to ask this, but shouldn't you use some kind of locking while reading/writing data->last_clear? Do you rely on atomicity of int? now = time(NULL); if (data->last_clear < now) { data->last_clear = now; ... } Another questions is related to the "CLEAR" operation: do we really need to update "expired" records and then retrieve one from unassigned list? Instead we could take an address if it's unassigned or assigned but expired, something like: SELECT framedipaddress FROM ${ippool_table} WHERE pool_name = '%{control:Pool-Name}' AND expiry_time IS NULL OR expiry_time <= NOW() - INTERVAL 1 SECOND ORDER BY RAND() LIMIT 1 FOR UPDATE OR is inefficient here so we can change expiry_time from: `expiry_time` datetime DEFAULT NULL to `expiry_time` datetime NOT NULL DEFAULT '0000-00-00 00:00:00' and later use the following query: SELECT framedipaddress FROM ${ippool_table} WHERE pool_name = '%{control:Pool-Name}' AND expiry_time <= NOW() - INTERVAL 1 SECOND ORDER BY RAND() LIMIT 1 FOR UPDATE Yes, I know I have covered mysql case only, but I believe the similar approach can be used with other databases. BR, -- George Chelidze Software Developer Magticom Ltd.
George Chelidze wrote:
Feeling stupid to ask this, but shouldn't you use some kind of locking while reading/writing data->last_clear? Do you rely on atomicity of int?
Writes to "int" are atomic. Even if they're not, there's minimal cost to doing the "clear" 2-3 times. The key is to avoid doing the clear 1000 times a second.
Another questions is related to the "CLEAR" operation: do we really need to update "expired" records and then retrieve one from unassigned list? Instead we could take an address if it's unassigned or assigned but expired, something like:
Yes, that would work. The suggestion to change the expiry time to NOT NULL is a good one. Having multiple possibilities for a field causes issues. If it's always a date, it's simpler.
Yes, I know I have covered mysql case only, but I believe the similar approach can be used with other databases.
As always, patches are welcome. :) Alan DeKok.
On 26 Nov 2013, at 13:55, Alan DeKok <aland@deployingradius.com> wrote:
George Chelidze wrote:
Feeling stupid to ask this, but shouldn't you use some kind of locking while reading/writing data->last_clear? Do you rely on atomicity of int?
Writes to "int" are atomic. Even if they're not, there's minimal cost to doing the "clear" 2-3 times.
A write to uint32_t on a 16bit system wouldn't be atomic, as a write to a uint64_t on a 32bit system wouldn't be atomic. But somehow I don't think this will be an issue on production systems. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
participants (4)
-
Alan DeKok -
Arran Cudbard-Bell -
George Chelidze -
Phil Mayers